fix(recipe,client): reject duplicate ComponentRef names, omit disable… #1917
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -857,6 +857,9 @@ func facadeResultFromInternal(r *recipe.RecipeResult, name string) *RecipeResult | |
| internal: r, | ||
| } | ||
| for _, c := range r.ComponentRefs { | ||
| if !c.IsEnabled() { | ||
|
Member
There was a problem hiding this comment. 🟡 Minor — Facade To be clear up front, this change is better than advertised: Blast radius: Docs only, but observable: Fix: Add the qualifier to both doc pages — e.g. " |
||
| 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 | ||
|
|
@@ -1055,6 +1058,16 @@ func (c *Client) BundleComponents(ctx context.Context, r *RecipeResult) ([]Compo | |
| return nil, preflightErr | ||
| } | ||
|
|
||
| // Agree with DefaultBundler.Make (bundler.go), which fails closed with | ||
| // ErrCodeInvalidRequest when every component is disabled. Without this, | ||
| // an all-disabled recipe silently returns an empty, successful bundle | ||
| // list here — an SDK caller that loops over the result and deploys | ||
| // would silently no-op instead of getting an actionable error. | ||
| if len(r.Components) == 0 && r.internal != nil && len(r.internal.ComponentRefs) > 0 { | ||
|
Member
There was a problem hiding this comment. 🟡 Minor — All-disabled guard: comment claims parity with The Blast radius: SDK callers of Fix: Guard on On @coderabbitai's version of this finding — it argued the guard should key off
Contributor
There was a problem hiding this comment.
I withdraw my earlier preference for keying the guard off The guard should therefore be simply: if len(r.Components) == 0 {
return nil, errors.New(errors.ErrCodeInvalidRequest,
"recipe has no enabled components")
}This removes the dead ✏️ Learnings added
|
||
| return nil, errors.New(errors.ErrCodeInvalidRequest, | ||
| "recipe has no enabled components") | ||
| } | ||
|
Comment on lines
+1061
to
+1069
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Determine all-disabled status from internal refs.
🤖 Prompt for AI Agents |
||
|
|
||
| bundles := make([]ComponentBundle, 0, len(r.Components)) | ||
| for i := range r.Components { | ||
| // Bail on every iteration so a long recipe doesn't hold | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -534,13 +534,13 @@ func (r *RecipeResult) backfillComponentTypes() error { | |
|
|
||
| // PrepareAndValidate normalizes a RecipeResult's component refs and rejects | ||
| // incoherent ones, in the required order: reject refs named with the reserved | ||
| // deployer override key, reject duplicate non-empty ref names, back-fill missing | ||
| // types on enabled refs from the registry, canonicalize the type casing, then | ||
| // validate | ||
| // coherence (which itself only inspects enabled refs). Boundaries | ||
| // that produce a RecipeResult WITHOUT running ApplyRegistryDefaults — file load | ||
| // (LoadFromFileWithProvider) and external adoption (client adoptRecipe) — call | ||
| // this single method so the four steps cannot drift or be partially applied | ||
| // (e.g. validating before canonicalizing would reject legitimate lowercase | ||
| // types; skipping the back-fill would reject type-less registry refs). The | ||
| // resolve path (finalizeRecipeResult) instead back-fills via ApplyRegistryDefaults | ||
|
|
@@ -563,13 +563,47 @@ 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 | ||
|
Member
There was a problem hiding this comment. 🟡 Minor — Criteria-resolve path still bypasses the duplicate check
Blast radius: Limited. Every deploy path still fails closed — Fix: Follow-up is fine: extract a shared |
||
| // registry back-fill. See validateRefNames for the rationale. | ||
| if err := validateRefNames(r.ComponentRefs); err != nil { | ||
| return err | ||
| } | ||
| if err := r.backfillComponentTypes(); err != nil { | ||
| return err | ||
| } | ||
| canonicalizeComponentTypes(r.ComponentRefs) | ||
| return r.ValidateCoherence() | ||
| } | ||
|
|
||
| // validateRefNames rejects duplicate non-empty component-ref names (enabled | ||
| // or disabled). Component names key value materialization, override | ||
| // routing, validation, filtering, and deployment ordering throughout the | ||
| // bundler/validations/deployer paths, all of which assume a unique | ||
| // name -> ref mapping. AICR-generated recipes never produce duplicates, but | ||
| // a hand-authored or externally-adopted recipe can, and a disabled | ||
| // duplicate must not be allowed to silently mask an enabled ref of the same | ||
| // name (or vice versa) — so ALL refs participate in the check, not just | ||
| // enabled ones. Empty names are exempt: they are never used as a lookup | ||
| // key. Shared between PrepareAndValidate (the file-load, adopt, and | ||
| // bundle/validate -r / POST /v1/bundle boundary) and finalizeRecipeResult | ||
| // (the criteria-resolve boundary), so both fail closed on the same | ||
| // recipes. See #1874. | ||
| func validateRefNames(refs []ComponentRef) error { | ||
| seen := make(map[string]int, len(refs)) | ||
| for i := range refs { | ||
| name := refs[i].Name | ||
| if name == "" { | ||
| continue | ||
| } | ||
| if first, ok := seen[name]; ok { | ||
| return errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("recipe has duplicate component ref name %q (refs %d and %d)", name, first, i)) | ||
| } | ||
| seen[name] = i | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // ValidateCoherence rejects enabled ComponentRefs whose deployment-shape fields | ||
| // are internally inconsistent (see coherenceProblem), aggregating every | ||
| // offender into one ErrCodeInvalidRequest so the author sees all problems at | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Nitpick — Uniqueness description omits the empty-name exemption
Confirming @coderabbitai's finding, and I checked the escape hatch it didn't:
required: [name]on line 1628 is a presence assertion, not non-emptiness ("name": ""satisfies it), andpkg/serverperforms no runtime OpenAPI validation — the spec is documentation-only (its only in-repo consumers are a doc comment inpkg/server/doc.goand the enum-drift test; there's nokin-openapi/libopenapiingo.mod). SovalidateRefNames'name == ""exemption really is reachable and repeated empty names really are accepted.Blast radius: Documentation accuracy only — a nameless enabled ref is already degenerate. Same wording applies to
docs/user/api-reference.md:487-489.Fix: Append "(refs with an empty name are exempt)" to both descriptions — or, better, make an empty
Nameon an enabled ref acoherenceProblem, which makes the docs true as written and closes the case below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 10022
@njhensley, confirmed.validateRefNamesdeliberately exemptsname == "", and the PR objective preserves that behavior. The OpenAPI and user API reference therefore overstate the uniqueness guarantee. I agree the appropriate scope here is to document the exemption rather than change enabled-ref validation.✏️ Learnings added