From 7afea99c6fa8923a61932f8284ae386b789a7139 Mon Sep 17 00:00:00 2001 From: Lingling Peng Date: Tue, 10 Feb 2026 15:21:27 -0500 Subject: [PATCH 1/4] fix bug; add test --- .../extensions/curator/schema_generation.py | 10 +++++-- .../unit_test_create_json_schema.py | 28 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/synapseclient/extensions/curator/schema_generation.py b/synapseclient/extensions/curator/schema_generation.py index 89574cdce..945d8eaab 100644 --- a/synapseclient/extensions/curator/schema_generation.py +++ b/synapseclient/extensions/curator/schema_generation.py @@ -4895,6 +4895,7 @@ def move_to_next_node(self) -> None: source_node=self.source_node, logger=self.logger, ) + # self.current_node.valid_values is class label self._update_valid_values_map( self.current_node.name, self.current_node.valid_values ) @@ -4994,6 +4995,7 @@ def get_conditional_properties( if self.current_node is None: raise ValueError("Current node is None") conditional_properties: list[tuple[str, str]] = [] + for value in self._reverse_dependencies[self.current_node.name]: if value in self._valid_values_map: properties = sorted(self._valid_values_map[value]) @@ -5002,8 +5004,12 @@ def get_conditional_properties( watched_property = self.dmge.get_nodes_display_names( [watched_property] )[0] - value = self.dmge.get_nodes_display_names([value])[0] - conditional_properties.append((watched_property, value)) + display_name_value = self.dmge.get_nodes_display_names([value])[ + 0 + ] + conditional_properties.append( + (watched_property, display_name_value) + ) return conditional_properties def _update_valid_values_map( diff --git a/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py b/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py index 048d5e791..1da1fad89 100644 --- a/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py +++ b/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py @@ -845,6 +845,34 @@ def test_get_conditional_properties(self, dmge: DataModelGraphExplorer) -> None: # THEN the current node should have conditional properties assert gts.get_conditional_properties() == [("Diagnosis", "Cancer")] + def test_get_conditional_properties_multiple_watched_properties( + self, dmge: DataModelGraphExplorer + ) -> None: + """Test GraphTraversalState.get_conditional_properties with multiple watched properties. + + This test covers a bug where the 'value' variable was being mutated inside + the inner loop when converting to display names, causing a KeyError on + subsequent iterations when there are multiple watched properties for the + same valid value. + """ + # GIVEN a GraphTraversalState instance where + # - CancerType has a reverse dependency of Cancer + # - Cancer is a valid value of MULTIPLE attributes (Diagnosis AND FamilyHistory) + gts = GraphTraversalState(dmge, "Patient", logger=Mock()) + gts._nodes_to_process = ["CancerType"] + gts._reverse_dependencies = {"CancerType": ["Cancer"]} + # Cancer is a valid value for both Diagnosis and FamilyHistory + gts._valid_values_map = {"Cancer": ["Diagnosis", "FamilyHistory"]} + # WHEN using move_to_next_node + gts.move_to_next_node() + # THEN the current node should have conditional properties for BOTH + # watched properties, and the call should not raise a KeyError + result = gts.get_conditional_properties() + # Should return tuples for both Diagnosis and FamilyHistory + assert len(result) == 2 + assert ("Diagnosis", "Cancer") in result + assert ("Family History", "Cancer") in result + def test_update_valid_values_map(self, dmge: DataModelGraphExplorer) -> None: """Test GraphTraversalState._update_valid_values_map""" # GIVEN a GraphTraversalState instance From 416475454031c3fd500e3aae5a45c3447623085f Mon Sep 17 00:00:00 2001 From: Lingling Peng Date: Tue, 10 Feb 2026 15:38:39 -0500 Subject: [PATCH 2/4] update test --- .../unit_test_create_json_schema.py | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py b/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py index 1da1fad89..115217998 100644 --- a/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py +++ b/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py @@ -851,27 +851,28 @@ def test_get_conditional_properties_multiple_watched_properties( """Test GraphTraversalState.get_conditional_properties with multiple watched properties. This test covers a bug where the 'value' variable was being mutated inside - the inner loop when converting to display names, causing a KeyError on - subsequent iterations when there are multiple watched properties for the - same valid value. + the inner loop when converting to display names. + + On the first iteration, `value` is converted from class label to display name. + On subsequent iterations, the code tries to look up the display name in the graph, + which fails with KeyError because graph nodes are keyed by class labels. """ # GIVEN a GraphTraversalState instance where - # - CancerType has a reverse dependency of Cancer - # - Cancer is a valid value of MULTIPLE attributes (Diagnosis AND FamilyHistory) + # - CancerType has a reverse dependency of FamilyHistory + # - FamilyHistory is a valid value of MULTIPLE attributes gts = GraphTraversalState(dmge, "Patient", logger=Mock()) gts._nodes_to_process = ["CancerType"] - gts._reverse_dependencies = {"CancerType": ["Cancer"]} - # Cancer is a valid value for both Diagnosis and FamilyHistory - gts._valid_values_map = {"Cancer": ["Diagnosis", "FamilyHistory"]} + + # Use FamilyHistory because its display name ("Family History") differs from class label + gts._reverse_dependencies = {"CancerType": ["FamilyHistory"]} + # FamilyHistory triggers multiple watched properties - this is key to triggering the bug + gts._valid_values_map = {"FamilyHistory": ["Sex", "YearofBirth"]} # WHEN using move_to_next_node gts.move_to_next_node() - # THEN the current node should have conditional properties for BOTH - # watched properties, and the call should not raise a KeyError result = gts.get_conditional_properties() - # Should return tuples for both Diagnosis and FamilyHistory assert len(result) == 2 - assert ("Diagnosis", "Cancer") in result - assert ("Family History", "Cancer") in result + assert ("Diagnosis", "Family History") in result + assert ("Sex", "Family History") in result def test_update_valid_values_map(self, dmge: DataModelGraphExplorer) -> None: """Test GraphTraversalState._update_valid_values_map""" From 0810823f4989f18abd357ce4e8d1b6fedbc10547 Mon Sep 17 00:00:00 2001 From: Lingling Peng Date: Tue, 10 Feb 2026 15:52:02 -0500 Subject: [PATCH 3/4] fix test --- .../synapseclient/extensions/unit_test_create_json_schema.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py b/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py index 115217998..c5bcaf8cf 100644 --- a/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py +++ b/tests/unit/synapseclient/extensions/unit_test_create_json_schema.py @@ -853,9 +853,6 @@ def test_get_conditional_properties_multiple_watched_properties( This test covers a bug where the 'value' variable was being mutated inside the inner loop when converting to display names. - On the first iteration, `value` is converted from class label to display name. - On subsequent iterations, the code tries to look up the display name in the graph, - which fails with KeyError because graph nodes are keyed by class labels. """ # GIVEN a GraphTraversalState instance where # - CancerType has a reverse dependency of FamilyHistory @@ -871,7 +868,7 @@ def test_get_conditional_properties_multiple_watched_properties( gts.move_to_next_node() result = gts.get_conditional_properties() assert len(result) == 2 - assert ("Diagnosis", "Family History") in result + assert ("Year of Birth", "Family History") in result assert ("Sex", "Family History") in result def test_update_valid_values_map(self, dmge: DataModelGraphExplorer) -> None: From cdf116f39eec977394946e9785d2eaf474b29701 Mon Sep 17 00:00:00 2001 From: Lingling Peng Date: Tue, 10 Feb 2026 16:19:23 -0500 Subject: [PATCH 4/4] fix bug --- synapseclient/extensions/curator/schema_generation.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/synapseclient/extensions/curator/schema_generation.py b/synapseclient/extensions/curator/schema_generation.py index 945d8eaab..e3d238d37 100644 --- a/synapseclient/extensions/curator/schema_generation.py +++ b/synapseclient/extensions/curator/schema_generation.py @@ -4895,7 +4895,6 @@ def move_to_next_node(self) -> None: source_node=self.source_node, logger=self.logger, ) - # self.current_node.valid_values is class label self._update_valid_values_map( self.current_node.name, self.current_node.valid_values ) @@ -5007,9 +5006,11 @@ def get_conditional_properties( display_name_value = self.dmge.get_nodes_display_names([value])[ 0 ] - conditional_properties.append( - (watched_property, display_name_value) - ) + conditional_properties.append( + (watched_property, display_name_value) + ) + else: + conditional_properties.append((watched_property, value)) return conditional_properties def _update_valid_values_map(