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

add Json schema compilation in smoke test #109

Closed
wants to merge 5 commits into from
Closed

add Json schema compilation in smoke test #109

wants to merge 5 commits into from

Conversation

wandmagic
Copy link
Contributor

Committer Notes

Fix JSON Schema validation issues

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 9, 2024 17:37
Copy link

XSpec Test Results

  4 files  +2   42 suites  +2   0s ⏱️ ±0s
109 tests +4   94 ✅ +4  15 💤 ±0  0 ❌ ±0 
118 runs  +4  103 ✅ +4  15 💤 ±0  0 ❌ ±0 

Results for commit d1365cb. ± Comparison against base commit 0010340.

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.

Are commits in this PR also in other outstanding PRs?

I need to be walked through this ... @wandmagic I can see a file adding to smoke test logic but do we want to accept the other commits here?

@wandmagic
Copy link
Contributor Author

sure i can cherry pick them, i still need to actually use the AJV to do a new separate test instead of adding it to an existing one

@wendellpiez
Copy link
Collaborator

Cool thanks. There is more being added to my working branch so this will have to be clean. Happy to discuss.

@@ -36,6 +36,9 @@
<flag ref="id"/>
<model>
<field ref="field-1only"/>
<field>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I brought this up in #111, but is there some intention we can explain there or here per https://github.com/usnistgov/metaschema-xslt/pull/111/files#r1524912715. It now seems 111 is only a dupe with this change but none of this substantive work in this PR, is there a reason for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was basically a requirement to get the schema to compile.
without this change we get two fields that try and find a JSON schema definition that doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so this is needed to fix a test we definitely have a bug on ours hands.

Is this #109 PR connected to a specific issue yet? Just got back from leave and I am talking to Wendell about this on Element/Matrix/Gitter/what-have-you, so I am trying to make sure I understand the context given the other (very good) changes in this PR, but this is basically invalid Metaschema so it caught my attention immediately. If we were purposefully testing "bad model behavior" and negative testing with this model I would let it go, but it seems that was not the intent of this model and behavior and we should identify it and move that out into a bug. Or be very lazy and fix it here, that just causes more confusing git commit reviews later on.

@wendellpiez, thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the scope of the PR, I am with AJ. If there is a bug in a metaschema that prohibits generating a JSON Schema, that is not a problem for this Issue but for another one, where we can bring the right attention to bear.

This doesn't mean the problem isn't real, just that we can't cut corners. Demonstration please. :-)

Boy would I love to be able to push a button and have the robots take my schema for a run -- I have that for XSD, can anyone help me with JSON Schema? See #110. Let's do that and determining whether metaschema X makes good JSON Schema will something we can see, not just argue about.

Meanwhile thanks to you both -- @wandmagic please do not hesitate to ask for assistance or further elucidation --

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the offending part of the schema when make test is run

use AJV to compile the json schema, and it complains about this base-64 malformed reference.

My change allows this to compile, but there may be a better way to fix this

Screenshot 2024-03-14 at 6 02 29 PM

Copy link
Collaborator

@wendellpiez wendellpiez Mar 15, 2024

Choose a reason for hiding this comment

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

Unfortunately the complexity of the system is such that both formal validation (against its own schema) and 'validation in operation' (i.e. try and see what happens) must be conducted. This is because although formal validation goes a long way to ensuring the correctness of the outputs wrt 'intent' (correctness defined by Metaschema semantics), it can't go the full distance.

To deploy and fully test a Metaschema (module) typically entails:

  • Formal validation (against the Metaschema XSD schema) of all Metaschema modules under 'import'
  • Schematron validation of top-level metaschem module to check imports
  • Run the Metaschema to produce derivative artifacts
    • Including XSD, JSON Schema, InspectorXSLT, oscal-cli version, converter XSLTs, documentation stacks, indexes, you name it
  • Unit test and field test any and all of these

We have been doing all this all along, albeit without much automation to help.

Indeed, that is essentially the problem we now have, that without being able both to record and document these processes, and automate testing to be supervised by Metaschema maintainers who are not developers -- we are essentially at a place where the knowledge of how it is done cannot be communicated. (Right, @iMichaela?)

The way I have been trying to address this is by starting to add tests, scripts and readmes in whatever application I need to be working on - recently JSON Schema (#108) and also InspectorXSLT (#98).

Oh look, there is #102 -- which I just merged. It contains work that could be further integrated into CI/CD as well -- I hope this helps.

Copy link
Collaborator

@wendellpiez wendellpiez Mar 15, 2024

Choose a reason for hiding this comment

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

Comment on my comment - note that the stack of tests/validations listed is a funnel. The rules enforced by the Metaschema XSD are very tight. But they are also assumed to be true (to be in force) by the Schematron at the next level down. This means the Schematron is much simpler than it would be otherwise, as it can be built with the XSD rules as "prior knowledge". Same with the next level. Because the Schematron has checked that imports work, the next level doesn't have to fail safe on that. Detecting and preventing the problems at the top of the stack is much easier, faster and easier to tailor than dealing with them at the bottom, where they are typically seen as "garbage out".

All this is in accordance with the 'functional' and 'declarative' mindset cultivated by XML, a descendant of LISP.

The stack is designed so that when a data instance (a Metaschema) passes both XSD and Schematron validation, everything below will be deterministic and 'just work'. So that any problems in unit testing the final XSDs and JSON Schemas etc can be treated as processor bugs to be reported back, and Metaschema authors do not have to be on the hook for processing errors (i.e. jump in and turn into developers).

@wandmagic wandmagic closed this Mar 18, 2024
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.

3 participants