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 diff --git a/internal/reconcile/preference.go b/internal/reconcile/preference.go index 9fc79a89c..b4677c829 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,10 +42,23 @@ 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) { + // 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") @@ -51,31 +66,30 @@ 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) } 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 defaults[name] - } - - 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 { - 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,9 +98,18 @@ 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}) + // 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 71bcc185f..3e8911d3b 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,18 @@ 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] + // 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}) @@ -130,21 +134,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..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} } @@ -94,6 +101,52 @@ 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("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 8bbf4afd3..da0e5c3d6 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,72 @@ 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("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. + _, 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 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 + // 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") + }) }