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 whenever there is nothing to deploy — whether | ||
| // that's every component disabled or a zero-componentRef recipe. An | ||
| // SDK caller that loops over the result and deploys would otherwise | ||
| // silently no-op on either input. | ||
| if len(r.Components) == 0 { | ||
| 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 |
|---|---|---|
|
|
@@ -367,6 +367,60 @@ func TestBundleComponents_HelmComponentLoadsManifestFiles(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestBundleComponents_RejectsAllDisabled locks in that an all-disabled | ||
| // recipe is rejected the same way DefaultBundler.Make rejects it | ||
| // (bundler.go), rather than silently returning an empty, successful | ||
| // bundle list. See #1917. | ||
| func TestBundleComponents_RejectsAllDisabled(t *testing.T) { | ||
|
Member
There was a problem hiding this comment. 🔵 Nitpick — Fold the two new all-disabled / zero-ref tests into one table-driven test
Blast radius: Test readability only — both tests pass and are correct as written. Fix: Optional: one table with
Contributor
There was a problem hiding this comment.
🧠 Learnings used |
||
| t.Parallel() | ||
|
|
||
| client := newClientForBundleTest(t) | ||
| r := newRecipeResultForBundleTest(client, | ||
| []recipe.ComponentRef{ | ||
| { | ||
| Name: "disabled-a", | ||
| Type: recipe.ComponentTypeHelm, | ||
| Overrides: map[string]any{"enabled": false}, | ||
| }, | ||
| }, | ||
| nil, // facade Components is empty because the only ref is disabled | ||
| ) | ||
| _, err := client.BundleComponents(context.Background(), r) | ||
| if err == nil { | ||
| t.Fatal("expected error for all-disabled recipe, got nil") | ||
| } | ||
| var se *aicrerrors.StructuredError | ||
| if !stderrors.As(err, &se) { | ||
| t.Fatalf("expected *aicrerrors.StructuredError, got %T: %v", err, err) | ||
| } | ||
| if se.Code != aicrerrors.ErrCodeInvalidRequest { | ||
| t.Errorf("expected ErrCodeInvalidRequest, got %s", se.Code) | ||
| } | ||
| } | ||
|
|
||
| // TestBundleComponents_RejectsZeroComponentRefs locks in that a recipe | ||
| // with no componentRefs at all is rejected the same way as an all- | ||
| // disabled recipe — the zero-ref divergence from DefaultBundler.Make | ||
| // that the dropped r.internal.ComponentRefs conjunct used to miss. | ||
| // See #1917. | ||
| func TestBundleComponents_RejectsZeroComponentRefs(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| client := newClientForBundleTest(t) | ||
| r := newRecipeResultForBundleTest(client, nil, nil) | ||
| _, err := client.BundleComponents(context.Background(), r) | ||
| if err == nil { | ||
| t.Fatal("expected error for zero-componentRef recipe, got nil") | ||
| } | ||
| var se *aicrerrors.StructuredError | ||
| if !stderrors.As(err, &se) { | ||
| t.Fatalf("expected *aicrerrors.StructuredError, got %T: %v", err, err) | ||
| } | ||
| if se.Code != aicrerrors.ErrCodeInvalidRequest { | ||
| t.Errorf("expected ErrCodeInvalidRequest, got %s", se.Code) | ||
| } | ||
| } | ||
|
Comment on lines
+370
to
+422
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use a table-driven test for the two rejection cases.
As per coding guidelines, Go tests with multiple inputs or outcomes must be table-driven. 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| // TestRecipeResultFromInternal_PlumbsHelmFields locks in that the | ||
| // translation from pkg/recipe.ComponentRef into the facade's | ||
| // ComponentRef carries Source, Chart, and Namespace through. Without | ||
|
|
@@ -1368,3 +1422,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) | ||
| } | ||
| } | ||
| 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