[SYNPY-1918] Fix JSON Schema converison bugs

synapseclient/extensions/curator/schema_generation.py

@@ -5109,7 +5109,7 @@ def as_json_schema_dict(
         #  valid JSON Schema keyword
         if self._conditional_dependencies:
             json_schema_dict["allOf"] = self._convert_conditional_properties_to_all_of(
-                self._conditional_dependencies
+                self._conditional_dependencies, self.properties
             )
         json_schema_dict.pop("_conditional_dependencies")
         return json_schema_dict
@@ -5158,18 +5158,19 @@ def add_conditional_dependency(
             enum_value: The value of the watched property that triggers the condition
             dependent_property: The property that becomes required when the condition is triggered
         """
-        if (watched_property, enum_value) not in self._conditional_dependencies:
-            self._conditional_dependencies[(watched_property, enum_value)] = [
-                dependent_property
-            ]
-        else:
-            self._conditional_dependencies[(watched_property, enum_value)].append(
-                dependent_property
-            )
+        dependent_properties = self._conditional_dependencies.setdefault(
+            (watched_property, enum_value), []
+        )
+        # A node can be reached by more than one path in the data model graph, so the
+        #  same dependent property can be added more than once. The draft-07
+        #  meta-schema requires the items of "required" to be unique.
+        if dependent_property not in dependent_properties:
+            dependent_properties.append(dependent_property)
 
     @staticmethod
     def _convert_conditional_properties_to_all_of(
         conditional_dependencies: dict[tuple[str, str], list[str]],
+        properties: dict[str, Property],
     ) -> list[AllOf]:
         """
         Converts the conditional dependencies dict to a list of JSON Schema allOf conditions
@@ -5178,6 +5179,8 @@ def _convert_conditional_properties_to_all_of(
             conditional_dependencies: A mapping of conditional dependencies to be added to the "allOf" keyword in JSON Schema.
                 The key is a tuple of (watched_property, enum_value)
                 The value is a list of properties that become required when watched_property has the value enum_value.
+            properties: The properties of the JSON Schema. These are used to determine
+                how the watched property holds the value that triggers the condition.
 
         Returns:
             A list of JSON Schema allOf conditions
@@ -5220,9 +5223,17 @@ def _convert_conditional_properties_to_all_of(
             watched_property,
             enum_value,
         ), dependent_properties in conditional_dependencies.items():
+            watched_property_schema = properties.get(watched_property, {})
+            if watched_property_schema.get("type") == "array":
+                trigger_condition: Property = {
+                    "type": "array",
+                    "contains": {"const": enum_value},
+                }
+            else:
+                trigger_condition = {"enum": [enum_value]}
             conditional_dep = {
                 "if": {
-                    "properties": {watched_property: {"enum": [enum_value]}},
+                    "properties": {watched_property: trigger_condition},
                     "required": [watched_property],
                 },
                 "then": {

tests/unit/synapseclient/extensions/schema_files/data_models/list_conditional_model.csv

@@ -0,0 +1,6 @@
+Attribute,IsTemplate,Description,Valid Values,DependsOn,Properties,Required,Parent,DependsOn Component,Source,Validation Rules,columnType,Format,Maximum,Minimum,Pattern
+Component,,,,,,TRUE,,,,,,,,,
+ListConditional,TRUE,Component with a list typed conditional trigger attribute,,"Component, Comorbidities",,FALSE,DataType,,,,,,,,
+Comorbidities,,,"Diabetes, Psoriasis",,,TRUE,DataProperty,,,,string_list,,,,
+Psoriasis,,,,PASI,,FALSE,ValidValue,,,,,,,,
+PASI,,,,,,TRUE,DataProperty,,,,integer,,,,

tests/unit/synapseclient/extensions/schema_files/data_models/shared_valid_value_model.csv

@@ -0,0 +1,7 @@
+Attribute,IsTemplate,Description,Valid Values,DependsOn,Properties,Required,Parent,DependsOn Component,Source,Validation Rules,columnType,Format,Maximum,Minimum,Pattern
+Component,,,,,,TRUE,,,,,,,,,
+SharedValidValue,TRUE,Component where one valid value is shared by two attributes,,"Component, Comorbidities, Diagnosis",,FALSE,DataType,,,,,,,,
+Comorbidities,,,"Diabetes, Psoriasis",,,TRUE,DataProperty,,,,string,,,,
+Diagnosis,,,"Healthy, Psoriasis",,,TRUE,DataProperty,,,,string,,,,
+Psoriasis,,,,PASI,,FALSE,ValidValue,,,,,,,,
+PASI,,,,,,FALSE,DataProperty,,,,integer,,,,

tests/unit/synapseclient/extensions/unit_test_create_json_schema.py

@@ -320,9 +320,12 @@ def test_convert_conditional_properties_to_all_of(self) -> None:
             ("Diagnosis", "Healthy"): ["CheckupDate"],
         }
 
+        # AND the watched property is not an array
+        properties = {"Diagnosis": {"enum": ["Cancer", "Healthy"], "title": "enum"}}
+
         # WHEN converting to allOf
         all_of = schema._convert_conditional_properties_to_all_of(
-            conditional_dependencies
+            conditional_dependencies, properties
         )
 
         # THEN the result should match the expected structure
@@ -365,12 +368,58 @@ def test_convert_no_conditional_properties_to_all_of(self) -> None:
 
         # WHEN converting to allOf
         all_of = schema._convert_conditional_properties_to_all_of(
-            conditional_dependencies
+            conditional_dependencies, {}
         )
 
         # THEN the result also be empty
         assert len(all_of) == 0
 
+    def test_convert_conditional_properties_to_all_of_array(self) -> None:
+        """
+        Test JSONSchema._convert_conditional_properties_to_all_of with an array typed
+        watched property.
+        """
+        # GIVEN a JSONSchema instance
+        schema = JSONSchema()
+
+        # AND a conditional dependency
+        conditional_dependencies = {("Comorbidities", "Psoriasis"): ["PASI"]}
+
+        # AND the watched property is an array
+        properties = {
+            "Comorbidities": {
+                "type": "array",
+                "title": "array",
+                "items": {"enum": ["Diabetes", "Psoriasis"], "type": "string"},
+            }
+        }
+
+        # WHEN converting to allOf
+        all_of = schema._convert_conditional_properties_to_all_of(
+            conditional_dependencies, properties
+        )
+
+        # THEN the if clause matches the array with contains, not enum
+        # AND the if clause also asserts the array type, so that it does not match a
+        #  value of another kind, for which a validator skips the contains keyword
+        assert all_of == [
+            {
+                "if": {
+                    "properties": {
+                        "Comorbidities": {
+                            "type": "array",
+                            "contains": {"const": "Psoriasis"},
+                        }
+                    },
+                    "required": ["Comorbidities"],
+                },
+                "then": {
+                    "properties": {"PASI": {"not": {"type": "null"}}},
+                    "required": ["PASI"],
+                },
+            }
+        ]
+
 
 @pytest.mark.parametrize(
     "node_name, expected_type, expected_is_array, expected_min, expected_max, expected_pattern, expected_format",
@@ -1168,6 +1217,109 @@ def test_validate_invalid_instances(
         validator.validate(instance)
 
 
+def test_create_json_schema_list_typed_trigger_ignores_non_array_value(
+    helpers,
+) -> None:
+    """
+    Tests that a list typed conditional trigger does not fire when the instance holds a
+    value that is not an array.
+
+    In the list_conditional_model data model the Comorbidities attribute has a
+    columnType of string_list, so the if clause of its conditional branch matches the
+    array with the contains keyword. The contains keyword only applies to arrays. A
+    validator skips it for every other kind of value, and a skipped keyword counts as
+    satisfied, so contains alone would let the if clause match a string and a null as
+    well.
+
+    The if clause therefore asserts the array type in addition to contains. Without the
+    type keyword an instance that holds the plain string Diabetes would tell the user
+    that PASI is missing, even though the trigger value Psoriasis was never supplied.
+    """
+    # GIVEN a data model where a list typed attribute drives a conditional
+    dmge = helpers.get_data_model_graph_explorer(
+        path="data_models/list_conditional_model.csv"
+    )
+
+    # AND a JSON Schema created for the component
+    schema_dict = create_json_schema(
+        dmge=dmge,
+        datatype="ListConditional",
+        schema_name="ListConditional_validation",
+        write_schema=False,
+        use_display_labels=False,
+        logger=logging.getLogger(__name__),
+    )
+    validator = Draft7Validator(schema_dict)
+
+    # WHEN an instance holds a value for the trigger attribute that is not an array
+    # AND that value is not the value that triggers the condition
+    instance = {"Component": "ListConditional", "Comorbidities": "Diabetes"}
+    errors = list(validator.iter_errors(instance))
+
+    # THEN the type of the trigger attribute is reported
+    assert any(error.validator == "type" for error in errors), errors
+    # AND the conditional dependency is not reported as missing
+    required_errors = [error for error in errors if error.validator == "required"]
+    assert not required_errors, (
+        "The conditional branch fired for a value that is not an array: "
+        f"{[error.message for error in required_errors]}"
+    )
+
+    # AND the same holds when the trigger attribute is null
+    null_errors = list(
+        validator.iter_errors({"Component": "ListConditional", "Comorbidities": None})
+    )
+    assert not [error for error in null_errors if error.validator == "required"], (
+        "The conditional branch fired for a null value: "
+        f"{[error.message for error in null_errors]}"
+    )
+
+
+def test_create_json_schema_with_valid_value_shared_by_two_attributes(
+    helpers,
+) -> None:
+    """
+    Tests create_json_schema with a data model where one node is reachable by more than
+    one path.
+
+    In the shared_valid_value_model data model both the Comorbidities attribute and the
+    Diagnosis attribute have the valid value Psoriasis, and Psoriasis is also an
+    attribute that dependsOn the PASI attribute. The graph traversal therefore queues
+    Psoriasis twice, once from each attribute.
+
+    The processed node guard in create_json_schema stops Psoriasis from being processed
+    twice, but move_to_next_node records the reverse dependencies of every node it pops.
+    PASI is recorded twice as a reverse dependency of Psoriasis, so
+    add_conditional_dependency appends PASI twice and each branch is written with
+    "required": ["PASI", "PASI"]. The draft-07 meta-schema declares required with
+    uniqueItems set to true, so the generated schema is invalid against the
+    meta-schema and strict validators refuse to load it.
+    """
+    # GIVEN a data model where one valid value is shared by two attributes
+    dmge = helpers.get_data_model_graph_explorer(
+        path="data_models/shared_valid_value_model.csv"
+    )
+
+    # WHEN a JSON Schema is created for the component
+    schema_dict = create_json_schema(
+        dmge=dmge,
+        datatype="SharedValidValue",
+        schema_name="SharedValidValue_validation",
+        write_schema=False,
+        use_display_labels=False,
+        logger=logging.getLogger(__name__),
+    )
+
+    # THEN each of the two attributes drives one conditional branch
+    assert len(schema_dict["allOf"]) == 2
+    # AND no branch repeats a dependent property in its required list
+    for branch in schema_dict["allOf"]:
+        required = branch["then"]["required"]
+        assert required == ["PASI"], f"Duplicate entries in required: {required}"
+    # AND the generated schema is valid against the draft-07 meta-schema
+    Draft7Validator.check_schema(schema_dict)
+
+
 def test_write_data_model_with_schema_path(test_directory: str) -> None:
     """Test for _write_data_model with the path provided."""
     schema_path = os.path.join(test_directory, "test_write_data_model1.json")