Skip to content

[SYNPY-1918] Fix JSON Schema converison bugs - #1451

Open
andrewelamb wants to merge 2 commits into
developfrom
SYNPY-1918
Open

[SYNPY-1918] Fix JSON Schema converison bugs#1451
andrewelamb wants to merge 2 commits into
developfrom
SYNPY-1918

Conversation

@andrewelamb

@andrewelamb andrewelamb commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Problem:

create_json_schema in synapseclient/extensions/curator/schema_generation.py builds the allOf block that holds the DependsOn logic of a CSV data model. Two defects made the generated conditionals wrong. Both defects were silent: the schema was produced and registered with no warning.

Defect 1: a list typed trigger attribute made an if clause that can never match.

JSONSchema._convert_conditional_properties_to_all_of built every if clause in the same way, and did not look at the type of the watched attribute:

"if": {
    "properties": {"Comorbidities": {"enum": ["Psoriasis"]}},
    "required": ["Comorbidities"]
}

An attribute with a list columnType (string_list, integer_list, boolean_list), is written as an array property by _create_enum_array_property:

"Comorbidities": {
    "type": "array",
    "items": {"enum": ["Diabetes", "Psoriasis"], "type": "string"}
}

Applied to the property itself, enum asks if the complete value is equal to the string "Psoriasis". The value is an array, and an array is never equal to a string under the enum comparison rules. Thus the if clause could not match, the then clause never applied, and the dependent attributes were never required. A curator could omit required metadata and get no validation error.

Root cause: add_conditional_dependency receives only the name of the watched attribute, so the converter could not tell if that attribute was written as a scalar enum or as an array of enums.

Defect 2: required could hold the same property two times.

A generated branch could contain "required": ["PASI", "PASI"]. The draft-07 meta-schema declares required with uniqueItems set to true, so a duplicate makes the generated schema invalid against the meta-schema. Strict validators refuse to load it.

Root cause: GraphTraversalState.move_to_next_node records the reverse dependencies of each node that it pops. The processed node guard (is_current_node_processed) is one level up, in the create_json_schema loop, so it prevents re-processing but not re-recording. A node that two paths can reach — for example a valid value such as Psoriasis that two attributes share — is queued two times and its dependencies are recorded two times. add_conditional_dependency then appended the dependent property two times, because it had no membership check.

Solution:

  • _convert_conditional_properties_to_all_of now takes the generated properties dict as an argument. JSONSchema.to_dict calls the converter after all properties are set, so the property schema of the watched attribute is available at that time.

  • When the watched property has "type": "array", the if clause uses contains with const in place of enum:

    "if": {
        "properties": {
            "Comorbidities": {"type": "array", "contains": {"const": "Psoriasis"}}
        },
        "required": ["Comorbidities"]
    }

    The clause also asserts "type": "array". The contains keyword applies only to arrays; a validator ignores it for a value of any other kind, and an ignored keyword counts as satisfied. Without the type keyword, an instance that holds the plain string "Diabetes", or null, would match the if clause and tell the user that PASI is missing, although the trigger value was never supplied. The scalar path keeps the enum form, so there is no change to schemas that have no list typed trigger.

  • add_conditional_dependency now uses setdefault and adds the dependent property only if it is not already in the list. This is a local guard, and it covers all callers. The deeper fix — de-duplication of the traversal queue and of the two maps in GraphTraversalState — is not part of this PR.

Testing:

New unit tests in tests/unit/synapseclient/extensions/unit_test_create_json_schema.py, with two new data model fixtures:

  • list_conditional_model.csv — a string_list attribute drives a conditional. The end-to-end test asserts that a non-array value ("Diabetes") and a null value report a type error, but do not report the dependent property as missing.
  • shared_valid_value_model.csv — the valid value Psoriasis is shared by the Comorbidities attribute and the Diagnosis attribute, and Psoriasis dependsOn PASI. The test asserts that there are two conditional branches, that each required list is exactly ["PASI"], and that Draft7Validator.check_schema accepts the generated schema.
  • test_convert_conditional_properties_to_all_of_array — a unit test of the converter with an array typed watched property.
  • The two existing converter tests were updated for the new properties argument, and act as the scalar regression check.

All 116 tests in unit_test_create_json_schema.py pass locally.

@andrewelamb
andrewelamb requested a review from a team as a code owner August 28, 2026 15:44
@andrewelamb
andrewelamb marked this pull request as draft August 28, 2026 15:49
@andrewelamb
andrewelamb marked this pull request as ready for review August 28, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant