Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotfix/json schema #113

Closed
wants to merge 6 commits into from
Closed

Hotfix/json schema #113

wants to merge 6 commits into from

Conversation

wandmagic
Copy link
Contributor

Committer Notes

The Json schema generation has an issue with creating the names for deginitions.

This pull request adds rudimentary testing to compile a json schema as a function which is only possible for valid schemas.

currently there is a warning regarding the decimal type, and that is addressed by a pull request up-stream.

usnistgov/metaschema#572

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you squashed any non-relevant commits and commit messages? [instructions]
  • Do all automated CI/CD checks pass?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you included examples of how to use your new feature(s)?
  • Have you updated all website and readme documentation affected by the changes you made? Changes to the website can be made in the website/content directory of your branch.

@wandmagic wandmagic requested a review from a team as a code owner March 18, 2024 20:13
Copy link
Contributor

@JustKuzya JustKuzya left a comment

Choose a reason for hiding this comment

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

I like the idea of AJV validation.

@wendellpiez
Copy link
Collaborator

As I remarked to @wandmagic in another channel, I do not like adding a Node JS dependency here. This test needs to be conducted, but adding dependencies only creates more technical debt.

(But I think it could be a good thing in a calling repository. Maybe the OSCAL repository already has a call on ajv -compile or even a test.)

Any comments on #110?

@wandmagic
Copy link
Contributor Author

Hey i've made the node JS dependency optional in the latest commit.
it can be called outside of the smoke tests and spec tests independently.

Copy link
Collaborator

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

I wonder if an illustration of the intended result JSON Schema could be provided, along with a pointer to the metaschema source file from which it is derived.

This has to be unit tested; it is possible the path could be collapsed further (removing support for @prefer) following Metaschema adjustment in usnistgov/metaschema#558.

Given copies of the source metaschema, the intended result and the current result (from a run), I can build a unit test that isolates this path logic and presents examples of the range of expected inputs.

This way when we patch the path logic we also expose assurance that it works (under many eyes), and we will not thrash.

Thanks for diligence so far @wandmagic

@wandmagic
Copy link
Contributor Author

see the output of current develop branch
invalid_develop.schema.json

and the output from this branch
corrected.schema.json

@wendellpiez
Copy link
Collaborator

Thanks much, @wandmagic

For transparency and the record, could I bother you also to provide code illustrations of the before-and-after?

This is mainly because I expect to provide this work with the unit testing to support it (validate and protect against regression), and it would save me a couple steps in writing it. But additionally because it makes the work easier to track for everyone.

@wandmagic
Copy link
Contributor Author

the only substantial change is in the difference in output illustrated here:
this code change:
Screenshot 2024-03-23 at 7 21 21 PM

results in this difference in output.
Screenshot 2024-03-23 at 7 18 29 PM

@xvii-corp xvii-corp closed this by deleting the head repository Mar 24, 2024
@wendellpiez
Copy link
Collaborator

@wandmagic thanks for the snips, very helpful indeed for repro.

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.

4 participants