Skip to content

Conversation

@andrewelamb
Copy link
Contributor

@andrewelamb andrewelamb commented Jun 17, 2025

Problem:

When creating JSON Schemas, the date and url validation rules were being ignored.

Solution:

When the date and url validation rules are present, the JSON Schema property gets the format keyword. The value fo this keyword is either "format": "date" or "format": "uri"

Testing:

Date and URL attributes were added to the test data model JSONSchemaComponent datatype. The expected generated JSON Schemas have these properties and the expected format keyword.

@andrewelamb andrewelamb marked this pull request as draft June 17, 2025 19:54
@andrewelamb andrewelamb requested a review from a team as a code owner June 17, 2025 19:54
@andrewelamb andrewelamb marked this pull request as ready for review June 18, 2025 15:53
@andrewelamb andrewelamb changed the title Schematic 295 [SCHEMATIC-295] Add ability for JSON Schemas to have format:date and format:uri based on date and url validaiton rules Jun 18, 2025
Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @andrewelamb ! Thanks for your hard work. My main concerns are:

  1. the look up of validation rule name using a list seems to be inefficient. See _get_rule_by_name and _VALIDATION_RULES
  2. I found some of the variable names a bit unclear on first read. For example, js_is_array, js_type, and js_format. It was a bit confusing that js_is_array is separate from js_type, since "array" is technically a JSON Schema type. Also, I'm not entirely sure how js_format is used or when it's expected to be set. Would it be possible to clarify the distinction between these variables, either in comments or the docstring? Or is it possible to consolidate some of them so that we have less variables to maintain?
  3. in _get_validation_rule_based_fields, we overwrite js_type for a couple of times. I am curious if there's a way to make get_js_type_from_inputted_rules more "authoritative" so that the logic becomes easier to follow.

@SageGJ
Copy link
Collaborator

SageGJ commented Jun 25, 2025

🔥 Great work @andrewelamb and reviews @Sage-Bionetworks/schematic-developers ! I'll defer to @SageGJ as final reviewer after all comments addressed.

@andrewelamb just let me know when you're ready for me to take a last look

@andrewelamb andrewelamb requested review from SageGJ and linglp June 26, 2025 15:14
Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @andrewelamb ! It looks good overall, just some nit comments.

@andrewelamb andrewelamb requested a review from linglp July 1, 2025 15:47
Copy link
Collaborator

@SageGJ SageGJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additons looks good, just a left a few comments before finalizing. Thanks again!

Copy link
Collaborator

@SageGJ SageGJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additions look good; just left a few comments before finalizing. Thanks again!

@andrewelamb andrewelamb requested a review from SageGJ July 1, 2025 16:48
Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @andrewelamb ! Thanks for your hard work. I took another look and spotted some small typos. There's also a mix of "Args"/"Arguments" in the docstring. I think that we don't have a standard in Schematic, but maybe it is a good idea to start keeping them consistent.

One issue that I noticed is that if a rule is not supported in _VALIDATION_RULES, we are doing _VALIDATION_RULES.get and filter out None in the return which means an unsupported validation rule would fail silently. I am a bit concerned about this since users might believe all rules were found and used, but some may be silently skipped which can lead to confusion. If we decide not to raise an error, I think a warning can be helpful too.

@BryanFauble BryanFauble requested a review from Copilot July 2, 2025 19:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for format: date and format: uri in generated JSON Schemas based on new date and url validation rules.

  • Introduce JSONSchemaFormat enum and extend validation rule registry for date and url
  • Update schema creation logic to infer format alongside type, minimum, maximum, and pattern
  • Expand tests and expected schema fixtures to cover date, URI, and renamed InRange cases

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
schematic/schemas/constants.py Add JSONSchemaFormat and rename validation rule enum to ValidationRuleName
schematic/schemas/json_schema_validation_rule_functions.py Register date/url rules and update rule helpers
schematic/schemas/create_json_schema.py Extract and apply format from validation rules
tests/unit/test_json_schema_validation_rule_functions.py Cover new date and url rules in unit tests
tests/unit/test_create_json_schema.py Assert format field in Node and _get_validation_rule_based_fields tests
tests/data/expected_jsonschemas/expected.MockComponent.schema.json Add "format": "date" / "format": "uri" entries
tests/data/expected_jsonschemas/expected.JSONSchemaComponent.schema.json Inject new properties for Date, URL, and InRange
tests/data/example.model.jsonld Include classes for String, Date, URL, InRange, Regex, List
tests/data/example.model.csv Update component rows to list date, url, and inRange rules
tests/data/example.model.column_type_component.jsonld Similar additions for column-type example
tests/data/example.model.column_type_component.invalid.jsonld Add invalid-case entries for date, URL, and inRange
tests/data/example.model.column_type_component.invalid.csv Update CSV for invalid column-type component
tests/data/example.model.column_type_component.csv Update CSV for column-type component
Comments suppressed due to low confidence (2)

schematic/schemas/constants.py:34

  • [nitpick] The enum member is named URI but the validation rule is called url; consider renaming this member to URL (or adding a URL alias) for consistency with ValidationRuleName.URL.
    URI = "uri"

schematic/schemas/create_json_schema.py:225

  • The docstring for _get_validation_rule_based_fields does not describe the actual return tuple order (is_array, type, format, minimum, maximum, pattern); update it to reflect the current signature.
    Gets the fields for the Node class that are based on the validation rules

andrewelamb and others added 4 commits July 2, 2025 14:04
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@andrewelamb andrewelamb requested a review from linglp July 2, 2025 21:07
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 LGTM! Helping in with the final final review (on top of Gianna's final review) since lingling has other program initiatives.

Appreciate the reviews and addition of warnings for validation rules not supported (maybe that's a good example of a Jira ticket but it was a good addition)

Note: I was testing the "dismiss review feature, and I am not a fan..." I appreciate everyone's reviews!

@thomasyu888 thomasyu888 dismissed linglp’s stale review July 3, 2025 04:33

I provided a final review

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 3, 2025

Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you @andrewelamb

@andrewelamb andrewelamb merged commit 19ba49f into develop Jul 3, 2025
10 of 12 checks passed
@andrewelamb andrewelamb deleted the SCHEMATIC-295 branch July 3, 2025 15:54
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.

5 participants