Repository: networkx/networkx
Pull request: fix issue #4173: cytoscape_graph(input_data) did modify the original data
I added a test that failed before this change.  Fixes #4173.

Problem statement:
readwrite.cytoscape_graph(data) removes "source" and "target" fields from edges
Hi,

I realized the function to get a graph from cytoscape data modifies the data that is received as argument. Is there any reason for that? I couldn't find that in the documentation, and I found it when browsing the code.

Thank you in advance!

Hints from issue discussion:
> I realized the function to get a graph from cytoscape data modifies the data that is received as argument. Is there any reason for that?

This looks like a defect to me, I don't think the input data dict should be modified. An MRE:

```python
>>> G = nx.path_graph(4)
>>> data_dict = nx.cytoscape_data(G)
>>> data_dict
{'data': [],
 'directed': False,
 'multigraph': False,
 'elements': {'nodes': [{'data': {'id': '0', 'value': 0, 'name': '0'}},
   {'data': {'id': '1', 'value': 1, 'name': '1'}},
   {'data': {'id': '2', 'value': 2, 'name': '2'}},
   {'data': {'id': '3', 'value': 3, 'name': '3'}}],
  'edges': [{'data': {'source': 0, 'target': 1}},
   {'data': {'source': 1, 'target': 2}},
   {'data': {'source': 2, 'target': 3}}]}}
>>> H = nx.cytoscape_graph(data_dict)
>>> data_dict   # 'source' and 'target' keys no longer present for edges
{'data': [],
 'directed': False,
 'multigraph': False,
 'elements': {'nodes': [{'data': {'id': '0', 'value': 0, 'name': '0'}},
   {'data': {'id': '1', 'value': 1, 'name': '1'}},
   {'data': {'id': '2', 'value': 2, 'name': '2'}},
   {'data': {'id': '3', 'value': 3, 'name': '3'}}],
  'edges': [{'data': {}}, {'data': {}}, {'data': {}}]}}
```

> I couldn't find that in the documentation, and I found it when browsing the code.

Unfortunately `cytoscape_graph` is also missing a docstring! Thanks for catching this as well.
FWIW this should be a very straightforward fix that is suitable for new contributors. To do so would require the following:
 1. Add/modify a test in `networkx/readwrite/json_graph/tests/test_cytoscape.py` that will fail with the current behavior but pass with the desired behavior (i.e. the input data is not modified when `cytoscape_graph` is called).
 2. Modify the `cytoscape_graph` function defined in `networkx/readwrite/json_graph/cytoscape.py` so that the input data is not modified by the function

Commit message: fix issue #4173: readwrite.json_graph.cytoscape_graph(input_data) did modify the original data

Patch to review:
diff --git a/networkx/readwrite/json_graph/cytoscape.py b/networkx/readwrite/json_graph/cytoscape.py
index 1a6f0c06c7e..8310a90bf0a 100644
--- a/networkx/readwrite/json_graph/cytoscape.py
+++ b/networkx/readwrite/json_graph/cytoscape.py
@@ -98,8 +98,8 @@ def cytoscape_graph(data, attrs=None):
 
     for d in data["elements"]["edges"]:
         edge_data = d["data"].copy()
-        sour = d["data"].pop("source")
-        targ = d["data"].pop("target")
+        sour = d["data"].get("source")
+        targ = d["data"].get("target")
         if multigraph:
             key = d["data"].get("key", 0)
             graph.add_edge(sour, targ, key=key)
diff --git a/networkx/readwrite/json_graph/tests/test_cytoscape.py b/networkx/readwrite/json_graph/tests/test_cytoscape.py
index ee4799fb697..881effb9600 100644
--- a/networkx/readwrite/json_graph/tests/test_cytoscape.py
+++ b/networkx/readwrite/json_graph/tests/test_cytoscape.py
@@ -10,6 +10,16 @@ def test_graph(self):
         H = cytoscape_graph(cytoscape_data(G))
         nx.is_isomorphic(G, H)
 
+    def test_input_data_is_not_modified_when_building_graph(self):
+        G = nx.path_graph(4)
+        input_data = cytoscape_data(G)
+        first_edge = input_data['elements']['edges'][0]['data']
+        assert first_edge['source'] == 0 and first_edge['target'] == 1
+
+        H = cytoscape_graph(input_data)
+        first_edge = input_data['elements']['edges'][0]['data']
+        assert first_edge['source'] == 0 and first_edge['target'] == 1
+
     def test_graph_attributes(self):
         G = nx.path_graph(4)
         G.add_node(1, color="red")


Code review comments:
diff_hunk: @@ -10,6 +10,16 @@ def test_graph(self):
         H = cytoscape_graph(cytoscape_data(G))
         nx.is_isomorphic(G, H)
 
+    def test_input_data_is_not_modified_when_building_graph(self):
+        G = nx.path_graph(4)
+        input_data = cytoscape_data(G)
+        first_edge = input_data['elements']['edges'][0]['data']
+        assert first_edge['source'] == 0 and first_edge['target'] == 1
+
+        H = cytoscape_graph(input_data)
+        first_edge = input_data['elements']['edges'][0]['data']
path: networkx/readwrite/json_graph/tests/test_cytoscape.py
text: @user1:
Can you check this? You create H but then don't actually test anything about H.
And you've already tested input_data above...

@author:
Hi, I can remove the assignment because we're not interested in the result.

Regarding the input_data, the two tests are there to show how in the first one the data contains those values, but after calling the "cytoscape_graph" function, it doesn't contain them anymore, meaning the function modified that data. Do you want me to remove the first assert?

@user1:
Got it!  Sorry -- I'm getting too little sleep...   :}  
Yes, this makes sense. The test fails with current code and is fixed with the new code.
Thanks!

I suppose you could take out the assignment and maybe add a comment before the second test. Something like:
```# make sure data didn't change```.  Not necessary -- it's in the function name, but might be easier to see(?) I'll leave that up to you.


@user2:
To chime in here: I think the test could be improved by testing the input in its entirety, rather than only testing a subset of the keys. So I'm thinking something along the lines of:

```python
G = nx.path_graph(4)
input_data = cytoscape_data(G)
orig_data = copy.deepcopy(input_data)
# Ensure input is unmodified by cytoscape_graph (gh-4173)
H = cytoscape_graph(input_data)
assert input_data == orig_data
```