Merge strategy refinements: scope-conflict resolution, RenameRef refusal, Squash_RenameByScope (2/5) - #447
Open
jimklimov wants to merge 13 commits into
Open
Merge strategy refinements: scope-conflict resolution, RenameRef refusal, Squash_RenameByScope (2/5)#447jimklimov wants to merge 13 commits into
jimklimov wants to merge 13 commits into
Conversation
…eStrategy Foundational types for a strategy-driven BOM merge engine: a marker interface (IBomEntity) plus IMergeable<T>/IEquivalent<T>, each with a default interface method body that falls back to the type's own IEquatable<T> equality. Most model classes will need nothing more than declaring the interface (zero-body opt-in); only types with real reconciliation logic override the defaults. Compiled for net8.0+/net10.0 only (default interface methods require an ABI this library's netstandard2.0 target cannot provide); netstandard2.0 consumers keep today's merge behavior unchanged. MergeStrategy configures the merge: independent on/off toggles for subset-dependency merging, dependency-as-extra-property treatment, and metadata refresh, plus a ComponentConflictResolution enum (rather than more booleans) so new resolution algorithms can be added as new cases without reshaping this type or any call site that reads it. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
This was referenced Sep 15, 2026
Open
Declares the new interfaces on every model type the merge engine needs to process generically as list elements (Tool, Service, ExternalReference, Dependency, Composition, Vulnerability, Annotation, Standard, Assessor, Attestation, Claim, OrganizationalEntity, Component, plus nested-list element types Hash, Property, OrganizationalContact, LicenseChoice, PatentAssertion). For types that already implement IEquatable<T>, this is a one-line, zero-body change (the interface defaults handle it). Property, OrganizationalContact, LicenseChoice and PatentAssertion had no equality implementation at all; they gain the same JSON-hash-based Equals(object)/ Equals(T)/GetHashCode pattern already used by Component/Annotation/etc. elsewhere in this codebase (all three together, not just Equals(T)/ GetHashCode, to keep the object.Equals/GetHashCode contract consistent for callers that only see the non-generic Equals), so the interface defaults have something real to fall back on. Hash already had Equals(T)/GetHashCode but was missing the same Equals(object) override; added here too since this commit already touches the file. Hash gets a real (non-default) Equivalent/MergeWith: two hashes of the same algorithm should carry the same content; if one side is missing content, fill it in from the other, and treat a genuine content mismatch for the same algorithm as an unmergeable conflict. Component's real merge logic follows in a separate commit. Signed-off-by: Jim Klimov <jimklimov@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Generic-constrained (T : IEquatable<T>, IEquivalent<T>, IMergeable<T>) list merge: exact-equal items dedupe as before, equivalent-but-unequal items attempt MergeWith, everything else is kept as a separate entry. Falls back to the existing exact-match ListMergeHelper<T> when a strategy disables entity merging. One implementation serves every mergeable list field in a Bom (components, services, hashes, external references, authors, ...) via real interface dispatch -- no reflection, no per-type branching. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
Implements Component's merge logic as explicit per-field code instead of a PropertyInfo/switch(Type) walk: Equivalent() checks type/name required-equal, version/group/purl equal-if-both-present (bom-ref deliberately excluded -- that's the merge orchestration's concern, not a per-field check). MergeWith() folds scalar fields via ??, list fields via the new MergeableListHelper.Merge (lives in CycloneDX.Core rather than CycloneDX.Utils so Component can call it directly -- Utils depends on Core, not the reverse), and Scope via TryMergeScope, which handles the both-Excluded case explicitly: two components that are both Excluded-scope but differ in some unrelated field should still merge, not be treated as an unresolvable scope conflict just because neither side is Optional. MergeableListHelper also gains small single-value/string-list/ nullable-bool merge helpers used by Component's scalar and simple-list fields. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
BomRefWalker.RewriteRefs(bom, rewrite) generalizes the per-type ref-
rewriting CycloneDXUtils.HierarchicalMerge already hand-rolls for
bom-ref namespacing (NamespaceComponentBomRefs, NamespaceDependencyBomRefs,
NamespaceCompositions, NamespaceVulnerabilitiesRefs,
NamespaceAnnotationsBomRefs) into one reusable entry point parameterized
on an arbitrary rewrite function instead of a fixed namespace prefix.
Namespacing becomes RewriteRefs(bom, r => $"{ns}:{r}"); a single manual
rename becomes RewriteRefs(bom, r => r == oldRef ? newRef : r).
Covers Metadata.Component, Components, Services, Dependencies,
Compositions, Vulnerabilities, and Annotations; the newer (CycloneDX
1.6) Declarations/Definitions sections are not yet walked -- a
mechanical follow-up, not an architectural one.
Bom.RenameRef builds on the walker to rename a bom-ref and every
back-reference to it throughout a document in one pass.
BomMetadataReferThisToolkit/BomMetadataUpdate stamp this library's
(and the entry assembly's) Tool reference into Metadata.Tools, and
refresh Version/SerialNumber/Timestamp.
Signed-off-by: Jim Klimov <jimklimov@gmail.com>
New FlatMerge(bom1, bom2, MergeStrategy) and HierarchicalMerge(boms, bomSubject, MergeStrategy) overloads sit alongside the existing fixed-behavior ones (which are untouched and keep today's exact behavior for existing callers). FlatMerge's overload swaps every ListMergeHelper<T> call for MergeableListHelper.Merge(..., strategy), so Components/Services/Tools/Hashes/etc. attempt real reconciliation (Component's Scope-aware squash, Hash's content fill-in) instead of only deduping exact matches, and applies RenameConflictingComponents (pre-merge bom-ref collision detection via BomRefWalker) and the metadata-update toggles. Known gaps, called out rather than silently dropped: MergeSubsetDependencies isn't wired to real subset-detection logic yet (Dependency still merges via its IMergeable<T> default), and HierarchicalMerge's strategy overload only adds the metadata-update toggles so far (namespacing already avoids the collisions FlatMerge's squash logic exists to resolve, so it needed less new behavior here). Signed-off-by: Jim Klimov <jimklimov@gmail.com>
Unit tests (not full-BOM snapshots, for faster feedback) covering: Component.Equivalent's type/name/version-if-both-present rules and its deliberate bom-ref exclusion; Component.MergeWith's scope squash, excluded-vs-required conflict refusal, and the both-Excluded case (two components that are both Excluded-scope but differ in some unrelated field still merge); Hash.MergeWith's content fill-in/mismatch cases; FlatMerge(bom1, bom2, strategy) actually squashing equivalent components across two BOMs; and Bom.RenameRef rewriting both an identifier and its back-reference (plus the not-found no-op case). Signed-off-by: Jim Klimov <jimklimov@gmail.com>
The earlier commit only added the 2-BOM FlatMerge(bom1, bom2, strategy) overload; the CLI's merge command (and anyone merging more than two documents) needs the IEnumerable<Bom> form too. Also fixes a now- ambiguous overload resolution: the existing single-arg FlatMerge(boms) delegated via `FlatMerge(boms, null)`, which became ambiguous between the Component and MergeStrategy overloads once the latter existed -- disambiguated with an explicit (Component)null cast. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
… enum MergeStrategy.Default()'s initial choice for resolving a Required-vs- Optional Scope conflict picked the narrower Optional reading. On reflection that is the wrong default: the specification says an absent/ambiguous Scope SHOULD be treated as required, so silently downgrading a Required dependency to Optional risks under-reporting it in downstream tooling (e.g. vulnerability scanners that key off Scope). Flips MergeStrategy.Default() to Squash_UpgradeScope and renames the enum values to contrast directly: Squash -> Squash_DowngradeScope, SquashUpgradeScope -> Squash_UpgradeScope. Also reserves Squash_RenameByScope (replacing the placeholder RenameByScope) and wires TryMergeScope to refuse merging differently-scoped components under it, ahead of implementing the actual scope-partitioning pass in a follow-up commit. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
RenameRef previously rewrote every occurrence of oldRef to newRef unconditionally -- if newRef already identified (or was referenced by) some other entity in the document, the rename would silently make two different entities share one bom-ref, or repoint an existing back-reference at the wrong entity. Now does a read-only collection pass first (reusing BomRefWalker's traversal, so "what counts as a ref" can't drift between the check and the real rewrite) and throws InvalidOperationException if newRef is already in use, instead of performing the corrupting rename. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
Independent of the RenameRef collision fix (different code path: this is about the generic Components/Dependencies list merge, not the rename-entity walker) -- addresses the gap where two BOMs describing the same component with different direct-dependency lists (e.g. a Maven module built standalone vs. as part of a parent build) would each contribute their own <dependency ref="X"> entry, since Dependency previously only merged via IMergeable<T>'s exact-equality default. Equivalent() matches on Ref alone. MergeWith() unions the two Dependencies sub-lists (via the same MergeableListHelper.Merge used everywhere else, so it recurses correctly into nested dependency trees), gated by MergeStrategy.MergeSubsetDependencies: when that's false, a genuine difference between the two dependsOn sets is treated as a real conflict (refuse, keep both entries as before) rather than silently unioned. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
… bom-ref When two Equivalent components (same type/name/version-if-present) differ in Scope, they are kept as two distinct entries instead of squashed or refused, suffixed :scope=<value> (e.g. "lp:scope=Required" / "lp:scope=Excluded"), with every back-reference in the document rewritten to match (via BomRefWalker, per-source-bom, before the generic Components merge runs). No suffix is ever added unless a real conflict appears: if every source agrees on Scope for a given identity, the original bom-ref is kept as-is (ApplyRenameByScope only mutates on an actual scope mismatch). A retroactive rename handles the first-ever split (the already-accumulated entry, and any of its back-references already recorded from earlier merged BOMs, get suffixed too, not just the new arrival); a third+ incoming copy matching an existing scope partition squashes into it independently of the others. TryMergeScope now refuses to merge differing scopes at all under this resolution (partitioning happens via bom-ref splitting instead, not field-level squashing). RenameBomRefCollisions (the existing same- bomref-but-unequal check for RenameConflictingComponents) now skips pairs that are Equivalent to each other when this resolution is active, so it doesn't rename the pair with a blunt ":2" suffix before ApplyRenameByScope gets a chance to do it precisely. Fixes a stale-closure bug caught by a test during development: the rewrite lambda in ApplyRenameByScope compared against `incoming.BomRef` read live from the (mutating) property instead of a captured snapshot, so a component's own bom-ref rename would silently poison the comparison used for its own back-references later in the same walk pass. Signed-off-by: Jim Klimov <jimklimov@gmail.com>
jimklimov
force-pushed
the
merge-strategy-refinements
branch
from
September 17, 2026 09:28
f057cd0 to
f7c0583
Compare
The comment above result.Dependencies still claimed reconciliation beyond exact-match was 'not yet implemented', left over from before this same PR's 'Dependency: real Equivalent/MergeWith for subset- dependency merging' commit gave Dependency real MergeWith logic. Update the comment to describe what actually happens now. Signed-off-by: Jim Klimov <jimklimov@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Stacked on #446 (merge-strategy-core). Review-round refinements to the same merge engine:
MergeStrategy: renames/flips the default component scope-conflict resolution toSquash_UpgradeScope.Bom.RenameRef: refuses rather than silently colliding when the target bom-ref is already in use.Dependency: realEquivalent()/MergeWith()for subset-dependency merging.Squash_RenameByScope: splits scope-conflicting components apart by bom-ref instead of merging theminto one (with tests).
Why
Part of the same #245 replacement as #446 — see that PR for the overall rationale. This is PR 2 of 5;
#446 is PR 1. Follow-up PRs (
Merge.csmetadata/empty-list cleanup,AttachDanglingComponents, arecursive empty-list pruner) are stacked on top of this one.