Skip to content

fix(recipe,client): reject duplicate ComponentRef names, omit disable… #1917

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

Merged
njhensley merged 5 commits into from
Jul 28, 2026
Merged

fix(recipe,client): reject duplicate ComponentRef names, omit disable… #1917

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
70b865d
fix(recipe,client): reject duplicate ComponentRef names, omit disable…
mohityadav8 Jul 25, 2026
486b11a
address review feedback from njhensley on #1917
mohityadav8 Jul 28, 2026
59de9b7
Merge branch 'main' into issue-1874-recipe-sdk-cleanups
njhensley Jul 28, 2026
2109ea2
address remaining review feedback: BundleComponents guard, empty-name…
mohityadav8 Jul 28, 2026
dc5a9d6
Merge branch 'main' into issue-1874-recipe-sdk-cleanups
njhensley Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/client/v1/aicr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,9 @@ func facadeResultFromInternal(r *recipe.RecipeResult, name string) *RecipeResult
internal: r,
}
for _, c := range r.ComponentRefs {
if !c.IsEnabled() {

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — Facade Components narrowing isn't reflected in the integrator docs

To be clear up front, this change is better than advertised: applyRegistryDefaults populates Type on disabled refs too, so pre-PR Client.BundleComponents returned fully-populated Helm bundles for components that must never deploy — diverging from DefaultBundler.Make's filterEnabledComponents. This is a genuine bug fix. The types.go:271 godoc ("lists the deployable components") already supports it, and the BundleComponents "1:1 — same order, same length" promise is intact because bundles derive from the already-filtered r.Components.

Blast radius: Docs only, but observable: recipes/overlays/ocp.yaml disables ~10 componentRefs, so an SDK caller resolving an OCP recipe sees a materially shorter Components slice after upgrade. docs/integrator/go-library.md#L276 and docs/integrator/public-api.md:67 both describe the field with no enabled-only qualifier.

Fix: Add the qualifier to both doc pages — e.g. "Components — enabled (deployable) components only; disabled refs remain visible via Resolved().ComponentRefs" — and point the types.go:271 godoc at Resolved() for the full set.

continue
}
// Project the chart the component actually deploys: a source-only
// Helm ref falls back to the component name (the deployers'
// EffectiveChart rule), so SDK consumers never see an empty chart
Expand Down
28 changes: 28 additions & 0 deletions pkg/client/v1/aicr_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,3 +1368,31 @@ func TestFacadeResultFromInternal_ChartProjection(t *testing.T) {
}
}
}

// TestFacadeResultFromInternal_OmitsDisabledComponents pins the facade
// contract: only deployable (enabled) components appear in the SDK
// facade result. See #1874.
func TestFacadeResultFromInternal_OmitsDisabledComponents(t *testing.T) {
t.Parallel()

internal := &recipe.RecipeResult{
ComponentRefs: []recipe.ComponentRef{
{Name: "enabled-a", Type: recipe.ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
{
Name: "disabled-b",
Type: recipe.ComponentTypeHelm,
Source: "https://charts.example.com",
Version: "1.0.0",
Overrides: map[string]any{"enabled": false},
},
},
}

out := facadeResultFromInternal(internal, "test")
if len(out.Components) != 1 {
t.Fatalf("components = %d, want 1", len(out.Components))
}
if out.Components[0].Name != "enabled-a" {
t.Errorf("got component %q, want enabled-a", out.Components[0].Name)
}
}
62 changes: 62 additions & 0 deletions pkg/recipe/componentref_coherence_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,3 +304,65 @@ func TestPrepareAndValidate_RejectsReservedDeployerName(t *testing.T) {
})
}
}

func TestPrepareAndValidate_RejectsDuplicateNames(t *testing.T) {

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — Worth one more edge case: reserved-key-vs-duplicate precedence

Two refs both named deployer hit the reserved-key loop first (metadata.go:560) and surface the "reserved" message, never the "duplicate" one. That precedence is deliberate and nothing currently pins it — and since neither the code nor the message is asserted today, a precedence inversion would be invisible.

Blast radius: Test completeness only.

Fix: Add the precedence case. (Three-way duplicates exercise no new path, and whitespace-padded names are treated as distinct — consistent with the total absence of name normalization anywhere in the codebase — so both of those are skippable.)

tests := []struct {
name string
refs []ComponentRef
wantErr bool
}{
{

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — Positional struct literals where every sibling table in the file uses keyed fields

TestPrepareAndValidate_RejectsReservedDeployerName twenty lines above uses name: / refs: / wantErr:, as does TestComponentRefCoherenceProblem earlier in the file.

Blast radius: Readability only. Keyed literals survive a field insertion into the anonymous struct without silently reassigning values, which is why the file already prefers them.

Fix: Add the name: / refs: / wantErr: keys.

"adjacent duplicate",
[]ComponentRef{
{Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
{Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
},
true,
},
{
"non-adjacent duplicate",
[]ComponentRef{
{Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
{Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
{Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
},
true,
},
{
"disabled duplicate still rejected",
[]ComponentRef{
{Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
{
Name: "a",
Overrides: map[string]any{"enabled": false},
},
},
true,
},
{
"empty names not flagged",

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — "empty names not flagged" uses disabled refs, so it doesn't pin what it's named for

Both refs are disabled, so backfillComponentTypes and ValidateCoherence skip them entirely (each guards on IsEnabled()). The case therefore passes even if the name == "" exemption were deleted — it passes for a reason unrelated to the exemption it's named after.

Blast radius: Test only. The configuration that actually reaches the deployers is two enabled nameless refs, and that has no coverage.

Fix: Swap the fixture to two enabled refs with Type: ComponentTypeHelm + Source + Version (the shape that clears coherence), or add that variant alongside. Makes the intended behavior explicit either way you decide the empty-name question above.

[]ComponentRef{
{Name: "", Overrides: map[string]any{"enabled": false}},
{Name: "", Overrides: map[string]any{"enabled": false}},
},
false,
},
{
"unique names ok",
[]ComponentRef{
{Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
{Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"},
},
false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := &RecipeResult{ComponentRefs: tt.refs}
err := r.PrepareAndValidate()
if (err != nil) != tt.wantErr {

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — Test asserts only err != nil, not the error code or message

The check is currently pinned — mutation-tested via go test -overlay with the production block stripped, and all three wantErr:true subtests fail. So this is drift risk, not a present false pass: PrepareAndValidate has four rejection paths (reserved key, back-fill failure, canonicalization, coherence), so a future fixture tweak making one ref merely incoherent would keep this green while the duplicate rule silently rots.

Blast radius: Test only. The sibling TestPrepareAndValidate_RejectsReservedDeployerName fifteen lines up already models the fix, asserting both the code and the message.

Fix: Behind the existing wantErr branch, add stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) and strings.Contains(err.Error(), "duplicate"). Two lines.

t.Fatalf("PrepareAndValidate() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
14 changes: 14 additions & 0 deletions pkg/recipe/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,20 @@ func (r *RecipeResult) PrepareAndValidate() error {
fmt.Sprintf("recipe component %q uses the reserved deployer override key as its name; %q is reserved for --set deployer:<key> Argo deployer options", r.ComponentRefs[i].Name, ReservedDeployerKey))
}
}
// Reject duplicate component-ref names (enabled or disabled) BEFORE

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — Criteria-resolve path still bypasses the duplicate check

finalizeRecipeResult calls ValidateCoherence directly and never PrepareAndValidate (metadata_store.go#L977). With an external --data base carrying duplicate refs and no matching overlay, initBaseMergedSpec plain-copies them and the duplicates reach aicr recipe -o, aicr query, and Client.ResolveRecipe.

Blast radius: Limited. Every deploy path still fails closed — bundler.Make calls PrepareAndValidate at bundler.go:237 — so the worst case is an emitted recipe that a later aicr bundle -r rejects. #1874's acceptance criteria asked for exactly what shipped, and the #1656 reserved-name check has the identical gap, so this is a scope boundary rather than a miss.

Fix: Follow-up is fine: extract a shared validateRefNames() and call it from finalizeRecipeResult alongside ValidateCoherence, closing both gaps at once. Worth a code comment either way — RecipeMetadataSpec.Merge silently last-wins-collapses same-name refs via componentMap, so when an overlay does merge, the duplicate vanishes without warning.

// registry back-fill. See #1874.
seen := make(map[string]bool, len(r.ComponentRefs))
for i := range r.ComponentRefs {
name := r.ComponentRefs[i].Name
if name == "" {

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — Empty-name exemption leaves the same collision class open (pre-existing)

Nothing rejects an empty Name — coherenceProblem never inspects it. An enabled nameless ref needs an explicit type: Helm + source + version to clear coherence (an unset type is caught by the default: branch), but if it does, two of them collide deployer-dependently: SafeJoin(outputDir, "") resolves to the base dir itself (helpers.go:56) so flux clobbers one helmrelease.yaml, while argocd's IsSafePathComponent("") rejects.

Blast radius: Entirely pre-existing — two empty-named refs collided identically before this PR — and outside #1874's scope, so not a defect this PR owns. Noted only because the PR is already adding a name-validity loop.

Fix: Optional: make the exemption an outright rejection (return errors.New(errors.ErrCodeInvalidRequest, "component ref has an empty name")), which also aligns Go with api/aicr/v1/server.yaml:1628's required: [name].

continue
}
if seen[name] {
return errors.New(errors.ErrCodeInvalidRequest,
fmt.Sprintf("recipe has duplicate component ref name %q", name))

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — Error names the duplicate but not the colliding positions

Fail-fast is right here — it matches the reserved-key loop immediately above, and the split between cheap pre-flight name gates and ValidateCoherence's aggregating shape pass is deliberate, not accidental. But the message identifies only the name.

Blast radius: On a 20-component machine-generated recipe the author can't tell which two entries collided or which one is the stray.

Fix: Make seen a map[string]int and emit fmt.Sprintf("recipe has duplicate component ref name %q (refs %d and %d)", name, seen[name], i). Costs nothing.

}
seen[name] = true
}
if err := r.backfillComponentTypes(); err != nil {
return err
}
Expand Down