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

Fix handling of allowed-values enumeration in JSON Schemas #253

Conversation

aj-stein-nist
Copy link
Collaborator

@aj-stein-nist aj-stein-nist commented Nov 10, 2022

Committer Notes

Closes #240.

To not clutter PR, sample JSON schema resulting from this change after update from previous PR review:

https://gist.github.com/aj-stein-nist/17c21528df4c12a33a49aff665b6e8e8/

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](https://pages.nist.gov/metaschema) and readme documentation affected by the changes you made? Changes to the website can be made in the website/content directory of your branch.

@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch 3 times, most recently from fd1d180 to 6446c73 Compare December 28, 2022 23:32
@aj-stein-nist aj-stein-nist marked this pull request as ready for review December 28, 2022 23:33
@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch from 6446c73 to 71233f4 Compare December 28, 2022 23:40
@aj-stein-nist
Copy link
Collaborator Author

@wendellpiez let me know how you feel about the cleaned up tests and how we ought to handle ongoing considerations, in the code to resolve this issue or elsewhere. Should we do a walk-through review to discuss changes and improvements (like the overlapping @target discussion)? Let me know what you think.

@aj-stein-nist aj-stein-nist linked an issue Dec 28, 2022 that may be closed by this pull request
@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch from de0a6bd to f9eccbe Compare January 6, 2023 18:01
@aj-stein-nist
Copy link
Collaborator Author

@david-waltermire-nist and @wendellpiez, this is ready for re-review.

@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch from 36d62ed to 40f1291 Compare January 10, 2023 18:33
@aj-stein-nist
Copy link
Collaborator Author

There does seem to be weirdness with how using the nm:compose-metaschema(/) function call works and fails the test as opposed to taking the resulting output from it, via the stylesheet below, and transforming that.

<!-- return-compose-metaschema.xsl file -->
<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
    xmlns:xs="http://www.w3.org/2001/XMLSchema"
    xmlns:math="http://www.w3.org/2005/xpath-functions/math" exclude-result-prefixes="xs math"
    xmlns:nm="http://csrc.nist.gov/ns/metaschema"
    version="3.0" xmlns="http://www.w3.org/2005/xpath-functions"
    xpath-default-namespace="http://csrc.nist.gov/ns/oscal/metaschema/1.0" expand-text="true">

    <!-- Purpose: Produce an XPath-JSON document representing JSON Schema declarations from Metaschema source data.
        The results are conformant to the rules for the XPath 3.1 definition of an XML format capable of being cast
        (using the xml-to-json() function) into JSON. -->
        
    <!-- Note: this XSLT will only be used on its own for development and debugging.
        It is however imported by `produce-json-converter.xsl` and possibly other stylesheets. -->
    
    <xsl:strip-space elements="METASCHEMA define-assembly define-field model"/>
    
    <xsl:output indent="yes" method="xml"/>
    <xsl:import href="../nist-metaschema-COMPOSE.xsl"/>
    
    <xsl:template match="/">
        <xsl:sequence select="nm:compose-metaschema(/)"/>
    </xsl:template>

</xsl:stylesheet>

This has left me scratching my head. Wendell believes, and there is a good chance, this difference has something to do with how XSpec builds context to make unit testing possible even with @run-as="external" something might be off. I need to explore that further. I will either resolve this hurdle in the next day or talk with @david-waltermire-nist about just adding became the "pre-composed metaschema" for the test and backing this out into a follow-on issue to figure out before it expends any other time.

@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch from 6dc496b to 31093a8 Compare January 17, 2023 22:31
@aj-stein-nist
Copy link
Collaborator Author

Touched up a rebase for now to come back to this in the evening or tomorrow morning.

@aj-stein-nist
Copy link
Collaborator Author

@wendellpiez recommended this morning an alternate approach and just using the shell (aka nist-metaschema-MAKE-JSON-SCHEMA.xsl and adjusting the tests accordingly. Experimenting with that now.

@aj-stein-nist
Copy link
Collaborator Author

aj-stein-nist commented Jan 19, 2023

I need to clean up base (weirdly git pull -r origin develop isn't doing what I'd expect), but I figured it out and this is ready for review.

Hat tip @liamquin, @martin-honnen, and of course @wendellpiez for helping me getting through what should have been an easy refactor.

@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch from b6e5f89 to 3b65cc1 Compare January 19, 2023 22:57
@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch from 3b65cc1 to 09c4afb Compare January 19, 2023 23:00
Copy link
Collaborator

@david-waltermire david-waltermire left a comment

Choose a reason for hiding this comment

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

The test vectors look good to me. I cannot weigh in on the XSLT. I'll rely on the review from @wendellpiez for that.

The shift in test approach, with the shell entrypoint at
ist-metaschema-MAKE-JSON-SCHEMA.xsl and not with the previous
make-json-schema-metamap.xsl replaced means that parameter, which
was not working particularly well in the previous approach.
@aj-stein-nist aj-stein-nist force-pushed the 240-json-schema-generation-yields-undesired-allowed-values branch from 3d628ac to 15b3b6d Compare January 24, 2023 15:22
@aj-stein-nist
Copy link
Collaborator Author

OK, @david-waltermire-nist and @wendellpiez I rebased to be clean against recent develop from yesterday afternoon, I have also created the requested tech debt issue I can begin working shortly.

usnistgov/metaschema-xslt#25

Please let me know if you need anything for finalizing a review and rolling this into develop and move onto the next issue.

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.

Looking good - so far!

@aj-stein-nist aj-stein-nist merged commit 5f51539 into usnistgov:develop Jan 25, 2023
aj-stein-nist added a commit to aj-stein-nist/OSCAL-forked that referenced this pull request Jan 31, 2023
 issue.

Determine if this submodule update fixes CI/CD breakage with JSON schema
or if other bugs and development issues leave it broken.
aj-stein-nist added a commit to aj-stein-nist/OSCAL-forked that referenced this pull request Feb 1, 2023
…bmodule-20230131

[WIP] Test out usnistgov/metaschema#253 fix for usnistgov/metaschema#…
david-waltermire pushed a commit that referenced this pull request Mar 9, 2023
* Fix JSON Schema generation for #240 and add tests.

* Adapt test suite approach per PR feedback.

* Final cleanup of unused parameter.

The shift in test approach, with the shell entrypoint at
ist-metaschema-MAKE-JSON-SCHEMA.xsl and not with the previous
make-json-schema-metamap.xsl replaced means that parameter, which
was not working particularly well in the previous approach.
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.

JSON Schema Generation Yields Undesired Allowed Values
3 participants