refactor: introduce IControlSelection/IControlMatching common interfaces#281
Conversation
OSCAL's select-control-by-id and matching assemblies have the same shape in both the profile and mapping-common modules, and the generator consolidates them into a single ProfileMatching/ProfileSelectControlById class. Introduce common interfaces so consumer code can use a neutral type instead of profile-specific names: - IControlSelection: getWithChildControls, getWithIds, getMatching - IControlMatching: getPattern - IProfileSelectControlById now extends IControlSelection with a covariant getMatching() returning List<ProfileMatching> - AbstractProfileMatching implements IControlMatching, wired to the matching binding via extend-base-class
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds common control-selection abstractions ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/dev/metaschema/oscal/lib/model/control/IControlSelection.java (1)
14-16: Clarify nullability/emptiness contract for collection accessors.Line [16] is consumed with defensive null handling in
DefaultControlSelectionFilter(Line [149]). Consider explicitly documenting whether these methods can returnnullvs empty lists (especially Line [14] and Line [16]) to make the interface contract unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/metaschema/oscal/lib/model/control/IControlSelection.java` around lines 14 - 16, Document and enforce a clear nullability contract for IControlSelection collection accessors: update the interface method declarations for getWithIds() and getMatching() to specify (via Javadoc and/or annotations such as `@NonNull/`@Nullable) whether they may return null or must return an empty list when absent, and update implementations (including DefaultControlSelectionFilter) to comply — preferably make them non-null by returning Collections.emptyList() when there are no elements and annotate Javadoc/@NonNull on getWithIds() and getMatching() so callers know they can avoid defensive null checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/dev/metaschema/oscal/lib/model/control/IControlSelection.java`:
- Around line 14-16: Document and enforce a clear nullability contract for
IControlSelection collection accessors: update the interface method declarations
for getWithIds() and getMatching() to specify (via Javadoc and/or annotations
such as `@NonNull/`@Nullable) whether they may return null or must return an empty
list when absent, and update implementations (including
DefaultControlSelectionFilter) to comply — preferably make them non-null by
returning Collections.emptyList() when there are no elements and annotate
Javadoc/@NonNull on getWithIds() and getMatching() so callers know they can
avoid defensive null checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6aea5c10-67e9-4b96-aeb0-517243ea5d75
📒 Files selected for processing (5)
src/main/java/dev/metaschema/oscal/lib/model/control/IControlMatching.javasrc/main/java/dev/metaschema/oscal/lib/model/control/IControlSelection.javasrc/main/java/dev/metaschema/oscal/lib/model/control/profile/AbstractProfileMatching.javasrc/main/java/dev/metaschema/oscal/lib/model/control/profile/IProfileSelectControlById.javasrc/main/metaschema-bindings/oscal-metaschema-bindings.xml
Rename the generated select-control-by-id classes in the binding config to reflect which OSCAL module each originates from, making the profile/mapping vs assessment distinction explicit: - ProfileSelectControlById -> ControlCommonSelectControlById (used by profile's InsertControls and mapping's GapSummary; generated from oscal_control-common_metaschema.xml) - SelectControlById -> AssessmentCommonSelectControlById (used by assessment-plan/assessment-results' ReviewedControls; generated from oscal_assessment-common_metaschema.xml, added as a new binding) Rename the hand-written supporting types in lockstep: - IProfileSelectControlById -> IControlCommonSelectControlById - AbstractProfileSelectControlById -> AbstractControlCommonSelectControlById Consumer references updated in IControlFilter, DefaultControlSelectionFilter, and their tests.
IControlCommonSelectControlById and AbstractControlCommonSelectControlById are no longer profile-specific (they cover control-common-defined types shared between profile and mapping-common). Move them up from dev.metaschema.oscal.lib.model.control.profile to dev.metaschema.oscal.lib.model.control so the package placement matches the naming. Updates the binding's extend-base-class reference and all consumer imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/metaschema-bindings/oscal-metaschema-bindings.xml`:
- Around line 162-169: The select-control-by-id metaschema binding must extend a
base class so the generated AssessmentCommonSelectControlById implements the
module-agnostic IControlSelection; update the <define-assembly-binding
name="select-control-by-id"> for the assessment metaschema to include an
extend-base-class that references AbstractAssessmentCommonSelectControlById
(mirroring the control-common pattern where
AbstractControlCommonSelectControlById implements
IControlCommonSelectControlById which extends IControlSelection), ensuring
AssessmentCommonSelectControlById implements the expected interface and
preserves polymorphic usage across modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 821a8f13-25d0-470d-abf0-76c346bb0876
📒 Files selected for processing (7)
src/main/java/dev/metaschema/oscal/lib/model/control/profile/AbstractControlCommonSelectControlById.javasrc/main/java/dev/metaschema/oscal/lib/model/control/profile/IControlCommonSelectControlById.javasrc/main/java/dev/metaschema/oscal/lib/profile/resolver/selection/DefaultControlSelectionFilter.javasrc/main/java/dev/metaschema/oscal/lib/profile/resolver/selection/IControlFilter.javasrc/main/metaschema-bindings/oscal-metaschema-bindings.xmlsrc/test/java/dev/metaschema/oscal/lib/profile/resolver/selection/DefaultControlSelectionFilterTest.javasrc/test/java/dev/metaschema/oscal/lib/profile/resolver/selection/ImportTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/dev/metaschema/oscal/lib/profile/resolver/selection/ImportTest.java
| <metaschema-binding | ||
| href="../../../oscal/src/metaschema/oscal_assessment-common_metaschema.xml"> | ||
| <define-assembly-binding name="select-control-by-id"> | ||
| <java> | ||
| <use-class-name>AssessmentCommonSelectControlById</use-class-name> | ||
| </java> | ||
| </define-assembly-binding> | ||
| </metaschema-binding> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's an AbstractAssessmentCommonSelectControlById or similar base class defined
fd -t f -e java -i "selectcontrolbyid" src/main/java | xargs -I {} grep -l "Assessment" {} 2>/dev/null
# Check the OSCAL assessment-common metaschema structure to understand the select-control-by-id definition
rg -n "select-control-by-id" --glob "*.xml" oscal/src/metaschema/ 2>/dev/null | head -20Repository: metaschema-framework/liboscal-java
Length of output: 60
🏁 Script executed:
# First, let's check the structure of the repository
git ls-files | grep -E "(metaschema-bindings|IControlSelection)" | head -20Repository: metaschema-framework/liboscal-java
Length of output: 401
🏁 Script executed:
# Let's read the actual file mentioned in the review to understand the context
wc -l src/main/metaschema-bindings/oscal-metaschema-bindings.xmlRepository: metaschema-framework/liboscal-java
Length of output: 140
🏁 Script executed:
# Read the relevant section and surrounding context
sed -n '150,180p' src/main/metaschema-bindings/oscal-metaschema-bindings.xmlRepository: metaschema-framework/liboscal-java
Length of output: 1330
🏁 Script executed:
# Search for IControlSelection interface
fd -t f -name "*.java" | xargs grep -l "interface IControlSelection" 2>/dev/nullRepository: metaschema-framework/liboscal-java
Length of output: 310
🏁 Script executed:
# Search for control-common binding to compare
grep -n "control-common" src/main/metaschema-bindings/oscal-metaschema-bindings.xml | head -10Repository: metaschema-framework/liboscal-java
Length of output: 156
🏁 Script executed:
# Let's see the control-common binding definition
sed -n '47,120p' src/main/metaschema-bindings/oscal-metaschema-bindings.xmlRepository: metaschema-framework/liboscal-java
Length of output: 2976
🏁 Script executed:
# Read the IControlSelection interface
cat src/main/java/dev/metaschema/oscal/lib/model/control/IControlSelection.javaRepository: metaschema-framework/liboscal-java
Length of output: 381
🏁 Script executed:
# Search for other assessment-related interfaces that might be relevant
fd -t f -name "*.java" -path "*/assessment*" | xargs grep -l "interface.*Assessment.*Select" 2>/dev/nullRepository: metaschema-framework/liboscal-java
Length of output: 310
🏁 Script executed:
# Look for any existing AssessmentCommonSelectControlById references
rg "AssessmentCommonSelectControlById" --type javaRepository: metaschema-framework/liboscal-java
Length of output: 60
🏁 Script executed:
# Check if there's an AbstractControlCommonSelectControlById class
fd -t f -name "*SelectControlById*" src/main/javaRepository: metaschema-framework/liboscal-java
Length of output: 308
🏁 Script executed:
# Find AbstractControlCommonSelectControlById
fd -t f -name "*AbstractControlCommonSelectControlById*" src/main/javaRepository: metaschema-framework/liboscal-java
Length of output: 308
🏁 Script executed:
# Search for the class definition
rg "class AbstractControlCommonSelectControlById" --type java -A 5Repository: metaschema-framework/liboscal-java
Length of output: 912
🏁 Script executed:
# Check if it implements IControlSelection
rg "AbstractControlCommonSelectControlById" --type java -A 10 | head -30Repository: metaschema-framework/liboscal-java
Length of output: 3470
🏁 Script executed:
# Look for any assessment module base classes that might be models
fd -t f -path "*/assessment*" -name "Abstract*.java" src/main/javaRepository: metaschema-framework/liboscal-java
Length of output: 246
🏁 Script executed:
# Search for the actual generated or expected class pattern in assessment module
rg "AssessmentCommon" --type java | head -20Repository: metaschema-framework/liboscal-java
Length of output: 60
🏁 Script executed:
# Find IControlCommonSelectControlById interface
rg "interface IControlCommonSelectControlById" --type-listRepository: metaschema-framework/liboscal-java
Length of output: 467
🏁 Script executed:
# Use correct search for the interface
rg "interface IControlCommonSelectControlById"Repository: metaschema-framework/liboscal-java
Length of output: 244
🏁 Script executed:
# Examine the interface
cat src/main/java/dev/metaschema/oscal/lib/model/control/IControlCommonSelectControlById.javaRepository: metaschema-framework/liboscal-java
Length of output: 414
🏁 Script executed:
# Check if AbstractControlCommonSelectControlById has any relationship with IControlSelection
rg "IControlSelection" src/main/java/dev/metaschema/oscal/lib/model/control/AbstractControlCommonSelectControlById.javaRepository: metaschema-framework/liboscal-java
Length of output: 60
🏁 Script executed:
# Look for what IControlCommonSelectControlById extends or implements
rg -A 3 "interface IControlCommonSelectControlById"Repository: metaschema-framework/liboscal-java
Length of output: 566
🏁 Script executed:
# Check if there are any assessment-related selection classes
rg "assessment.*select" -i src/main/metaschema-bindings/oscal-metaschema-bindings.xmlRepository: metaschema-framework/liboscal-java
Length of output: 148
🏁 Script executed:
# Check if AbstractAssessmentCommonSelectControlById exists
find src/main/java -name "*AssessmentCommon*" -type fRepository: metaschema-framework/liboscal-java
Length of output: 60
🏁 Script executed:
# Search for any assessment module select-control references in code
rg "assessment.*SelectControlById" -iRepository: metaschema-framework/liboscal-java
Length of output: 207
🏁 Script executed:
# Check what other assessment-common bindings look like in the file
rg "oscal_assessment-common" src/main/metaschema-bindings/oscal-metaschema-bindings.xml -A 8Repository: metaschema-framework/liboscal-java
Length of output: 454
🏁 Script executed:
# Look at the full assessment module bindings pattern
grep -A 6 "oscal_assessment" src/main/metaschema-bindings/oscal-metaschema-bindings.xml | head -60Repository: metaschema-framework/liboscal-java
Length of output: 962
Add extend-base-class to AssessmentCommonSelectControlById to implement IControlSelection.
The select-control-by-id binding in oscal_assessment-common_metaschema.xml lacks an extend-base-class, unlike its counterpart in oscal_control-common_metaschema.xml. The control-common binding extends AbstractControlCommonSelectControlById, which implements IControlCommonSelectControlById (which extends IControlSelection), enabling polymorphic usage via the module-agnostic interface. Without a corresponding base class for the assessment module, AssessmentCommonSelectControlById won't implement IControlSelection, breaking consistency with PR objectives for module-agnostic interfaces.
Consider creating AbstractAssessmentCommonSelectControlById as the extend-base-class for this binding to maintain interface consistency across modules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/metaschema-bindings/oscal-metaschema-bindings.xml` around lines 162
- 169, The select-control-by-id metaschema binding must extend a base class so
the generated AssessmentCommonSelectControlById implements the module-agnostic
IControlSelection; update the <define-assembly-binding
name="select-control-by-id"> for the assessment metaschema to include an
extend-base-class that references AbstractAssessmentCommonSelectControlById
(mirroring the control-common pattern where
AbstractControlCommonSelectControlById implements
IControlCommonSelectControlById which extends IControlSelection), ensuring
AssessmentCommonSelectControlById implements the expected interface and
preserves polymorphic usage across modules.
The abstract base class applies to the control-common-defined matching assembly used by both profile and mapping-common, so the profile-specific name was misleading. Rename and move up to dev.metaschema.oscal.lib.model.control to match the IControlMatching interface's package and the sibling AbstractControlCommonSelectControlById placement.
…interface Satisfies the pmd-verify step in the Website CI job: - AbstractControlMatching: add protected no-arg constructor (AbstractClassWithoutAnyMethod) - IControlMatching: add @SuppressWarnings(PMD.ImplicitFunctionalInterface) since the interface is implemented by generated bean classes, not intended as a lambda target
david-waltermire
merged commit 088a7a7
into
metaschema-framework:develop
1 participant
Summary
OSCAL's `select-control-by-id` and `matching` assemblies are defined locally across multiple modules with two distinct shapes:
This PR makes the module relationship explicit in generated class names, introduces common interfaces, and places hand-written types in packages that match their scope.
Changes
New common interfaces in `dev.metaschema.oscal.lib.model.control`:
Generated class renames (in binding config):
Hand-written class renames + moves (lockstep with the generated renames):
Consumer references updated in `IControlFilter`, `DefaultControlSelectionFilter`, and their tests.
Test plan
Summary by CodeRabbit
New Features
Refactor
Tests