Use anyOf when @allow-other = 'yes' in JSON Schema

[laurelmay]

Committer Notes

When there is a type and an enumeration in JSON Schema, using allOf
means that a value must satisfy both types. This is appropriate when
@allow-other = 'no' because it effectively limits the permitted values
to the enumeration; however, when @allow-other = 'yes', that
limitation means that you are not able to use 'locally defined' values.
In this case, anyOf is preferable, this allows values in the
enumeration and otherwise any value that matches the schema for the
type.

A check is added explicitly for @allow-other = 'yes' as
@allow-other = 'no' is the default and matches the existing behavior.

This follows the pattern used in #253.

metaschema/toolchains/xslt-M4/schema-gen/make-json-schema-metamap.xsl

Lines 328 to 352 in 7008554

	<xsl:choose>
	<xsl:when test="constraint/allowed-values/@allow-other = 'yes' and (constraint/allowed-values/@target = '.' or empty(constraint/allowed-values/@target))">
	<array key="anyOf">
	<map>
	<xsl:sequence select="$nominal-object-type"/>
	</map>
	<map>
	<xsl:apply-templates select="constraint/allowed-values"/>
	</map>
	</array>
	</xsl:when>
	<xsl:when test="constraint/allowed-values/@allow-other = 'no' and (constraint/allowed-values/@target = '.' or empty(constraint/allowed-values/@target))">
	<array key="allOf">
	<map>
	<xsl:sequence select="$nominal-object-type"/>
	</map>
	<map>
	<xsl:apply-templates select="constraint/allowed-values"/>
	</map>
	</array>
	</xsl:when>
	<xsl:otherwise>
	<xsl:sequence select="$nominal-object-type"/>
	</xsl:otherwise>
	</xsl:choose>

Closes: usnistgov/OSCAL#1773

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? @aj-stein-nist will do this in a follow-on PR
• 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.

[laurelmay]

Differences in the OSCAL repo after running build/ci-cd/generate-schema.sh with this branch can be seen at usnistgov/OSCAL@main...kylelaker:OSCAL:oscal-jsonschema-allow-other-repro

[david-waltermire]

@wendellpiez , @aj-stein-nist , and @kylelaker Do we need a new unit test to check for this?

[aj-stein-nist]

@wendellpiez , @aj-stein-nist , and @kylelaker Do we need a new unit test to check for this?

I am looking into this more thoroughly but I am not sure what we had intended but I checked the following test (and intentionally mangled it locally to see the test generated correctly.

Given this test Metaschema module below, the following JSON schema is desired for the allow-other="yes" case.

https://github.com/usnistgov/metaschema/pull/253/files#diff-dae4d2728d103a48a2c7e833fd994fc629956156de0372ddd93716c16ede5a56

https://github.com/usnistgov/metaschema/pull/253/files#diff-4d22d8b5a7ea6dd9c55c7b943a106e921e9c26d491cd135d1de9e7ad00fa799b

I see how you can code around it with your PR, but this means in OSCAL something is being missed and the following code is not handling a certain condition the way it should.

metaschema/toolchains/xslt-M4/schema-gen/make-json-schema-metamap.xsl

Lines 329 to 338 in 7008554

	<xsl:when test="constraint/allowed-values/@allow-other = 'yes' and (constraint/allowed-values/@target = '.' or empty(constraint/allowed-values/@target))">
	<array key="anyOf">
	<map>
	<xsl:sequence select="$nominal-object-type"/>
	</map>
	<map>
	<xsl:apply-templates select="constraint/allowed-values"/>
	</map>
	</array>
	</xsl:when>

In short, this is short-circuiting that code to avoid the bug. I guess this is an approach, but it calls into question why the test works in Metaschema but fails in the OSCAL case. I have investigated this locally on and off this evening, but I want to track down the root cause before approving this or recommending a different course of action (would also like Wendell's take on this).

Apologies for the delays, but I will continue to look into it.

[aj-stein-nist]

@kylelaker, thanks for the PR. You can disregard my previous comments and we paired as team. It comes full circle, I now better understand why this change has to happen.

It seems we have been testing constraints on them being closed, loose, or totally open on fields, not on flags. You have fixed the part of the code we did not address in some previous work that missed this case after updating several enhancements in develop, so we really need to add some tests to the schema generation suite to address both flags and fields.

So in summation:

1. Great work! 🎉
2. You indicate some area of the test coverage I missed in previous work. 😢
3. One of us needs to write tests: do you want to do it, I do it, or we work on it together? Let me know!

[laurelmay]

Thanks for the feedback. To get started with, I will work on integrating the specific feedback you provided in the next day or so.

As far as writing tests, I am going to be 100% honest here and say that I am not really sure how to go about that... I neglected looking into that during the original PR. I'm happy to read and guess at it but if you want it done well/quickly it'd probably best to either have you do it or to at least collaborate.

[aj-stein-nist]

I'm happy to read and guess at it but if you want it done well/quickly it'd probably best to either have you do it or to at least collaborate.

I am fine either way, if you want to collaborate on this, what is your week looking like? I would love to collaborate on community-authored tests (whether I drive or back-seat drive). And if that is too much for this week, totally understandable, I just PR recommended tests to your branch and we go through them?

[aj-stein-nist]

OK I guess I should get started on those tests then. :-)

[aj-stein-nist]

I am ok with the changes but will attempt to add relevant tests to ensure this is working in another PR.

[wendellpiez]

Agree with @aj-stein-nist . The changes look fine in the view and may not be likely to break anything 'worse', but the only way to prevent further thrashing is to prove out the full range of cases (with and without @target and with @target='.' in unit tests such that any of us can validate it. So let's look at expanding the XSpec coverage as well.

[aj-stein-nist]

but the only way to prevent further thrashing is to prove out the full range of cases (with and without @target and with @target='.' in unit tests such that any of us can validate it

We do actually test for that, but we don't test the different scenarios for allowed-values for both fields and flags, that was the but I missed. I'll start work on it (albeit outside this PR, in a separate one).

[aj-stein-nist]

OK, so this is a fun teachable moment. @david-waltermire-nist, please revert the PR back as-is when Kyle originally submitted the PR (in commit 6fb8a74) and I provided stylistic feedback. By doing that, I introduced a subtle bug and found it through testing #380. When testing some of the variations with the flag, I realized that we had inadvertently taken my recommendation to collapse the second if clause (or when rather and that is distinct but different. With #380 I now have coverage and noticed some errors I couldn't resolve until going back to the make-json-schema-metamap.xsl in the original.

I could find clever refactors or what not, but the real go of my previous review was to ensure I understood this submission by Kyle through tests and verify it, and in adding stuff I broke his original submission. So let's go back with that, and also add the tests with it or after. I leave that to your discretion. :-)

[aj-stein-nist]

OK, I found another edge case while touching up my tests and I cannot easily recommend the change or commit directly to Kyle's PR. I am going to recommend we close this one out, I pull in his change, and also update the relevant code in a part Kyle didn't notice to address the edge case in both places.

Please a revised #380 with updated comments shortly.