Skip to content

Fix profile resolution when children of nested controls selected #233

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix profile resolution when children of nested controls selected #233

wants to merge 2 commits into from

Conversation

@aj-stein-nist
Copy link
Contributor

@aj-stein-nist aj-stein-nist commented Feb 1, 2024 •
edited
Loading

Committer Notes

Closes #232.

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 and readme documentation affected by the changes you made?

@aj-stein-nist aj-stein-nist self-assigned this Feb 1, 2024
@aj-stein-nist aj-stein-nist changed the base branch from main to develop February 1, 2024 15:01
@aj-stein-nist
Fix unselected control handling for usnistgov#232
37fcc27 
This may be a business logic error or perhaps at one time there was/is
a valid reason, but as it stands the BasicIndexer would detect
unselected controls for resolution of the profile but only drop them
when their parent was selected for the default strategy of flattening.
For simple trees where c1 is the root control and c1.1 is the child, so
c1 -> c1.1, c1 was not being correctly removed.

NOTE: Logging indicates a double index of c1.1 even with this change
so that still needs to be fixed most likely to resolve this issue.
@aj-stein-nist aj-stein-nist force-pushed the 232-fix-profile-resolution branch from 19e4ca4 to 37fcc27 Compare February 1, 2024 15:10
@aj-stein-nist
Copy link
Contributor Author

@david-waltermire I am going to ask for a preliminary review but the fix seems pretty simple so I am worried I am misunderstand the scope of the issue. Additionally, when running the regression test, I am notice in the gov.nist.secauto.oscal.lib.profile.resolver.ProfileResolver.resolveProfile() function I get the following warning because of c1.1 control being a dupe at the handleMerge() stage because of duplicate index records.

10:39:58.383 [main] DEBUG gov.nist.secauto.oscal.lib.profile.resolver.ProfileResolver.resolveImport(ProfileResolver.java:322) - resolving profile import 'issue106-catalog.xml'
10:39:58.540 [main] ERROR gov.nist.secauto.metaschema.model.common.constraint.LoggingConstraintValidationHandler.logConstraint(LoggingConstraintValidationHandler.java:121) - ERROR: (/catalog/control[1]) Expect constraint 'prop[@name='status']/@value=('withdrawn','Withdrawn') or part[@name='statement']' did not match the data at path '/catalog/control[1]'
10:39:58.543 [main] ERROR gov.nist.secauto.metaschema.model.common.constraint.LoggingConstraintValidationHandler.logConstraint(LoggingConstraintValidationHandler.java:121) - ERROR: (/catalog/control[1]/control[1]) Expect constraint 'prop[@name='status']/@value=('withdrawn','Withdrawn') or part[@name='statement']' did not match the data at path '/catalog/control[1]/control[1]'
10:43:53.065 [main] DEBUG gov.nist.secauto.oscal.lib.profile.resolver.selection.DefaultResult.promoteControl(DefaultResult.java:103) - promoting control 'c1.1'
10:45:05.843 [main] DEBUG gov.nist.secauto.oscal.lib.profile.resolver.selection.DefaultResult.removeControl(DefaultResult.java:199) - Requesting removal of control 'c1'.
10:53:58.002 [main] DEBUG gov.nist.secauto.oscal.lib.profile.resolver.support.BasicIndexer.removeItem(BasicIndexer.java:246) - Removing CONTROL 'c1' from index.
10:55:28.634 [main] DEBUG gov.nist.secauto.oscal.lib.profile.resolver.ProfileResolver.structureFlat(ProfileResolver.java:462) - applying flat structuring directive
10:57:38.239 [main] DEBUG gov.nist.secauto.oscal.lib.profile.resolver.selection.DefaultResult.promoteControl(DefaultResult.java:103) - promoting control 'c1.1'
10:59:44.536 [main] WARN  gov.nist.secauto.oscal.lib.profile.resolver.support.BasicIndexer.addItem(BasicIndexer.java:218) - Duplicate control found with identifier c1.1 in index.

I cannot seem to track that down and I am not sure given the warning type on the log record if it is serious, but since the document should be reindex at this point, I am left scratching my head. Let me know.

@aj-stein-nist aj-stein-nist marked this pull request as ready for review February 1, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@david-waltermire david-waltermire Awaiting requested review from david-waltermire

Assignees

@aj-stein-nist aj-stein-nist

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NPE when resolving profile selecting catalog children controls without parent

1 participant

@aj-stein-nist