fix: update test for metaschema-java PR #529 compatibility

[david-waltermire]

Summary

• Replace removed XMLBeans-based classes with binding-based alternatives in AllowedValueCollectingNodeItemVisitorTest
• Migrate from XmlMetaConstraintLoader to BindingConstraintLoader
• Migrate from ModuleLoader to IBindingModuleLoader
• Migrate from IXmlMetaschemaModule to IBindingMetaschemaModule

This change is required because metaschema-java PR #529 removed the gov.nist.secauto.metaschema.core.model.xml package, requiring migration to the databind module's binding-based loaders.

Test plan

• mvn test passes (47 tests run, 0 failures)
• Build compiles without errors

Summary by CodeRabbit

• Refactor

  • Switched constraint-loading from the previous XML-based mechanism to a new binding-based approach, simplifying public APIs and updating tests to the new workflow.
  • Adjusted dynamic binding output locations under target/dynamic-bindings.

• Chores

  • Updated a submodule reference to a newer commit (no functional behavior changes).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces XML-based constraint and metaschema module loading with binding-based APIs: tests updated to use BindingConstraintLoader, IBindingContext, and IBindingModuleLoader; deleted XML loader/module classes and a utility; dynamic test binding directories moved under target/dynamic-bindings.

Changes

Cohort / File(s)	Change Summary
Test migration to binding APIs src/test/java/gov/nist/secauto/oscal/lib/model/util/AllowedValueCollectingNodeItemVisitorTest.java	Replaced XML-based constraint/module loading with BindingConstraintLoader, IBindingContext, and IBindingModuleLoader; adjusted dynamic binding dirs to target/dynamic-bindings/allowed-values-1 and .../allowed-values-2; updated imports and setup.
Removed XML-based loader/module classes & util gov/nist/secauto/metaschema/core/model/constraint/ExternalConstraintsModulePostProcessor, gov/nist/secauto/metaschema/core/model/xml/XmlMetaConstraintLoader, gov/nist/secauto/metaschema/core/model/xml/ModuleLoader, gov/nistsecauto/metaschema/core/model/xml/IXmlMetaschemaModule, gov/nist/secauto/metaschema/core/util/CollectionUtil	Deleted public classes/interfaces and a utility related to XML-based constraint and metaschema module loading.
Added binding-based constraint loader gov/nist/secauto/metaschema/databind/model/metaschema/BindingConstraintLoader	Introduced new BindingConstraintLoader to load IConstraintSet via an IBindingContext.
Submodule pin update oscal (submodule reference)	Updated subproject commit pointer from e580ba3... to 909b7558... without functional code changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Verify AllowedValueCollectingNodeItemVisitorTest correctly constructs and configures IBindingContext compile/load paths and that BindingConstraintLoader yields the expected IConstraintSet.
• Search for any remaining references to removed XML-based classes or the deleted utility.
• Review BindingConstraintLoader for error handling, edge cases, and interactions with IBindingModuleLoader/IBindingMetaschemaModule.

Poem

🐰 I nibble bindings where XML once grew,

hopping through folders, fresh paths anew.
Constraints now follow the context's tune,
under the moon of target/dynamic-bindings soon.
A little rabbit cheers this tidy view.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main objective: updating tests for compatibility with metaschema-java PR #529 which removed XML-based classes.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9aa4ad1 and 79f9449.

📒 Files selected for processing (1)

• oscal (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• oscal

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Code
• GitHub Check: Website

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/test/java/gov/nist/secauto/oscal/lib/model/util/AllowedValueCollectingNodeItemVisitorTest.java (1)

56-69: Consider deduplicating binding/constraint setup between the two tests

testAllowedValuesMissingVariableBindings2 repeats most of the context and constraint loading logic from the first test, differing mainly in variable names and the generated directory (allowed-values-2). This is fine functionally, but you could optionally extract a small helper (e.g., to load the constraint set or to build a binding context given genDir) to reduce duplication and keep future changes to the binding setup in one place.

If you choose to refactor, re-run both tests to ensure the shared helper still configures constraintSet and compilePath correctly for each directory.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a685c6 and 0c30118.

📒 Files selected for processing (1)

• src/test/java/gov/nist/secauto/oscal/lib/model/util/AllowedValueCollectingNodeItemVisitorTest.java (3 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Code
• GitHub Check: Website

🔇 Additional comments (2)

src/test/java/gov/nist/secauto/oscal/lib/model/util/AllowedValueCollectingNodeItemVisitorTest.java (2)

14-17: BindingConstraintLoader import and binding-based types look consistent with API migration

The new BindingConstraintLoader import, together with IBindingContext, IBindingMetaschemaModule, and IBindingModuleLoader, matches the intended migration away from XML-based loaders; nothing problematic stands out here.

Please double-check that the metaschema databind dependency version in your build contains BindingConstraintLoader and these binding interfaces.

---

32-47: First test’s binding-context and constraint loading flow looks correct

Creating an initial IBindingContext, loading List<IConstraintSet> via BindingConstraintLoader, then building a new context with .constraintSet(constraintSet) and .compilePath(genDir) before using newModuleLoader() aligns well with the binding-based workflow and should preserve the original test intent.

Please run the updated test against the target metaschema-java/databind version to confirm that constraint resolution and module loading behave as expected with the new binding-based APIs.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/test/java/gov/nist/secauto/oscal/lib/model/util/AllowedValueCollectingNodeItemVisitorTest.java (1)

32-45: Constraint loading via BindingConstraintLoader looks correct; consider de‑duplicating setup

Both tests now build an initial IBindingContext, load constraints through BindingConstraintLoader, then construct a second context with .constraintSet(constraintSet) and .compilePath(genDir) before creating the IBindingModuleLoader. That pattern is consistent and should preserve the prior behavior with the new binding-based APIs.

Since the constraint-loading block (paths, loader construction, and load(...) call) is effectively identical in both tests, you might optionally extract it into a small private helper (e.g., loadConstraintSet() taking the resource path) to keep the test setup DRY and make future path or API changes less error-prone.

Also applies to: 56-60

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c30118 and 9aa4ad1.

📒 Files selected for processing (1)

• src/test/java/gov/nist/secauto/oscal/lib/model/util/AllowedValueCollectingNodeItemVisitorTest.java (3 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Code
• GitHub Check: Website

🔇 Additional comments (2)

src/test/java/gov/nist/secauto/oscal/lib/model/util/AllowedValueCollectingNodeItemVisitorTest.java (2)

14-14: BindingConstraintLoader import aligns with new binding-based constraint flow

Importing BindingConstraintLoader here correctly switches the test over to the binding-based constraint loading APIs and removes reliance on the old XMLBeans-based classes. No issues from a test-usage perspective.

---

62-62: Separate dynamic-binding output directory per test is reasonable

Switching the second test to use target/dynamic-bindings/allowed-values-2 keeps its generated bindings isolated from the first test’s allowed-values-1 directory, which helps avoid cross-test interference while still living under target/ so mvn clean will clear them. This looks like a sound choice.