Update Converter Generators to Emit Missing IDs for Fields in Certain Assembly Contexts

[aj-stein-nist]

Committer Notes

Closes #235.

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]

More testing and research is needed. Will want to confirm with @david-waltermire-nist if this needs to be pulled into the sprint or not for complete resolution.

For more realistic end-to-end tests, @wendellpiez and I ran oscal-content SSPs and catalogs through the pipeline locally with this change. There is minimal, if any changes, detected by git after the JSON is reformatted with whitespacing cleanup. More testing is likely needed and maybe a RCA. Will wait to discuss the issue and PR in triage next week.

[aj-stein-nist]

Per feedback from Dave, review count(parent::field/parent::group[@in-json='SINGLETON_OR_ARRAY']/child::*) construct around SINGLETON_OR_ARRAY in particular, using the test suite, and ensure the RHS of the boolean is accounted for. He would like us to be certain and account for that before removing the draft marking and declaring it ready for review.

[aj-stein-nist]

Per feedback from Dave, review count(parent::field/parent::group[@in-json='SINGLETON_OR_ARRAY']/child::*) construct around SINGLETON_OR_ARRAY in particular, using the test suite, and ensure the RHS of the boolean is accounted for. He would like us to be certain and account for that before removing the draft marking and declaring it ready for review.

Talked with Dave and Wendell about this at the tail end of triage.

• Create a new test around this issue for the converter generator (not use and validate an existing test)
  • Test correct behavior
  • Test failure behavior
• Mark this PR ready for review

[aj-stein-nist]

OK with #240 out of the way, I need to come back this to understand it better.

[wendellpiez]

The PR includes a schema correction and XSpec for it. What do we need to finish? Could be an easy one.

If not clear, then maybe this is an Issue-not-an-Issue, or maybe we need to return to #235.

[aj-stein-nist]

The PR includes a schema correction and XSpec for it. What do we need to finish? Could be an easy one.

If not clear, then maybe this is an Issue-not-an-Issue, or maybe we need to return to #235.

Per a woefully under-detailed comment from before, I will need a solid block of time, perhaps 2 hours or more, to integrate work upstream in OSCAL and verify the bug is a no-op or bad report. The commit change did not materially expose a bug or a deviation in behavior with the PR, hence the head scratch. I apologize for the delays.

[aj-stein-nist]

@david-waltermire-nist, I have spent some time thinking about this but cannot really envision more tests for other edge cases or other things needed. So for now, I would like to request a review of the PR. If merged, we can move forward accordingly, thanks!

/cc @wendellpiez if you have any feedback or thoughts on the tests or additional edge cases, let me know, I could not think of any.

[wendellpiez]

👍 If we are happy with the testing we have and confident we can build on that going forward, we could merge this and let it rest.

It's not that there is not more testing, it's that defining the bounds of such testing is itself an exercise. Indeed as we have discovered, testing the converter generator is nigh-impossible without also being able to test (and validate) the converters they are generating.

This suggests that the time to pick this up again is when we have robust testing of bi-directional conversion from valid XML to JSON (or other syntax/es) and back. Maybe this means a spin-off Issue to work on the testing.

[aj-stein-nist]

@david-waltermire-nist any update on review for this PR? Let me know if you have any questions, comments, or concerns.

[david-waltermire]

This looks consistent with what we discussed over the discussion thread. Approved. Thanks!