[SYNPY-1918] Fix JSON Schema converison bugs - #1451
Open
andrewelamb wants to merge 2 commits into
Open
Conversation
andrewelamb
marked this pull request as draft
August 28, 2026 15:49
andrewelamb
marked this pull request as ready for review
August 28, 2026 16:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
create_json_schemainsynapseclient/extensions/curator/schema_generation.pybuilds theallOfblock that holds theDependsOnlogic 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
ifclause that can never match.JSONSchema._convert_conditional_properties_to_all_ofbuilt everyifclause in the same way, and did not look at the type of the watched attribute:An attribute with a list
columnType(string_list,integer_list,boolean_list), is written as an array property by_create_enum_array_property:Applied to the property itself,
enumasks 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 theenumcomparison rules. Thus theifclause could not match, thethenclause never applied, and the dependent attributes were never required. A curator could omit required metadata and get no validation error.Root cause:
add_conditional_dependencyreceives 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:
requiredcould hold the same property two times.A generated branch could contain
"required": ["PASI", "PASI"]. The draft-07 meta-schema declaresrequiredwithuniqueItemsset totrue, so a duplicate makes the generated schema invalid against the meta-schema. Strict validators refuse to load it.Root cause:
GraphTraversalState.move_to_next_noderecords the reverse dependencies of each node that it pops. The processed node guard (is_current_node_processed) is one level up, in thecreate_json_schemaloop, so it prevents re-processing but not re-recording. A node that two paths can reach — for example a valid value such asPsoriasisthat two attributes share — is queued two times and its dependencies are recorded two times.add_conditional_dependencythen appended the dependent property two times, because it had no membership check.Solution:
_convert_conditional_properties_to_all_ofnow takes the generatedpropertiesdict as an argument.JSONSchema.to_dictcalls 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", theifclause usescontainswithconstin place ofenum:The clause also asserts
"type": "array". Thecontainskeyword applies only to arrays; a validator ignores it for a value of any other kind, and an ignored keyword counts as satisfied. Without thetypekeyword, an instance that holds the plain string"Diabetes", ornull, would match theifclause and tell the user thatPASIis missing, although the trigger value was never supplied. The scalar path keeps theenumform, so there is no change to schemas that have no list typed trigger.add_conditional_dependencynow usessetdefaultand 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 inGraphTraversalState— 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— astring_listattribute drives a conditional. The end-to-end test asserts that a non-array value ("Diabetes") and anullvalue report atypeerror, but do not report the dependent property as missing.shared_valid_value_model.csv— the valid valuePsoriasisis shared by theComorbiditiesattribute and theDiagnosisattribute, andPsoriasisdependsOnPASI. The test asserts that there are two conditional branches, that eachrequiredlist is exactly["PASI"], and thatDraft7Validator.check_schemaaccepts the generated schema.test_convert_conditional_properties_to_all_of_array— a unit test of the converter with an array typed watched property.propertiesargument, and act as the scalar regression check.All 116 tests in
unit_test_create_json_schema.pypass locally.