feat(reconcile): validate preference values against the trait at plan time

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

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,42 +42,54 @@ 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")
 		}
 		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 })

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 {

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