Profiler resolver support for importing another profile #1596
Conversation
|
I didn't check the boxes about examples, the OSCAL website, or readme documentation. I'm willing to contribute to those things but will need some clarification on what I should do. @david-waltermire-nist , can you help guide me? Thanks! |
|
@galtm I admit I am a bit of a freak for legibility in tracebacks. What about a rejigger of the reports format, maybe something like? You get the idea - something that exploits the fixed-width font of most terminals for ASCII-artish improved display (human consumers) as well as possibly later parsing/polling (bot consumers). What do you think? |
@wendellpiez : Good suggestion! I'll make some improvements and push them to this branch. |
44cd74d updates messages to come out like the following instead. @wendellpiez what do you think? Having an "end" message for steps 6 and 7 didn't seem that worthwhile, so I left that out. |
|
So awesome! |
aj-stein-nist
force-pushed
the
develop
branch
from
eff9ce3 to
dd6486a
Compare
|
Hi @galtm, thanks for submitting yet another excellent PR. We had to rebase develop and that leads to the "This branch has conflicts that must be resolved" conflicts below. I can help you with that, please let me know if and when you need that help. Thanks. |
galtm
force-pushed
the
profiler-recursive
branch
from
44cd74d to
883e1f3
Compare
|
@aj-stein-nist : I resolved the merge conflicts. If anything doesn't look right to you, let me know. Thanks. |
|
I tested this recently and found some anomalies: https://gitter.im/usnistgov-OSCAL/nist-fedramp-collaboration?at=63cba0f88e760b51fdfea008 |
@GaryGapinski , thanks a lot for testing this. The link doesn't work for me, though. Would you be able to post your findings here? |
|
I have a truly marvelous demonstration of this proposition that this margin is too narrow to contain. — Fermat Apologies: that was a Gitter link. Since then, I realized I had not checked out the I've been using SaxonHE 12.0 I cloned [email protected]:galtm/OSCAL.git and checked out the I created an alias for SaxonHE: I performed the transform Previously, on the wrong ( Since then I updated the workstation I am using from macOS 12.6.2 to 12.6.3. So, with the This is at odds with the examples in https://github.com/galtm/OSCAL/blob/profiler-recursive/src/utils/util/resolver-pipeline/readme.md. If I try qualifying the source document using a URI scheme (and thus a relative URI but one with a scheme), I get A couple of days ago (on the wrong branch and with a different OS version) I managed to produce a resolved catalog with just metadata and nothing else. So current state implies it works with non-relative source document URIs. I have not yet tried a profile importing a profile. |
|
Thanks, @GaryGapinski . The Saxon error message definitely looks like an XSLT bug. I'll try to reproduce your steps in the next several days and get back to you. (This isn't my day job, so thanks in advance for your patience.) PS - I love the Fermat quotation. |
|
I can also help look. It is probably something simple and small maybe in the interface. Thanks @GaryGapinski this is exactly what we need. δῶς μοι πᾶ στῶ καὶ τὰν γᾶν κινάσω (Archimedes) |
|
@GaryGapinski , please try the following situation:
but with one difference: try Saxon-HE version 10.8 (https://github.com/Saxonica/Saxon-HE/tree/main/10/Java) instead of version 11 or 12. I am able to reproduce the error message with Saxon-HE 11.4, and I believe it is due to a Saxon incompatibility that was introduced in version 11. (There is some discussion of it at https://saxonica.plan.io/issues/5339 .) By contrast, when I use Saxon HE 10.8, I get a schema-valid catalog file that includes metadata, groups of controls, and back matter. There are no empty href attributes, either. I logged issue #1629 and will plan to make the XSLT profile resolver compatible with Saxon 11 and up. Thanks for reporting this issue with the detail I needed to reproduce it. |
|
Works correctly with Saxon-HE 10.8. Thanks for finding the odd behavior change. Works with non-local source document: Edit: Outputs were validated (and found to be valid) using https://raw.githubusercontent.com/usnistgov/OSCAL/main/xml/schema/oscal_complete_schema.xsd. |
aj-stein-nist
left a comment
aj-stein-nist
left a comment
There was a problem hiding this comment.
I would personally like to have these tests automated in our build pipeline such that we catch the issue in #1629 before it may have been approved and merged otherwise. Whether this will be difficult in the scope of this PR or not, I would like to discuss with you @galtm and @wendellpiez. Can you set aside time to discuss when is prudent?
|
Indeed, although hardening it against arbitrary-older-versions of Saxon can be as onerous as we want it to be. It has been moving and indeed a Saxon12 is just out. So we have to draw the line. Best solution is probably to document the version being tested. If this is SaxonHE so much the better, as it also tests against slipping into a dependency on licensed-only features. If a developer can ensure that the pipeline works with both project-official-Saxon and latest Saxon, so much the better. @galtm any thoughts here? If we settle on a "current for now" Saxon version what would you recommend? Also glad we had our community find this, as it's a difficult problem to defend against. Lesson there might be that we did the right thing by asking @GaryGapinski to try. 🚀 |
Agreed. We should cover the different versions we do support though. Our CI/CD only uses one version, as does our development container. Can we sync up to discuss what versions we ought to support, given issues like this, and plot out how to keep CI/CD as-is or expand the matrix of supported versions accordingly?
Completely agree, lines up with what we can discuss at the next available opportunity.
I too would like to hear from them on this.
I too am glad but think this a problem we can do a small amount of work to defend against many of the common upgrade scenarios like this in the future as we change OSCAL and run across |
|
All sounds good. Stabilizing this saves everyone work in the medium term not only the long term. |
@aj-stein-nist it would be great to have these tests automated in the build pipeline. I think it is probably not in scope for this PR. Do you want to talk more about that with @wendellpiez and me this Friday morning, or another Friday?
My recommendation would be Saxon 11.4, assuming #1629 is straightforward to fix. The Saxonica web site currently says, "The latest maintenance release of 11 is Saxon 11.4, released on 28 July 2022. Saxon 11 is currently considered the most stable and reliable release." [UPDATE 5/4/2023: It now says Saxon 11.5 is the most stable and reliable release.] Although Saxon 12 has been released, I think making OSCAL use the most stable and reliable release of Saxon makes sense. What do you think? |
@aj-stein-nist : Some quick pointers about how the github.com/xspec/xspec repo automates running its own XSpec tests in GitHub:
|
|
3/9/23: Manually resolved merge conflicts now that #1685 has been merged into develop branch. |
Arminta-Jenkins-NIST
force-pushed
the
develop
branch
from
fb85a27 to
6661368
Compare
aj-stein-nist
changed the title
aj-stein-nist
changed the base branch from
develop
to
feature-recursive-profile-resolver
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.
aj-stein-nist
force-pushed
the
profiler-recursive
branch
from
1179967 to
b60589e
Compare
aj-stein-nist
changed the title
aj-stein-nist
left a comment
aj-stein-nist
left a comment
There was a problem hiding this comment.
Thanks again for your work, @galtm. I backed up the branch before rebase in my fork just in case, but this looks ready to be merged into a feature branch. This work is not fixing a bug and should be prioritized for inclusion in a later minor release since it adds new functionality, after other bug fixes, including other wonderful additions by you to the profile resolver and testing. This merged recursive profile resolver work will be merged at a later date.
Excellent contribution and keep up the good work.
4 participants
Committer Notes
This PR implements support in the XSLT profile resolver for an XML profile importing an XML profile. This is a feature that the spec describes and that was partially implemented by @wendellpiez earlier.
This PR builds on top of #1549, so it might be simpler to merge that PR first. Let me know if I should create this PR in a different way when it depends on unmerged code. What's new in this PR are commits ccef4cf and 883e1f3, containing the following pieces.
XSLT changes:
so it can be passed back to top level in recursion
and after, to make recursive operation clearer
circular references among profiles
XSpec changes:
Sample profile documents with their expected output:
profile and in turn imports another profile
Sample trace messages showing recursion
When I enabled tracing and ran the resolver on
import-profile1_profile.xmlwhich importsimport-profile2_profile.xmlwhich in turn importsbase-test_profile.xml, the messages about steps in the pipeline were in the following sequence:All Submissions:
"?
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 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.