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

Correctly pruning unused definitions in metaschema composition #199

Conversation

wendellpiez
Copy link
Collaborator

@wendellpiez wendellpiez commented Apr 6, 2022

By replacing the pruning step in metaschema composition, this closes #180 and #198.

Also we no longer need #196 with this PR (right @aj-stein-nist ?)

Also includes plenty of good work towards #81 #186, usnistgov/metaschema-xslt#19.

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.

@wendellpiez
Copy link
Collaborator Author

This PR now contains a solution, with running unit tests, to expose and remedy #198.

I also suggest we try results from this pipeline applied to OSCAL metaschemas, to head off potential future unpleasantness.

@wendellpiez wendellpiez changed the title Metaschema composition XSpecs Correctly pruning unused definitions in metaschema composition Apr 12, 2022
@wendellpiez wendellpiez force-pushed the issue196-definition-pruning-enhancement branch from 3e5217f to 9cc3fc1 Compare April 12, 2022 18:48
@wendellpiez wendellpiez marked this pull request as ready for review April 12, 2022 19:00
@aj-stein-nist
Copy link
Collaborator

Hey @wendellpiez, Dave was reviewing PRs with me and asked for a brief on #196 and reminded him the work is going to continue here. Is this issue ready for review and merge or are you still working on it?

@wendellpiez
Copy link
Collaborator Author

@david-waltermire-nist and @aj-stein-nist - this issue as scoped above is done: this PR contains a new XSLT step for pruning that repairs the functional bug, plus a unit test that demonstrates the solution. This addresses both #180 and #198.

It also includes lots of other efforts (Metaschema unit tests) described in other Issues, so it becomes a dependency for them. Please pull whenever convenient!

@aj-stein-nist
Copy link
Collaborator

@david-waltermire-nist and @aj-stein-nist - this issue as scoped above is done: this PR contains a new XSLT step for pruning that repairs the functional bug, plus a unit test that demonstrates the solution. This addresses both #180 and #198.

Sounds good. Shall we meet and get it rebased and good to go for final review by Dave?

@aj-stein-nist aj-stein-nist changed the base branch from master to develop April 18, 2022 20:11
@aj-stein-nist aj-stein-nist changed the base branch from develop to master April 18, 2022 20:13
@aj-stein-nist
Copy link
Collaborator

I am going to review it as-is, @david-waltermire-nist, but @wendellpiez and I discussed this yesterday afternoon and we wanted confirmation: current sprint work like this is target master and not the develop branch, correct? I compared to the two branches and more urgent pressing work is being merged into master, so I presume this will need to get updated, but want to make sure.

aj-stein-nist
aj-stein-nist previously approved these changes Apr 19, 2022
Copy link
Collaborator

@aj-stein-nist aj-stein-nist left a comment

Choose a reason for hiding this comment

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

Looks good, I have mostly minor feedback and one question about a modification to the Metaschema collection part of the composition pipeline. Other than that, nice work and thanks for cleaning up the repo!

Comment on lines 22 to 42
<!--<xsl:strip-space
elements="catalog metadata param control group back-matter prop link part usage constraint guideline select remarks description test revisions revision role location party responsible-party address external-id resource citation rlink base64"/>-->


<!--All assemblies defined in the catalog model:

https://raw.githubusercontent.com/wendellpiez/OSCAL/profile-resolution-update-v1a/xml/schema/oscal_catalog_schema.xsd

//xs:element[exists(xs:complexType[not(@mixed='true')])]/@name => distinct-values() => string-join(' | ')


catalog | metadata | param | control | group | back-matter | prop | link | part | usage | constraint | guideline | select | remarks | description | test | revisions | revision | role | location | party | responsible-party | address | external-id | resource | citation | rlink | base64


-->
<!--<xsl:template match="/">
<xsl:param name="source" select="." as="document-node()"/>
<xsl:document>
<xsl:apply-templates select="$source" mode="scrubbing"/>
</xsl:document>
</xsl:template>-->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: should we not just delete this because it is dead code?

Suggested change
<!--<xsl:strip-space
elements="catalog metadata param control group back-matter prop link part usage constraint guideline select remarks description test revisions revision role location party responsible-party address external-id resource citation rlink base64"/>-->
<!--All assemblies defined in the catalog model:
https://raw.githubusercontent.com/wendellpiez/OSCAL/profile-resolution-update-v1a/xml/schema/oscal_catalog_schema.xsd
//xs:element[exists(xs:complexType[not(@mixed='true')])]/@name => distinct-values() => string-join(' | ')
catalog | metadata | param | control | group | back-matter | prop | link | part | usage | constraint | guideline | select | remarks | description | test | revisions | revision | role | location | party | responsible-party | address | external-id | resource | citation | rlink | base64
-->
<!--<xsl:template match="/">
<xsl:param name="source" select="." as="document-node()"/>
<xsl:document>
<xsl:apply-templates select="$source" mode="scrubbing"/>
</xsl:document>
</xsl:template>-->

Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird I am not sure why it gets highlighted to be like it is not commented code in the suggestion diff, but it is! haha

test-suite/metaschema-xspec/readme.md Outdated Show resolved Hide resolved
schematron="../../../toolchains/xslt-M4/validate/metaschema-composition-check.sch"
run-as="external">

<!--<x:param name="global-context" select="document('scope-local-importing_metaschema.xml')"/>-->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: consider deleting if unused?

Suggested change
<!--<x:param name="global-context" select="document('scope-local-importing_metaschema.xml')"/>-->

Comment on lines +177 to 179
<!--<p:with-param name="show-warnings" select="'yes'"/>-->
<!-- With EXCEPTION problem-type="missing-root" for no roots found when the metaschema is not abstract -->
<!-- With EXCEPTION problem-type="unused-definition" for definitions removed as unused -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: consider deleting if unused?

Suggested change
<!--<p:with-param name="show-warnings" select="'yes'"/>-->
<!-- With EXCEPTION problem-type="missing-root" for no roots found when the metaschema is not abstract -->
<!-- With EXCEPTION problem-type="unused-definition" for definitions removed as unused -->

toolchains/xslt-M4/compose/readme.md Show resolved Hide resolved
toolchains/xslt-M4/compose/testing/.gitignore Show resolved Hide resolved
toolchains/xslt-M4/compose/testing/readme.md Show resolved Hide resolved
@aj-stein-nist
Copy link
Collaborator

Per our conversation earlier this afternoon, I was able to make a branch on my fork of OSCAL and test the output line by line just to make double sure. It looks good, line-for-line parity with all models, couldn't get anything to commit on my end.

@wendellpiez
Copy link
Collaborator Author

Next committing an update addressing points of discussion above.

@aj-stein-nist
Copy link
Collaborator

I am going to review it as-is, @david-waltermire-nist, but @wendellpiez and I discussed this yesterday afternoon and we wanted confirmation: current sprint work like this is target master and not the develop branch, correct? I compared to the two branches and more urgent pressing work is being merged into master, so I presume this will need to get updated, but want to make sure.

@wendellpiez sorry for the delay on this but I talked to Dave and he said he wanted this one pointing to the develop branch.

@david-waltermire david-waltermire changed the base branch from main to develop April 28, 2022 16:51
@david-waltermire david-waltermire force-pushed the issue196-definition-pruning-enhancement branch from 549c331 to 3719be7 Compare April 28, 2022 17:27
@david-waltermire david-waltermire force-pushed the issue196-definition-pruning-enhancement branch from 1368b02 to c3f8a16 Compare April 28, 2022 17:47
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.

Looks good to me.

@david-waltermire david-waltermire linked an issue Apr 28, 2022 that may be closed by this pull request
3 tasks
@david-waltermire david-waltermire merged commit 42db043 into usnistgov:develop Apr 28, 2022
david-waltermire added a commit that referenced this pull request Jun 21, 2022
* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for #198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - #81 #186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
david-waltermire added a commit that referenced this pull request Dec 7, 2022
* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for #198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - #81 #186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
aj-stein-nist added a commit to aj-stein-nist/metaschema that referenced this pull request Jan 9, 2023
…tgov#199)

* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for usnistgov#198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - usnistgov#81 usnistgov#186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
aj-stein-nist added a commit to aj-stein-nist/metaschema that referenced this pull request Jan 10, 2023
…tgov#199)

* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for usnistgov#198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - usnistgov#81 usnistgov#186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
aj-stein-nist added a commit to aj-stein-nist/metaschema that referenced this pull request Jan 10, 2023
…tgov#199)

* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for usnistgov#198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - usnistgov#81 usnistgov#186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
aj-stein-nist added a commit to aj-stein-nist/metaschema that referenced this pull request Jan 10, 2023
…tgov#199)

* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for usnistgov#198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - usnistgov#81 usnistgov#186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
aj-stein-nist added a commit to aj-stein-nist/metaschema that referenced this pull request Jan 10, 2023
…tgov#199)

* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for usnistgov#198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - usnistgov#81 usnistgov#186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
aj-stein-nist added a commit to aj-stein-nist/metaschema that referenced this pull request Jan 10, 2023
…tgov#199)

* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for usnistgov#198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - usnistgov#81 usnistgov#186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
david-waltermire added a commit that referenced this pull request Mar 9, 2023
* Metaschema composition XSpecs
* Adding 'a8' prune-unused-definitions filter for comparison (most advanced yet).
* Added new prune filter with slight adjustments to interface; new test for running it standalone
* Removing noisy XSpec result
* More tuning up v9 pruning step for #198
* Cleanup; restoring top-level compose XSLT
* Updating files before merge
* First draft of testing approach with current WIP prune phase transform.
* Fix tests to align with Wendell exception msg check.
* Move pruning tests back to their own folder.
* Refactor test input XML docs into separate files
* Make the tests more reusable across implementations.
* Relocate input files to subdir per convo with @wendellpiez.
* Slight improvement to prototype
* Much cleanup and rearrangement; readme docs
* Updating readme plus entry (shell) XSLT and XProc to current
* Removing obsolete XSpec reports
* Adding .gitignore to exclude XSpec results
* More rearrangement and cleanup; adding step 4 XSpec
* New filter detecting and removing unused definitions passes unit tests
* Removing early copy of remedied step 4 XSLT
* Updating Metaschema Schematron XSpecs to functional state: they are now stable, addressing #47
* More adjustments prepping to M4 unit tests - #81 #186 #201 - now everything is green or grey (pending) no pink
* Added more clarification to pruning XSpec
* Edits in response to feedback from @aj-stein-nist thanks AJ!
* adding blank lines at end of files

Co-authored-by: Alexander Stein <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants