Profile resolver compatibility with Saxon 11 (on top of other unmerged PRs) #1639

galtm · 2023-02-07T00:30:58Z

Committer Notes

This PR addresses issue #1629 by replacing usage of document-uri with base-uri. The document-uri function is known to return different results in Saxon 10 vs. 11. Using base-uri removes a difference in the XSLT profile resolver's behavior between Saxon 10 and 11.

This PR builds on top of #1596, which in turn builds on #1549. What's new in this PR is commit 3c96945.

All Submissions:

Have you selected the correct base branch per Contributing guidance?
Have you set "Allow edits and access to secrets by maintainers
"?
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?

By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.

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 OSCAL website and readme documentation affected by the changes you made? Changes to the OSCAL website can be made in the docs/content directory of your branch.

Testing Done

Ran all the unit-level XSpec tests (that is, the .xspec files in the testing/ directory) with Oxygen 24.1, which uses Saxon 10.
Ran all the unit-level XSpec tests with Oxygen 25.0, which uses Saxon 11.
With both Oxygen 24.1 and 25, transformed all the profiles in src/specifications/profile-resolution/profile-resolution-examples. Diff'd the results, and they are the same except time stamps. The profiles that deliberately error out (e.g., broken_profile.xml) behave the same way in both Oxygen versions.
Used the command in the "How do we replicate this issue" section of XSLT profile resolver does not work with Saxon version 11 #1629. Now the command works with Saxon 12.0.

* Separate before/after from starting/ending in modify phase * Enhance message-handler.xsl for pipeline usage and testability * Adapt XSLT to message handler returning PI * Enable saving intermediate XML files for debugging purposes * Add sample document for nonfatal errors/warnings Bug fixes: * Enable param to have insertions before/after AND have param set * Fix merge phase bug where descendant controls weren't being flattened

XSLT changes: * Pass $trace global param from top level to selection phase so it can be passed back to top level in recursion * When calling pipeline steps, output a trace message before and after, to make recursive operation clearer * Handle terminating errors during recursion, including circular references among profiles XSpec changes: * Update tests for match='profile' mode='o:select' template * Update tests for o:resolve-profile function Sample profile documents with their expected output: * import-profile_profile.xsl * import-profile2_profile.xsl, which is imported by the above profile and in turn imports another profile

Now that opr:oscal-version returns item()+ to accommodate error PI, output of $source should use xsl:sequence instead of xsl:value-of to retain string data type. XSpec tests for this function should expect PI, not error map.

Replace document-uri with base-uri. Saxon 10 and 11 can return different values from document-uri() function. For compatibility and to avoid errors when document-uri() returns empty with Saxon 11, use base-uri() instead.

galtm · 2023-02-07T00:34:39Z

Hi, @aj-stein-nist and @wendellpiez . I mentioned in the description that this PR builds on two earlier unmerged PRs. That seemed like the simplest approach, and it means that end users who want to try out the latest XSLT profile resolver can try it from this branch instead of having to merge code themselves. If you want me to rebase this code differently, let me know.

GaryGapinski · 2023-02-10T17:48:24Z

I tested this (https://github.com/galtm/OSCAL/tree/saxon11) with Saxon-HE v11.5 and v12.0 and my tests succeeded.

wendellpiez

Please to accept the PR! Much awesome work here. Going beyond the Saxon compatibility correction it includes more features for exception-handling and tracing as well as more/better tests.

aj-stein-nist

EDIT: Woops, I pressed the wrong button. These changes are still excellent, but I do want the PR ~~fixed~~ changed first. See below. 👇

aj-stein-nist

First off, thank you very much for taking the time to do this, @galtm. I reviewed and focused on the Saxon 11 compatibility fixes and tests with the specific commit. I had to cherry-pick it to realize it was actually helpful to fix another big that is breaking our pipelines.

To that end, can you please update the PR to back out other changes and only include the 3c96945 work? I would like to test the feature improvements for profile resolution separate from this bug fix, and it is good we merge this sooner, not later. If you need my help with that or prefer I do it for myself, please let me know. Since it is your branch, I would like to ask for permission first, not forgiveness. :-)

galtm · 2023-02-28T00:54:06Z

To that end, can you please update the PR to back out other changes and only include the 3c96945 work?

@aj-stein-nist : The galtm:saxon11-off-develop branch has only the Saxon 11 compatibility work, on top of the develop branch's head. You can see the commit here: galtm@91a1243

The commit on that branch is a little different from commit [3c96945] in this PR, because the work for #1549 and #1596 intersects with the Saxon 11 compatibility changes. I don't want to accidentally lose the updates in the intersecting code as a result of changing this PR, so I'm hesitant to change this PR immediately. Does staging it according to the following plan make sense?

I make a new PR from galtm:saxon11-off-develop
You merge the new PR
I push new commits to Profile resolver error handling #1549 and Profiler resolver support for importing another profile #1596 so that they maintain compatibility with Saxon 11
One of us closes this PR (without merging) after we determine that its code is already part of the other PRs

wendellpiez · 2023-02-28T14:28:36Z

I have already explained to AJ that it's my fault the stuff is all together. Planning for the wrong contingency.

galtm · 2023-02-28T14:31:18Z

I have already explained to AJ that it's my fault the stuff is all together. Planning for the wrong contingency.

No worries. I don't see it as your fault and I don't think it even matters. We'll move forward in some fashion and get where we need to be.

aj-stein-nist · 2023-02-28T14:31:34Z

To that end, can you please update the PR to back out other changes and only include the 3c96945 work?

@aj-stein-nist : The galtm:saxon11-off-develop branch has only the Saxon 11 compatibility work, on top of the develop branch's head. You can see the commit here: galtm@91a1243

The commit on that branch is a little different from commit [3c96945] in this PR, because the work for #1549 and #1596 intersects with the Saxon 11 compatibility changes. I don't want to accidentally lose the updates in the intersecting code as a result of changing this PR, so I'm hesitant to change this PR immediately. Does staging it according to the following plan make sense?
1. I make a new PR from `galtm:saxon11-off-develop`

2. You merge the new PR

3. I push new commits to [Profile resolver error handling #1549](https://github.com/usnistgov/OSCAL/pull/1549) and [Profiler resolver support for importing another profile #1596](https://github.com/usnistgov/OSCAL/pull/1596) so that they maintain compatibility with Saxon 11

4. One of us closes this PR (without merging) after we determine that its code is already part of the other PRs

That plan is fine by me, thank you! And no worries, Wendell, we can help each other. If @galtm feels this course of action is better than cherry-picking or rebasing, we are good. In the future, I will be more clear about expectations around these things in advance.

galtm · 2023-02-28T14:34:52Z

That plan is fine by me,

OK, great. I will redo the testing on the other branch before I make the new pull request, just to make sure. Look for the new pull request in the next couple of days.

galtm · 2023-03-02T13:47:57Z

That plan is fine by me,

OK, great. I will redo the testing on the other branch before I make the new pull request, just to make sure. Look for the new pull request in the next couple of days.

@aj-stein-nist : The new pull request is #1685.

aj-stein-nist · 2023-03-03T02:19:32Z

OK, @galtm, are we good to close this one in favor of #1685 and track the other feature PRs after a rebase? I do not want to lose track of your efforts. Thanks for working with us as I had asked.

galtm · 2023-03-03T19:56:25Z

OK, @galtm, are we good to close this one in favor of #1685 and track the other feature PRs after a rebase? I do not want to lose track of your efforts. Thanks for working with us as I had asked.

@aj-stein-nist , yes, this pull request can be closed without merging it. I'll update the other two PRs to bring them up to date with #1685.

galtm · 2023-03-10T21:47:57Z

I'll update the other two PRs to bring them up to date with #1685.

Done!

galtm added 6 commits January 12, 2023 19:20

Make trace messages neater

883e1f3

Update opr:oscal-version and its test for error handling

6599e16

Now that opr:oscal-version returns item()+ to accommodate error PI, output of $source should use xsl:sequence instead of xsl:value-of to retain string data type. XSpec tests for this function should expect PI, not error map.

Update expected message to reflect tweaks in XSLT verbiage

0898883

Compatibility with Saxon 10 and 11

3c96945

Replace document-uri with base-uri. Saxon 10 and 11 can return different values from document-uri() function. For compatibility and to avoid errors when document-uri() returns empty with Saxon 11, use base-uri() instead.

GaryGapinski mentioned this pull request Feb 19, 2023

Profile resolution flattens nested control structure #1663

Open

nikitawootten-nist mentioned this pull request Feb 22, 2023

Profile resolution how-to readmes #1178

Closed

aj-stein-nist requested review from wendellpiez and aj-stein-nist February 23, 2023 21:21

wendellpiez approved these changes Feb 24, 2023

View reviewed changes

aj-stein-nist approved these changes Feb 27, 2023

View reviewed changes

aj-stein-nist suggested changes Feb 27, 2023

View reviewed changes

aj-stein-nist mentioned this pull request Feb 27, 2023

Profile resolution how-to readmes #1666

Merged

7 tasks

aj-stein-nist mentioned this pull request Mar 2, 2023

Upgrade GitHub CI/CD Workflows to Current Stable Version usnistgov/oscal-content#116

Closed

6 tasks

galtm mentioned this pull request Mar 2, 2023

Profile resolver: Compatibility with Saxon 10 and 11 #1685

Merged

7 tasks

galtm changed the title ~~Profile resolver compatibility with Saxon 11~~ Profile resolver compatibility with Saxon 11 (on top of other unmerged PRs) Mar 2, 2023

galtm closed this Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profile resolver compatibility with Saxon 11 (on top of other unmerged PRs) #1639

Profile resolver compatibility with Saxon 11 (on top of other unmerged PRs) #1639

galtm commented Feb 7, 2023

galtm commented Feb 7, 2023

GaryGapinski commented Feb 10, 2023

wendellpiez left a comment •

edited

Loading

aj-stein-nist left a comment •

edited

Loading

aj-stein-nist left a comment

galtm commented Feb 28, 2023

wendellpiez commented Feb 28, 2023

galtm commented Feb 28, 2023

aj-stein-nist commented Feb 28, 2023

galtm commented Feb 28, 2023

galtm commented Mar 2, 2023

aj-stein-nist commented Mar 3, 2023

galtm commented Mar 3, 2023

galtm commented Mar 10, 2023

Profile resolver compatibility with Saxon 11 (on top of other unmerged PRs) #1639

Profile resolver compatibility with Saxon 11 (on top of other unmerged PRs) #1639

Conversation

galtm commented Feb 7, 2023

Committer Notes

All Submissions:

Changes to Core Features:

Testing Done

galtm commented Feb 7, 2023

GaryGapinski commented Feb 10, 2023

wendellpiez left a comment • edited Loading

Choose a reason for hiding this comment

aj-stein-nist left a comment • edited Loading

Choose a reason for hiding this comment

aj-stein-nist left a comment

Choose a reason for hiding this comment

galtm commented Feb 28, 2023

wendellpiez commented Feb 28, 2023

galtm commented Feb 28, 2023

aj-stein-nist commented Feb 28, 2023

galtm commented Feb 28, 2023

galtm commented Mar 2, 2023

aj-stein-nist commented Mar 3, 2023

galtm commented Mar 3, 2023

galtm commented Mar 10, 2023

wendellpiez left a comment •

edited

Loading

aj-stein-nist left a comment •

edited

Loading