From d1720666892fc5377ecbdfbdd2106b2cda98d1fc Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 23 Jul 2026 12:34:56 +0530 Subject: [PATCH 1/3] feat(reconcile): validate preference values against the trait at plan time A file entry could set a value the server rejects (an empty value, or a value outside a checkbox or select trait's options). It passed validation and showed a set in the plan, then failed at apply, breaking rule 2 (a plan never contains a change that cannot apply). The diff now rebuilds each trait from the describe-preferences response and runs its own validator over the desired value, and over a trait default before a reset, so an unappliable change fails the plan with a clear message. Reuses core/preference validators, so plan-time and server-side checks cannot drift. --- internal/reconcile/preference.go | 33 +++++++--- internal/reconcile/preference_reconciler.go | 64 +++++++++++++++---- .../reconcile/preference_reconciler_test.go | 10 +++ internal/reconcile/preference_test.go | 61 +++++++++++++----- 4 files changed, 134 insertions(+), 34 deletions(-) diff --git a/internal/reconcile/preference.go b/internal/reconcile/preference.go index 9fc79a89c..3187bf253 100644 --- a/internal/reconcile/preference.go +++ b/internal/reconcile/preference.go @@ -4,6 +4,8 @@ import ( "fmt" "sort" "strings" + + "github.com/raystack/frontier/core/preference" ) // KindPreference is the desired-state document kind for platform preferences. @@ -40,9 +42,10 @@ func (o preferenceOp) String() string { // diffPreferences returns the ops that make the current platform preferences // match the desired spec. The file is the full desired state: a preference the // file lists is set to its value, and a preference the file leaves out is reset -// to its trait default. defaults holds every valid platform trait and its -// default, so it is both the source of defaults and the set of known names. -func diffPreferences(desired []PreferenceSpec, current, defaults map[string]string) ([]preferenceOp, error) { +// to its trait default. traits holds every valid platform trait, so it is the +// source of defaults, the set of known names, and the validator each value must +// pass. +func diffPreferences(desired []PreferenceSpec, current map[string]string, traits map[string]preference.Trait) ([]preferenceOp, error) { desiredByName := make(map[string]string, len(desired)) for _, s := range desired { if strings.TrimSpace(s.Name) == "" { @@ -51,9 +54,17 @@ func diffPreferences(desired []PreferenceSpec, current, defaults map[string]stri if _, dup := desiredByName[s.Name]; dup { return nil, fmt.Errorf("preference %q is listed more than once", s.Name) } - if _, known := defaults[s.Name]; !known { + trait, known := traits[s.Name] + if !known { return nil, fmt.Errorf("unknown platform preference %q", s.Name) } + // The server validates a value against its trait and rejects one it does + // not accept: an empty value, or a value outside a checkbox or select + // trait's options. Check it here so an unappliable set fails the plan with + // a clear message instead of failing late at the API. + if !trait.GetValidator().Validate(s.Value) { + return nil, fmt.Errorf("preference %q: %q is not a valid value for its trait", s.Name, s.Value) + } desiredByName[s.Name] = s.Value } @@ -65,7 +76,7 @@ func diffPreferences(desired []PreferenceSpec, current, defaults map[string]stri if v, ok := current[name]; ok && v != "" { return v } - return defaults[name] + return traits[name].Default } var sets, resets []preferenceOp @@ -75,7 +86,7 @@ func diffPreferences(desired []PreferenceSpec, current, defaults map[string]stri } } for name, value := range current { - def, known := defaults[name] + trait, known := traits[name] if !known { // A stored value whose trait no longer exists: leave it alone. The // file cannot name it (unknown names fail), so nothing manages it. @@ -84,8 +95,14 @@ func diffPreferences(desired []PreferenceSpec, current, defaults map[string]stri if _, listed := desiredByName[name]; listed { continue } - if value != def { - resets = append(resets, preferenceOp{action: opReset, name: name, value: def}) + if value != trait.Default { + // A reset writes the trait default back. If the trait would reject its + // own default, the default is not reachable and the reset can never + // apply, so fail the plan rather than emit an op that dies at the API. + if !trait.GetValidator().Validate(trait.Default) { + return nil, fmt.Errorf("preference %q: its trait default %q is not a valid value, so it cannot be reset", name, trait.Default) + } + resets = append(resets, preferenceOp{action: opReset, name: name, value: trait.Default}) } } diff --git a/internal/reconcile/preference_reconciler.go b/internal/reconcile/preference_reconciler.go index 71bcc185f..76f3a2aa2 100644 --- a/internal/reconcile/preference_reconciler.go +++ b/internal/reconcile/preference_reconciler.go @@ -7,6 +7,7 @@ import ( "strings" "connectrpc.com/connect" + "github.com/raystack/frontier/core/preference" "github.com/raystack/frontier/internal/bootstrap/schema" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" ) @@ -65,12 +66,12 @@ func (r *PreferenceReconciler) Reconcile(ctx context.Context, spec []byte, dryRu if err != nil { return Report{}, err } - defaults, err := r.fetchDefaults(ctx) + traits, err := r.fetchTraits(ctx) if err != nil { return Report{}, err } - ops, err := diffPreferences(specs, current, defaults) + ops, err := diffPreferences(specs, current, traits) if err != nil { return Report{}, err } @@ -99,15 +100,15 @@ func (r *PreferenceReconciler) Export(ctx context.Context) (any, error) { if err != nil { return nil, err } - defaults, err := r.fetchDefaults(ctx) + traits, err := r.fetchTraits(ctx) if err != nil { return nil, err } specs := make([]PreferenceSpec, 0, len(current)) for name, value := range current { - def, known := defaults[name] - if !known || value == def { + trait, known := traits[name] + if !known || value == trait.Default { continue } specs = append(specs, PreferenceSpec{Name: name, Value: value}) @@ -130,21 +131,62 @@ func (r *PreferenceReconciler) fetchCurrent(ctx context.Context) (map[string]str return current, nil } -// fetchDefaults reads the platform traits and their defaults. It is both the -// map of defaults and the set of valid platform preference names. -func (r *PreferenceReconciler) fetchDefaults(ctx context.Context) (map[string]string, error) { +// fetchTraits reads the platform traits, keyed by name. A trait carries its +// default and its validator, so this one map is the source of defaults, the set +// of valid platform preference names, and the rule a value must pass. +func (r *PreferenceReconciler) fetchTraits(ctx context.Context) (map[string]preference.Trait, error) { resp, err := r.client.DescribePreferences(ctx, authReq(&frontierv1beta1.DescribePreferencesRequest{}, r.header)) if err != nil { return nil, fmt.Errorf("describe preferences: %w", err) } - defaults := map[string]string{} + traits := map[string]preference.Trait{} for _, t := range resp.Msg.GetTraits() { if t.GetResourceType() != schema.PlatformNamespace { continue } - defaults[t.GetName()] = t.GetDefault() + traits[t.GetName()] = traitFromPB(t) + } + return traits, nil +} + +// traitFromPB rebuilds the core trait from its wire form, so the plan can +// validate a value with the same validator the server applies on write. +func traitFromPB(t *frontierv1beta1.PreferenceTrait) preference.Trait { + tr := preference.Trait{ + Name: t.GetName(), + Default: t.GetDefault(), + Input: inputTypeFromPB(t.GetInputType()), + InputHints: t.GetInputHints(), + } + for _, o := range t.GetInputOptions() { + tr.InputOptions = append(tr.InputOptions, preference.InputHintOption{ + Name: o.GetName(), + Description: o.GetDescription(), + }) + } + return tr +} + +// inputTypeFromPB maps the wire input type back to the core one, the inverse of +// transformPreferenceTraitToPB in the API layer. An unset or unknown type falls +// back to text, which the trait validator treats as "any non-empty value". +func inputTypeFromPB(it frontierv1beta1.PreferenceTrait_InputType) preference.TraitInput { + switch it { + case frontierv1beta1.PreferenceTrait_INPUT_TYPE_TEXTAREA: + return preference.TraitInputTextarea + case frontierv1beta1.PreferenceTrait_INPUT_TYPE_SELECT: + return preference.TraitInputSelect + case frontierv1beta1.PreferenceTrait_INPUT_TYPE_COMBOBOX: + return preference.TraitInputCombobox + case frontierv1beta1.PreferenceTrait_INPUT_TYPE_CHECKBOX: + return preference.TraitInputCheckbox + case frontierv1beta1.PreferenceTrait_INPUT_TYPE_MULTISELECT: + return preference.TraitInputMultiselect + case frontierv1beta1.PreferenceTrait_INPUT_TYPE_NUMBER: + return preference.TraitInputNumber + default: + return preference.TraitInputText } - return defaults, nil } func (r *PreferenceReconciler) apply(ctx context.Context, op preferenceOp) error { diff --git a/internal/reconcile/preference_reconciler_test.go b/internal/reconcile/preference_reconciler_test.go index ef2294758..9b446f121 100644 --- a/internal/reconcile/preference_reconciler_test.go +++ b/internal/reconcile/preference_reconciler_test.go @@ -94,6 +94,16 @@ func TestPreferenceReconciler(t *testing.T) { assert.Empty(t, api.created) }) + t.Run("a value the trait rejects fails before applying", func(t *testing.T) { + api := &fakePreferenceAPI{traits: platformTraits()} + spec := []byte("- {name: disable_orgs_on_create, value: \"\"}\n") // empty is never accepted + + _, err := NewPreferenceReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.ErrorContains(t, err, "not a valid value") + assert.Empty(t, api.created) + }) + t.Run("an unknown field in the spec fails the plan", func(t *testing.T) { api := &fakePreferenceAPI{traits: platformTraits()} // `valu` instead of `value`: must fail, not silently ignore the value diff --git a/internal/reconcile/preference_test.go b/internal/reconcile/preference_test.go index 8bbf4afd3..1bbf9d06e 100644 --- a/internal/reconcile/preference_test.go +++ b/internal/reconcile/preference_test.go @@ -3,21 +3,24 @@ package reconcile import ( "testing" + "github.com/raystack/frontier/core/preference" "github.com/stretchr/testify/assert" ) func TestDiffPreferences(t *testing.T) { - defaults := map[string]string{ - "disable_orgs_on_create": "false", - "disable_orgs_listing": "false", - "invite_with_roles": "true", + // All three are checkbox traits, so their validator accepts only "true" or + // "false" and rejects an empty value. + traits := map[string]preference.Trait{ + "disable_orgs_on_create": {Input: preference.TraitInputCheckbox, InputHints: "true,false", Default: "false"}, + "disable_orgs_listing": {Input: preference.TraitInputCheckbox, InputHints: "true,false", Default: "false"}, + "invite_with_roles": {Input: preference.TraitInputCheckbox, InputHints: "true,false", Default: "true"}, } t.Run("no changes when the file matches the server", func(t *testing.T) { current := map[string]string{"disable_orgs_on_create": "true"} ops, err := diffPreferences([]PreferenceSpec{ {Name: "disable_orgs_on_create", Value: "true"}, - }, current, defaults) + }, current, traits) assert.NoError(t, err) assert.Empty(t, ops) }) @@ -26,7 +29,7 @@ func TestDiffPreferences(t *testing.T) { current := map[string]string{"disable_orgs_on_create": "false"} ops, err := diffPreferences([]PreferenceSpec{ {Name: "disable_orgs_on_create", Value: "true"}, - }, current, defaults) + }, current, traits) assert.NoError(t, err) if assert.Len(t, ops, 1) { assert.Equal(t, "set preference disable_orgs_on_create = true", ops[0].String()) @@ -38,7 +41,7 @@ func TestDiffPreferences(t *testing.T) { // not in the DB yet: the effective value is the default "false" ops, err := diffPreferences([]PreferenceSpec{ {Name: "disable_orgs_on_create", Value: "true"}, - }, map[string]string{}, defaults) + }, map[string]string{}, traits) assert.NoError(t, err) if assert.Len(t, ops, 1) { assert.Equal(t, opSet, ops[0].action) @@ -48,7 +51,7 @@ func TestDiffPreferences(t *testing.T) { t.Run("a value equal to the default and stored nowhere is a no-op", func(t *testing.T) { ops, err := diffPreferences([]PreferenceSpec{ {Name: "disable_orgs_on_create", Value: "false"}, - }, map[string]string{}, defaults) + }, map[string]string{}, traits) assert.NoError(t, err) assert.Empty(t, ops) }) @@ -60,7 +63,7 @@ func TestDiffPreferences(t *testing.T) { } ops, err := diffPreferences([]PreferenceSpec{ {Name: "disable_orgs_on_create", Value: "true"}, - }, current, defaults) + }, current, traits) assert.NoError(t, err) if assert.Len(t, ops, 1) { assert.Equal(t, "reset preference invite_with_roles to default", ops[0].String()) @@ -75,7 +78,7 @@ func TestDiffPreferences(t *testing.T) { } ops, err := diffPreferences([]PreferenceSpec{ {Name: "disable_orgs_on_create", Value: "true"}, // set - }, current, defaults) + }, current, traits) assert.NoError(t, err) if assert.Len(t, ops, 2) { assert.Equal(t, "set preference disable_orgs_on_create = true", ops[0].String()) @@ -89,7 +92,7 @@ func TestDiffPreferences(t *testing.T) { current := map[string]string{"disable_orgs_on_create": ""} ops, err := diffPreferences([]PreferenceSpec{ {Name: "disable_orgs_on_create", Value: "false"}, // the default - }, current, defaults) + }, current, traits) assert.NoError(t, err) assert.Empty(t, ops) }) @@ -97,7 +100,7 @@ func TestDiffPreferences(t *testing.T) { t.Run("an unknown preference name fails the plan", func(t *testing.T) { _, err := diffPreferences([]PreferenceSpec{ {Name: "not_a_trait", Value: "x"}, - }, map[string]string{}, defaults) + }, map[string]string{}, traits) assert.ErrorContains(t, err, `unknown platform preference "not_a_trait"`) }) @@ -105,19 +108,47 @@ func TestDiffPreferences(t *testing.T) { _, err := diffPreferences([]PreferenceSpec{ {Name: "invite_with_roles", Value: "true"}, {Name: "invite_with_roles", Value: "false"}, - }, map[string]string{}, defaults) + }, map[string]string{}, traits) assert.ErrorContains(t, err, "listed more than once") }) t.Run("an empty name fails the plan", func(t *testing.T) { - _, err := diffPreferences([]PreferenceSpec{{Name: "", Value: "x"}}, map[string]string{}, defaults) + _, err := diffPreferences([]PreferenceSpec{{Name: "", Value: "x"}}, map[string]string{}, traits) assert.ErrorContains(t, err, "name is required") }) t.Run("a stored value whose trait no longer exists is left alone", func(t *testing.T) { current := map[string]string{"gone_trait": "whatever"} - ops, err := diffPreferences(nil, current, defaults) + ops, err := diffPreferences(nil, current, traits) assert.NoError(t, err) assert.Empty(t, ops) }) + + t.Run("an empty value the trait does not accept fails the plan", func(t *testing.T) { + // A checkbox trait accepts only "true" or "false", so an empty value is a + // set the server would reject. It must fail the plan, not appear in it. + _, err := diffPreferences([]PreferenceSpec{ + {Name: "disable_orgs_on_create", Value: ""}, + }, map[string]string{}, traits) + assert.ErrorContains(t, err, "not a valid value") + }) + + t.Run("a value outside a trait's options fails the plan", func(t *testing.T) { + _, err := diffPreferences([]PreferenceSpec{ + {Name: "disable_orgs_on_create", Value: "maybe"}, + }, map[string]string{}, traits) + assert.ErrorContains(t, err, "not a valid value") + }) + + t.Run("a reset to an unreachable default fails the plan", func(t *testing.T) { + // A misconfigured text trait whose default its own validator rejects + // (text cannot be empty). Resetting it would write a value the server + // refuses, so the plan must fail rather than emit that reset. + broken := map[string]preference.Trait{ + "broken": {Input: preference.TraitInputText, Default: ""}, + } + current := map[string]string{"broken": "something"} // stored, not listed -> reset + _, err := diffPreferences(nil, current, broken) + assert.ErrorContains(t, err, "cannot be reset") + }) } From d19cf3aa248428c1593808b011d2e9acf4d46966 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 23 Jul 2026 12:34:56 +0530 Subject: [PATCH 2/3] docs(reconcile): note preference values are checked against the trait --- docs/content/docs/reconcile.mdx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/content/docs/reconcile.mdx b/docs/content/docs/reconcile.mdx index b37de5ed2..7eb3592c2 100644 --- a/docs/content/docs/reconcile.mdx +++ b/docs/content/docs/reconcile.mdx @@ -187,6 +187,9 @@ spec: - Values are strings. A yes/no setting is `"true"` or `"false"`. - The name must be a platform trait the server knows. An unknown name fails the plan. +- The value must be one the trait accepts. The plan checks it against the live trait, so an + empty value, or a value outside a yes/no or select trait's options, fails the plan up front + instead of failing later when it is applied. - This kind resets instead of deleting. The file is the full desired state: a preference you list is set to your value, and a preference you leave out goes back to its default. There is no `delete` flag, because setting a preference back to its default is what From aec40aad61031766fdaa9698d08f8d8eca6fc7ce Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 23 Jul 2026 12:54:26 +0530 Subject: [PATCH 3/3] fix(reconcile): validate a preference value only when it becomes a set The first cut validated every listed value, which broke the export round trip (rule 5): a value stored under an older or looser trait definition is exported verbatim, and re-reconciling it was rejected even though it equals the server's own value and plans no op. Validate only a value that differs from the one in effect, so a planned set is still appliable but a no-op set is left alone. Also treat a stored empty value as unset in both the reset loop and export, so it is not reset and not exported as an empty value the plan would reject. Adds tests for the stale-value round trip, the stored-empty cases, and value validation over the wire (exercising the proto-to-core trait mapping, not just the text path). --- internal/reconcile/preference.go | 66 ++++++++++--------- internal/reconcile/preference_reconciler.go | 5 +- .../reconcile/preference_reconciler_test.go | 43 ++++++++++++ internal/reconcile/preference_test.go | 25 +++++++ 4 files changed, 108 insertions(+), 31 deletions(-) diff --git a/internal/reconcile/preference.go b/internal/reconcile/preference.go index 3187bf253..b4677c829 100644 --- a/internal/reconcile/preference.go +++ b/internal/reconcile/preference.go @@ -46,7 +46,19 @@ func (o preferenceOp) String() string { // source of defaults, the set of known names, and the validator each value must // pass. func diffPreferences(desired []PreferenceSpec, current map[string]string, traits map[string]preference.Trait) ([]preferenceOp, error) { + // serverValue is the value in effect on the server: the stored value, or the + // trait default when nothing (or an empty string) is stored. An empty stored + // value counts as unset, matching how the server resolves platform + // preferences, so a file entry equal to the default plans no change. + serverValue := func(name string) string { + if v, ok := current[name]; ok && v != "" { + return v + } + return traits[name].Default + } + desiredByName := make(map[string]string, len(desired)) + var sets []preferenceOp for _, s := range desired { if strings.TrimSpace(s.Name) == "" { return nil, fmt.Errorf("preference name is required") @@ -58,33 +70,24 @@ func diffPreferences(desired []PreferenceSpec, current map[string]string, traits if !known { return nil, fmt.Errorf("unknown platform preference %q", s.Name) } - // The server validates a value against its trait and rejects one it does - // not accept: an empty value, or a value outside a checkbox or select - // trait's options. Check it here so an unappliable set fails the plan with - // a clear message instead of failing late at the API. - if !trait.GetValidator().Validate(s.Value) { - return nil, fmt.Errorf("preference %q: %q is not a valid value for its trait", s.Name, s.Value) - } desiredByName[s.Name] = s.Value - } - // serverValue is the value in effect on the server: the stored value, or - // the trait default when nothing is stored. An empty stored value counts as - // unset, matching how the server resolves platform preferences, so a file - // entry equal to the default plans no change against it. - serverValue := func(name string) string { - if v, ok := current[name]; ok && v != "" { - return v + // Only a value that differs from the one in effect becomes a set. Validate + // just those against the trait, so a set the server would reject fails the + // plan up front instead of at the API. A value equal to the server's own + // value plans nothing, so it is left unchecked: that keeps the export of a + // value stored under an older, looser trait definition round-tripping to + // zero ops (rule 5), rather than rejecting a state the server can reach. + if s.Value == serverValue(s.Name) { + continue } - return traits[name].Default - } - - var sets, resets []preferenceOp - for name, value := range desiredByName { - if value != serverValue(name) { - sets = append(sets, preferenceOp{action: opSet, name: name, value: value}) + if !trait.GetValidator().Validate(s.Value) { + return nil, fmt.Errorf("preference %q: %q is not a valid value for its trait", s.Name, s.Value) } + sets = append(sets, preferenceOp{action: opSet, name: s.Name, value: s.Value}) } + + var resets []preferenceOp for name, value := range current { trait, known := traits[name] if !known { @@ -95,15 +98,18 @@ func diffPreferences(desired []PreferenceSpec, current map[string]string, traits if _, listed := desiredByName[name]; listed { continue } - if value != trait.Default { - // A reset writes the trait default back. If the trait would reject its - // own default, the default is not reachable and the reset can never - // apply, so fail the plan rather than emit an op that dies at the API. - if !trait.GetValidator().Validate(trait.Default) { - return nil, fmt.Errorf("preference %q: its trait default %q is not a valid value, so it cannot be reset", name, trait.Default) - } - resets = append(resets, preferenceOp{action: opReset, name: name, value: trait.Default}) + // A stored empty value counts as unset, so it is already at its default and + // needs no reset. Only a stored value that really differs is reset. + if value == "" || value == trait.Default { + continue + } + // A reset writes the trait default back. If the trait would reject its own + // default, the default is not reachable and the reset can never apply, so + // fail the plan rather than emit an op that dies at the API. + if !trait.GetValidator().Validate(trait.Default) { + return nil, fmt.Errorf("preference %q: its trait default %q is not a valid value, so it cannot be reset", name, trait.Default) } + resets = append(resets, preferenceOp{action: opReset, name: name, value: trait.Default}) } sort.Slice(sets, func(i, j int) bool { return sets[i].name < sets[j].name }) diff --git a/internal/reconcile/preference_reconciler.go b/internal/reconcile/preference_reconciler.go index 76f3a2aa2..3e8911d3b 100644 --- a/internal/reconcile/preference_reconciler.go +++ b/internal/reconcile/preference_reconciler.go @@ -108,7 +108,10 @@ func (r *PreferenceReconciler) Export(ctx context.Context) (any, error) { specs := make([]PreferenceSpec, 0, len(current)) for name, value := range current { trait, known := traits[name] - if !known || value == trait.Default { + // A stored empty value counts as unset, so it is already at its default; + // leave it out, matching how the diff resolves it. Exporting it would emit + // a value the plan then rejects. + if !known || value == "" || value == trait.Default { continue } specs = append(specs, PreferenceSpec{Name: name, Value: value}) diff --git a/internal/reconcile/preference_reconciler_test.go b/internal/reconcile/preference_reconciler_test.go index 9b446f121..49fdf51bb 100644 --- a/internal/reconcile/preference_reconciler_test.go +++ b/internal/reconcile/preference_reconciler_test.go @@ -33,6 +33,13 @@ func platformTraitPB(name, def string) *frontierv1beta1.PreferenceTrait { return &frontierv1beta1.PreferenceTrait{Name: name, Default: def, ResourceType: schema.PlatformNamespace} } +func platformCheckboxTraitPB(name, def string) *frontierv1beta1.PreferenceTrait { + return &frontierv1beta1.PreferenceTrait{ + Name: name, Default: def, ResourceType: schema.PlatformNamespace, + InputType: frontierv1beta1.PreferenceTrait_INPUT_TYPE_CHECKBOX, InputHints: "true,false", + } +} + func prefPB(name, value string) *frontierv1beta1.Preference { return &frontierv1beta1.Preference{Name: name, Value: value} } @@ -104,6 +111,42 @@ func TestPreferenceReconciler(t *testing.T) { assert.Empty(t, api.created) }) + t.Run("validates a value over the wire using the trait input type", func(t *testing.T) { + // The trait comes back as a checkbox, so the boolean validator rejects a + // value that is not true or false. This exercises the proto-to-core trait + // mapping, not just the default text path. + api := &fakePreferenceAPI{traits: []*frontierv1beta1.PreferenceTrait{ + platformCheckboxTraitPB("disable_orgs_on_create", "false"), + }} + spec := []byte("- {name: disable_orgs_on_create, value: maybe}\n") + + _, err := NewPreferenceReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.ErrorContains(t, err, "not a valid value") + assert.Empty(t, api.created) + }) + + t.Run("a stored empty value is left out of the export and round-trips", func(t *testing.T) { + // The server treats an empty stored value as unset, so it is effectively at + // its default. Export must leave it out, and reconciling the export must + // plan nothing, not a set of "" that the plan would now reject. + api := &fakePreferenceAPI{ + traits: platformTraits(), + prefs: []*frontierv1beta1.Preference{prefPB("disable_orgs_on_create", "")}, + } + registry := map[string]Reconciler{KindPreference: NewPreferenceReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindPreference) + assert.NoError(t, err) + assert.NotContains(t, string(out), "disable_orgs_on_create") + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + t.Run("an unknown field in the spec fails the plan", func(t *testing.T) { api := &fakePreferenceAPI{traits: platformTraits()} // `valu` instead of `value`: must fail, not silently ignore the value diff --git a/internal/reconcile/preference_test.go b/internal/reconcile/preference_test.go index 1bbf9d06e..da0e5c3d6 100644 --- a/internal/reconcile/preference_test.go +++ b/internal/reconcile/preference_test.go @@ -124,6 +124,16 @@ func TestDiffPreferences(t *testing.T) { assert.Empty(t, ops) }) + t.Run("a stored empty value not in the file is left alone, not reset", func(t *testing.T) { + // An empty stored value counts as unset, so it is already at its default + // and needs no reset. Planning a reset here would write the default on + // every run and break the export round trip. + current := map[string]string{"disable_orgs_on_create": ""} + ops, err := diffPreferences(nil, current, traits) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + t.Run("an empty value the trait does not accept fails the plan", func(t *testing.T) { // A checkbox trait accepts only "true" or "false", so an empty value is a // set the server would reject. It must fail the plan, not appear in it. @@ -140,6 +150,21 @@ func TestDiffPreferences(t *testing.T) { assert.ErrorContains(t, err, "not a valid value") }) + t.Run("a listed value equal to the server value round-trips even if the trait would now reject it", func(t *testing.T) { + // R5: export lists the server's stored value verbatim. If a trait's options + // tighten after a value was stored (a changed custom trait, or an upgrade), + // the server still holds the old value. Reconciling its export must plan + // zero ops, not reject the value it just exported. So a value equal to the + // one in effect is never re-validated, only a value that becomes a set is. + theme := map[string]preference.Trait{ + "theme": {Input: preference.TraitInputSelect, InputHints: "light,dark", Default: "light"}, + } + current := map[string]string{"theme": "auto"} // stored when "auto" was allowed + ops, err := diffPreferences([]PreferenceSpec{{Name: "theme", Value: "auto"}}, current, theme) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + t.Run("a reset to an unreachable default fails the plan", func(t *testing.T) { // A misconfigured text trait whose default its own validator rejects // (text cannot be empty). Resetting it would write a value the server