From e09479be3d4d9b007ea5fd7c74b89ffbd992ce96 Mon Sep 17 00:00:00 2001 From: Alex Karev Date: Mon, 8 Jun 2026 16:22:14 -0700 Subject: [PATCH 1/7] feat(): add string type support --- README.md | 34 ++++++++++++++++++++++++++++++++-- cli/cli.go | 4 ++-- cli/controller/controller.go | 2 +- client/client.go | 11 +++++++++++ client/client_test.go | 12 +++++++++++- models/feature.go | 14 +++++++++++++- models/feature_test.go | 19 +++++++++++++++++++ 7 files changed, 89 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index d6b2d8f..da413d4 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ Each of these components are comprised of lower level libraries that you can use ### About Feature Flags -Feature flags have many use cases and there are many implementations. With Decider, the three supported types of flags are `boolean`, `percentile`, and `scalar`. For our purposes at [VSCO](http://vsco.co), these have been enough to handle our needs. +Feature flags have many use cases and there are many implementations. With Decider, the supported types of flags are `boolean`, `percentile`, `scalar`, and `string`. For our purposes at [VSCO](http://vsco.co), these have been enough to handle our needs. #### Boolean Flags An example use case for a `boolean` flag would be an API kill switch that could alleviate load for a backing database. @@ -75,6 +75,18 @@ waitMS := dcdr.ScaleValue("daemon-db-insert-wait-ms", 0, 1000) time.Sleep(waitMS * time.Millisecond) ``` +#### String Flags +A `string` flag holds a free-form text value. Any `-value` that is not a boolean or a number is stored as a string. A common use case is a runtime-tunable setting such as a minimum log level. + +``` +min-log-level => "debug" +``` + +```Go +// Returns the configured value, or "" if the flag is absent or not a string. +level := dcdr.GetString("min-log-level") +``` + [Read more](#using-the-go-client) on how to use the `Client`. ### Caveat @@ -349,7 +361,7 @@ if err != nil { ### Checking feature flags -The client has three main methods for interacting with flags `IsAvailable(feature string)`. `IsAvailableForID(feature string, id uint64)`, and `ScaleValue(feature string, min float64, max float64)`. +The client has four main methods for interacting with flags `IsAvailable(feature string)`. `IsAvailableForID(feature string, id uint64)`, `ScaleValue(feature string, min float64, max float64)`, and `GetString(feature string)`. #### IsAvailable @@ -472,6 +484,24 @@ for { } ``` +### GetString + +`GetString` returns the value of a `string` feature. It returns `""` when the feature is absent or is not a string-typed flag, so callers should fall back to a sane default for unknown values. + +``` +# set a string feature +dcdr set -n min-log-level -v debug +``` + +```Go +// min-log-level would be "debug" +level := dcdr.GetString("min-log-level") + +if level == "" { + level = "info" // fall back to a default +} +``` + ## Building a custom Server Exposing your feature flags to the open internet would be a terrible idea in most cases. The default server will work fine as long as access is restricted to internal network clients but what if we want to allow access to mobile devices? Since there are entirely too many auth strategies to cover and we are kind of lazy, Decider `Server` allows you to add middleware to customize its behavior to suit your authentication needs. diff --git a/cli/cli.go b/cli/cli.go index 9a59f74..7151278 100644 --- a/cli/cli.go +++ b/cli/cli.go @@ -78,7 +78,7 @@ func (c *CLI) Commands() []climax.Command { { Name: "set", Brief: "create or update a feature flag", - Usage: `set -name flag_name -value [0.0-1.0|true/false] -comment "flag description"`, + Usage: `set -name flag_name -value [0.0-1.0|true/false|string] -comment "flag description"`, Help: ` @@ -117,7 +117,7 @@ func (c *CLI) Commands() []climax.Command { { Name: "value", Short: "v", - Usage: `--value=0.0-1.0 or true|false`, + Usage: `--value=0.0-1.0, true|false, or a string`, Help: `the value of the flag`, Variable: true, }, diff --git a/cli/controller/controller.go b/cli/controller/controller.go index 1e60829..3e18005 100644 --- a/cli/controller/controller.go +++ b/cli/controller/controller.go @@ -23,7 +23,7 @@ import ( const filePerms = 0775 var ( - errInvalidFeatureType = errors.New("invalid -value format. use -value=[0.0-1.0] or [true|false]") + errInvalidFeatureType = errors.New("invalid -value format. use -value=[0.0-1.0], [true|false], or a string") errInvalidRange = errors.New("invalid -value for percentile. use -value=[0.0-1.0]") errNameRequired = errors.New("-name is required") ) diff --git a/client/client.go b/client/client.go index 38fc59a..ce3c4c1 100644 --- a/client/client.go +++ b/client/client.go @@ -16,6 +16,7 @@ import ( type IFace interface { IsAvailable(feature string) bool IsAvailableForID(feature string, id uint64) bool + GetString(feature string) string ScaleValue(feature string, min float64, max float64) float64 UpdateFeatures(bts []byte) FeatureExists(feature string) bool @@ -168,6 +169,16 @@ func (c *Client) IsAvailable(feature string) bool { } } +// GetString returns the string value of `feature`, or "" if it is absent or +// not a string-typed feature. +func (c *Client) GetString(feature string) string { + if val, ok := c.Features()[feature].(string); ok { + return val + } + + return "" +} + // IsAvailableForID used to check features with float values between 0.0-1.0. // Returns false if a non-percentile type `feature` is passed. func (c *Client) IsAvailableForID(feature string, id uint64) bool { diff --git a/client/client_test.go b/client/client_test.go index f06e881..8405e02 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -49,7 +49,8 @@ var JSONBytes = []byte(`{ "float": 0, "bool_false": false, "bool": true, - "default_float": 0.5 + "default_float": 0.5, + "str": "debug" } }, "info": { @@ -151,6 +152,15 @@ func TestIsAvailableForID(t *testing.T) { assert.True(t, c.IsAvailableForID("default_float", 5)) } +func TestGetString(t *testing.T) { + m := MockFeatureMap() + c := NewTestClient().SetFeatureMap(m) + + assert.Equal(t, "debug", c.GetString("str")) + assert.Equal(t, "", c.GetString("nope")) + assert.Equal(t, "", c.GetString("bool")) +} + func TestScaleValue(t *testing.T) { m := MockFeatureMap() c := NewTestClient().SetFeatureMap(m) diff --git a/models/feature.go b/models/feature.go index 9c7f3f1..78a5172 100644 --- a/models/feature.go +++ b/models/feature.go @@ -21,6 +21,8 @@ const ( Percentile FeatureType = "percentile" // Boolean boolean `FeatureType` Boolean FeatureType = "boolean" + // String string `FeatureType` + String FeatureType = "string" // Invalid invalid `FeatureType` Invalid FeatureType = "invalid" // FeatureScope scoping for feature keys @@ -47,7 +49,10 @@ func ParseValueAndFeatureType(v string) (interface{}, FeatureType) { return i, Percentile } - return nil, Invalid + // Any value that is not a bool or a number is treated as a free-form + // string feature, so this function never returns Invalid. The Invalid + // constant is retained for callers that compare against it explicitly. + return v, String } // Feature KV model for feature flags @@ -89,6 +94,8 @@ func NewFeature(name string, value interface{}, comment string, user string, sco ft = Percentile case bool: ft = Boolean + case string: + ft = String } f = &Feature{ @@ -114,6 +121,11 @@ func (f *Feature) BoolValue() bool { return f.Value.(bool) } +// StringValue cast Value to string +func (f *Feature) StringValue() string { + return f.Value.(string) +} + // ToJSON marshal feature to json func (f *Feature) ToJSON() ([]byte, error) { return json.Marshal(f) diff --git a/models/feature_test.go b/models/feature_test.go index f2ce52d..ee85245 100644 --- a/models/feature_test.go +++ b/models/feature_test.go @@ -14,6 +14,21 @@ func TestGetFeatureTypeFromValue(t *testing.T) { _, ft := ParseValueAndFeatureType(v) assert.Equal(t, Percentile, ft, v) } + + booleans := []string{"true", "false"} + + for _, v := range booleans { + _, ft := ParseValueAndFeatureType(v) + assert.Equal(t, Boolean, ft, v) + } + + strings := []string{"debug", "info", "some-string"} + + for _, v := range strings { + val, ft := ParseValueAndFeatureType(v) + assert.Equal(t, String, ft, v) + assert.Equal(t, v, val, v) + } } func TestMarshaling(t *testing.T) { @@ -40,4 +55,8 @@ func TestTypes(t *testing.T) { pf = NewFeature("key", true, "comment", "user", "scope", "n") assert.Equal(t, Boolean, pf.FeatureType) assert.Equal(t, true, pf.BoolValue()) + + pf = NewFeature("key", "debug", "comment", "user", "scope", "n") + assert.Equal(t, String, pf.FeatureType) + assert.Equal(t, "debug", pf.StringValue()) } From a0760a55a07fa60f562e5fd2f829fce5d75c666b Mon Sep 17 00:00:00 2001 From: Alex Karev Date: Mon, 8 Jun 2026 17:56:32 -0700 Subject: [PATCH 2/7] add -type param --- README.md | 9 ++-- cli/api/client_test.go | 20 +++++++ cli/cli.go | 15 +++++- cli/controller/controller.go | 64 ++++++++++++++++++---- cli/controller/controller_test.go | 90 +++++++++++++++++++++++++++++++ client/client_test.go | 9 ++++ models/feature.go | 32 +++++++++++ models/feature_test.go | 43 +++++++++++++++ 8 files changed, 267 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index da413d4..1e04f2c 100644 --- a/README.md +++ b/README.md @@ -76,9 +76,12 @@ time.Sleep(waitMS * time.Millisecond) ``` #### String Flags -A `string` flag holds a free-form text value. Any `-value` that is not a boolean or a number is stored as a string. A common use case is a runtime-tunable setting such as a minimum log level. +A `string` flag holds a free-form text value. Strings require an explicit `--type=string` (`-t string`) when setting them, while `boolean` and `percentile` are inferred from the value when `--type` is omitted. A common use case is a runtime-tunable setting such as a minimum log level. ``` +# --type=string is required for string values +dcdr set -n min-log-level -v debug -t string + min-log-level => "debug" ``` @@ -489,8 +492,8 @@ for { `GetString` returns the value of a `string` feature. It returns `""` when the feature is absent or is not a string-typed flag, so callers should fall back to a sane default for unknown values. ``` -# set a string feature -dcdr set -n min-log-level -v debug +# set a string feature (--type=string is required) +dcdr set -n min-log-level -v debug -t string ``` ```Go diff --git a/cli/api/client_test.go b/cli/api/client_test.go index c8ab7d5..29eb5ca 100644 --- a/cli/api/client_test.go +++ b/cli/api/client_test.go @@ -127,6 +127,26 @@ func TestDeleteWithError(t *testing.T) { assert.Equal(t, e, err) } +func TestKVsToFeatureMapStringValue(t *testing.T) { + kvb := stores.KVBytes{ + &stores.KVByte{ + Key: "dcdr/features/default/min-log-level", + Bytes: []byte(`{ "feature_type": "string", "key": "min-log-level", "value": "debug" }`), + }, + } + + cs := stores.NewMockStore(nil, nil) + cfg := config.DefaultConfig() + c := New(cs, &stores.MockRepo{}, cfg, nil) + + fm, err := c.KVsToFeatureMap(kvb) + assert.Nil(t, err) + assert.NotNil(t, fm) + + // the served value map flattens to just the value, preserving the string + assert.Equal(t, "debug", fm.Dcdr.Defaults()["min-log-level"]) +} + func TestKVsToFeatureMapInfoExistByNameSpace(t *testing.T) { kvb := stores.KVBytes{ diff --git a/cli/cli.go b/cli/cli.go index 7151278..ffee52b 100644 --- a/cli/cli.go +++ b/cli/cli.go @@ -78,7 +78,7 @@ func (c *CLI) Commands() []climax.Command { { Name: "set", Brief: "create or update a feature flag", - Usage: `set -name flag_name -value [0.0-1.0|true/false|string] -comment "flag description"`, + Usage: `set -name flag_name -value [-type boolean|percentile|string] -comment "flag description"`, Help: ` @@ -117,10 +117,17 @@ func (c *CLI) Commands() []climax.Command { { Name: "value", Short: "v", - Usage: `--value=0.0-1.0, true|false, or a string`, + Usage: `--value=0.0-1.0 or true|false`, Help: `the value of the flag`, Variable: true, }, + { + Name: "type", + Short: "t", + Usage: `--type=boolean|percentile|string`, + Help: `flag type; required for string values, inferred for boolean/percentile when omitted`, + Variable: true, + }, { Name: "comment", Short: "c", @@ -146,6 +153,10 @@ func (c *CLI) Commands() []climax.Command { Usecase: `-n "flag_name" -v false -c "the flag desc"`, Description: `sets a boolean flag to false`, }, + { + Usecase: `-n "flag_name" -v debug -t string -c "the flag desc"`, + Description: `sets a string flag to "debug"`, + }, }, Handle: c.Ctrl.Set, diff --git a/cli/controller/controller.go b/cli/controller/controller.go index 3e18005..ded767c 100644 --- a/cli/controller/controller.go +++ b/cli/controller/controller.go @@ -23,9 +23,11 @@ import ( const filePerms = 0775 var ( - errInvalidFeatureType = errors.New("invalid -value format. use -value=[0.0-1.0], [true|false], or a string") - errInvalidRange = errors.New("invalid -value for percentile. use -value=[0.0-1.0]") - errNameRequired = errors.New("-name is required") + errInvalidType = errors.New("invalid -type. use boolean, percentile, or string") + errTypeRequiredForString = errors.New("-type=string is required for non-numeric, non-boolean values") + errInvalidBool = errors.New("invalid -value for boolean. use -value=[true|false]") + errInvalidRange = errors.New("invalid -value for percentile. use -value=[0.0-1.0]") + errNameRequired = errors.New("-name is required") ) // Controller handler for CLI commands @@ -277,6 +279,7 @@ func (cc *Controller) Watch(ctx climax.Context) int { func (cc *Controller) ParseContext(ctx climax.Context) (*models.Feature, error) { name, _ := ctx.Get("name") val, _ := ctx.Get("value") + typ, _ := ctx.Get("type") cmt, _ := ctx.Get("comment") scp, _ := ctx.Get("scope") @@ -288,21 +291,62 @@ func (cc *Controller) ParseContext(ctx climax.Context) (*models.Feature, error) var ft models.FeatureType if val != "" { - v, ft = models.ParseValueAndFeatureType(val) + var err error + v, ft, err = parseValue(val, typ) - if ft == models.Invalid { - return nil, errInvalidFeatureType + if err != nil { + return nil, err + } + } + + f := models.NewFeature(name, v, cmt, cc.Config.Username, scp, cc.Config.Namespace) + f.FeatureType = ft + + return f, nil +} + +// parseValue resolves the value and feature type for a `set` command. When +// `typ` is provided the value is parsed strictly for that type; when omitted +// the type is inferred for boolean/percentile only, and a string value is +// rejected so the caller is forced to pass -type=string explicitly. +func parseValue(val string, typ string) (interface{}, models.FeatureType, error) { + if typ != "" { + ft, ok := models.ParseFeatureType(typ) + + if !ok { + return nil, models.Invalid, errInvalidType + } + + v, err := models.ParseValueForType(val, ft) + + if err != nil { + if ft == models.Boolean { + return nil, models.Invalid, errInvalidBool + } + + return nil, models.Invalid, errInvalidRange } if ft == models.Percentile { if v.(float64) > 1.0 || v.(float64) < 0 { - return nil, errInvalidRange + return nil, models.Invalid, errInvalidRange } } + + return v, ft, nil } - f := models.NewFeature(name, v, cmt, cc.Config.Username, scp, cc.Config.Namespace) - f.FeatureType = ft + v, ft := models.ParseValueAndFeatureType(val) - return f, nil + if ft == models.String { + return nil, models.Invalid, errTypeRequiredForString + } + + if ft == models.Percentile { + if v.(float64) > 1.0 || v.(float64) < 0 { + return nil, models.Invalid, errInvalidRange + } + } + + return v, ft, nil } diff --git a/cli/controller/controller_test.go b/cli/controller/controller_test.go index 25f995d..89c0355 100644 --- a/cli/controller/controller_test.go +++ b/cli/controller/controller_test.go @@ -125,3 +125,93 @@ func TestSet(t *testing.T) { assert.Equal(t, Success, code) } + +func newParseController() *Controller { + return New(config.DefaultConfig(), NewMockClient(nil, nil, nil)) +} + +func parseCtx(vars map[string]string) climax.Context { + return climax.Context{Variable: vars} +} + +func TestParseContextExplicitTypes(t *testing.T) { + ctl := newParseController() + + f, err := ctl.ParseContext(parseCtx(map[string]string{ + "name": "min-log-level", "value": "debug", "type": "string", + })) + assert.NoError(t, err) + assert.Equal(t, models.String, f.FeatureType) + assert.Equal(t, "debug", f.Value) + + // string type stores bool/number-looking values verbatim + f, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "literal", "value": "true", "type": "string", + })) + assert.NoError(t, err) + assert.Equal(t, models.String, f.FeatureType) + assert.Equal(t, "true", f.Value) + + f, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "flag", "value": "true", "type": "boolean", + })) + assert.NoError(t, err) + assert.Equal(t, models.Boolean, f.FeatureType) + assert.Equal(t, true, f.Value) + + f, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "flag", "value": "0.5", "type": "percentile", + })) + assert.NoError(t, err) + assert.Equal(t, models.Percentile, f.FeatureType) + assert.Equal(t, 0.5, f.Value) +} + +func TestParseContextExplicitTypeErrors(t *testing.T) { + ctl := newParseController() + + _, err := ctl.ParseContext(parseCtx(map[string]string{ + "name": "flag", "value": "debug", "type": "nope", + })) + assert.Equal(t, errInvalidType, err) + + _, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "flag", "value": "notabool", "type": "boolean", + })) + assert.Equal(t, errInvalidBool, err) + + _, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "flag", "value": "notanumber", "type": "percentile", + })) + assert.Equal(t, errInvalidRange, err) + + _, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "flag", "value": "2.0", "type": "percentile", + })) + assert.Equal(t, errInvalidRange, err) +} + +func TestParseContextInference(t *testing.T) { + ctl := newParseController() + + // bool/percentile still work without -type + f, err := ctl.ParseContext(parseCtx(map[string]string{"name": "flag", "value": "false"})) + assert.NoError(t, err) + assert.Equal(t, models.Boolean, f.FeatureType) + + f, err = ctl.ParseContext(parseCtx(map[string]string{"name": "flag", "value": "0.25"})) + assert.NoError(t, err) + assert.Equal(t, models.Percentile, f.FeatureType) + + // a string value without -type is rejected + _, err = ctl.ParseContext(parseCtx(map[string]string{"name": "flag", "value": "debug"})) + assert.Equal(t, errTypeRequiredForString, err) + + // a numeric typo infers to string and is likewise rejected + _, err = ctl.ParseContext(parseCtx(map[string]string{"name": "flag", "value": "0.5x"})) + assert.Equal(t, errTypeRequiredForString, err) + + // out-of-range percentile without -type + _, err = ctl.ParseContext(parseCtx(map[string]string{"name": "flag", "value": "2.0"})) + assert.Equal(t, errInvalidRange, err) +} diff --git a/client/client_test.go b/client/client_test.go index 8405e02..bcc7145 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -161,6 +161,15 @@ func TestGetString(t *testing.T) { assert.Equal(t, "", c.GetString("bool")) } +// TestGetStringRoundTrip proves a string value survives the full client API +// path: served JSON payload -> UpdateFeatures -> GetString. +func TestGetStringRoundTrip(t *testing.T) { + c := NewTestClient() + c.UpdateFeatures(JSONBytes) + + assert.Equal(t, "debug", c.GetString("str")) +} + func TestScaleValue(t *testing.T) { m := MockFeatureMap() c := NewTestClient().SetFeatureMap(m) diff --git a/models/feature.go b/models/feature.go index 78a5172..b8985d0 100644 --- a/models/feature.go +++ b/models/feature.go @@ -55,6 +55,38 @@ func ParseValueAndFeatureType(v string) (interface{}, FeatureType) { return v, String } +// ParseFeatureType resolves a user-supplied `--type` value to a FeatureType. +// Only the canonical names are accepted. The second return value is false when +// the type is unrecognized. +func ParseFeatureType(s string) (FeatureType, bool) { + switch s { + case string(Boolean): + return Boolean, true + case string(Percentile): + return Percentile, true + case string(String): + return String, true + default: + return Invalid, false + } +} + +// ParseValueForType parses `val` into the concrete value for an explicitly +// provided `ft`. Unlike ParseValueAndFeatureType it does not infer the type, +// so a String stores the literal value verbatim (e.g. "true" or "0.5"). +func ParseValueForType(val string, ft FeatureType) (interface{}, error) { + switch ft { + case Boolean: + return strconv.ParseBool(val) + case Percentile: + return strconv.ParseFloat(val, 64) + case String: + return val, nil + default: + return nil, fmt.Errorf("unsupported feature type %q", ft) + } +} + // Feature KV model for feature flags type Feature struct { FeatureType FeatureType `json:"feature_type"` diff --git a/models/feature_test.go b/models/feature_test.go index ee85245..300b725 100644 --- a/models/feature_test.go +++ b/models/feature_test.go @@ -31,6 +31,49 @@ func TestGetFeatureTypeFromValue(t *testing.T) { } } +func TestParseFeatureType(t *testing.T) { + cases := map[string]FeatureType{ + "boolean": Boolean, + "percentile": Percentile, + "string": String, + } + + for in, want := range cases { + ft, ok := ParseFeatureType(in) + assert.True(t, ok, in) + assert.Equal(t, want, ft, in) + } + + for _, in := range []string{"", "bool", "pct", "decimal", "nope"} { + ft, ok := ParseFeatureType(in) + assert.False(t, ok, in) + assert.Equal(t, Invalid, ft, in) + } +} + +func TestParseValueForType(t *testing.T) { + v, err := ParseValueForType("true", Boolean) + assert.NoError(t, err) + assert.Equal(t, true, v) + + _, err = ParseValueForType("notabool", Boolean) + assert.Error(t, err) + + v, err = ParseValueForType("0.5", Percentile) + assert.NoError(t, err) + assert.Equal(t, 0.5, v) + + _, err = ParseValueForType("notanumber", Percentile) + assert.Error(t, err) + + // String stores the literal value verbatim, even bool/number-looking input. + for _, s := range []string{"debug", "true", "0.5"} { + v, err = ParseValueForType(s, String) + assert.NoError(t, err) + assert.Equal(t, s, v) + } +} + func TestMarshaling(t *testing.T) { f := &Feature{ Key: "test", From 37d060b90921e4a3643e0458c562270f5efb237d Mon Sep 17 00:00:00 2001 From: Alex Karev Date: Mon, 8 Jun 2026 18:15:41 -0700 Subject: [PATCH 3/7] clean --- cli/controller/controller.go | 20 +++++++++++++++----- cli/controller/controller_test.go | 2 +- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/cli/controller/controller.go b/cli/controller/controller.go index ded767c..0ca4dc5 100644 --- a/cli/controller/controller.go +++ b/cli/controller/controller.go @@ -26,6 +26,7 @@ var ( errInvalidType = errors.New("invalid -type. use boolean, percentile, or string") errTypeRequiredForString = errors.New("-type=string is required for non-numeric, non-boolean values") errInvalidBool = errors.New("invalid -value for boolean. use -value=[true|false]") + errInvalidPercentile = errors.New("invalid -value for percentile. must be a number") errInvalidRange = errors.New("invalid -value for percentile. use -value=[0.0-1.0]") errNameRequired = errors.New("-name is required") ) @@ -324,12 +325,12 @@ func parseValue(val string, typ string) (interface{}, models.FeatureType, error) return nil, models.Invalid, errInvalidBool } - return nil, models.Invalid, errInvalidRange + return nil, models.Invalid, errInvalidPercentile } if ft == models.Percentile { - if v.(float64) > 1.0 || v.(float64) < 0 { - return nil, models.Invalid, errInvalidRange + if err := validatePercentile(v); err != nil { + return nil, models.Invalid, err } } @@ -343,10 +344,19 @@ func parseValue(val string, typ string) (interface{}, models.FeatureType, error) } if ft == models.Percentile { - if v.(float64) > 1.0 || v.(float64) < 0 { - return nil, models.Invalid, errInvalidRange + if err := validatePercentile(v); err != nil { + return nil, models.Invalid, err } } return v, ft, nil } + +// validatePercentile ensures a percentile value falls within the 0.0-1.0 range. +func validatePercentile(v interface{}) error { + if f := v.(float64); f > 1.0 || f < 0 { + return errInvalidRange + } + + return nil +} diff --git a/cli/controller/controller_test.go b/cli/controller/controller_test.go index 89c0355..e652d27 100644 --- a/cli/controller/controller_test.go +++ b/cli/controller/controller_test.go @@ -183,7 +183,7 @@ func TestParseContextExplicitTypeErrors(t *testing.T) { _, err = ctl.ParseContext(parseCtx(map[string]string{ "name": "flag", "value": "notanumber", "type": "percentile", })) - assert.Equal(t, errInvalidRange, err) + assert.Equal(t, errInvalidPercentile, err) _, err = ctl.ParseContext(parseCtx(map[string]string{ "name": "flag", "value": "2.0", "type": "percentile", From e9807e5dc58193dbf7f25141a36a4a4656c53e95 Mon Sep 17 00:00:00 2001 From: Alex Karev Date: Mon, 8 Jun 2026 18:47:09 -0700 Subject: [PATCH 4/7] simplify --- cli/controller/controller.go | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/cli/controller/controller.go b/cli/controller/controller.go index 0ca4dc5..05ba479 100644 --- a/cli/controller/controller.go +++ b/cli/controller/controller.go @@ -311,38 +311,34 @@ func (cc *Controller) ParseContext(ctx climax.Context) (*models.Feature, error) // the type is inferred for boolean/percentile only, and a string value is // rejected so the caller is forced to pass -type=string explicitly. func parseValue(val string, typ string) (interface{}, models.FeatureType, error) { + var v interface{} + var ft models.FeatureType + if typ != "" { - ft, ok := models.ParseFeatureType(typ) + var ok bool + ft, ok = models.ParseFeatureType(typ) if !ok { return nil, models.Invalid, errInvalidType } - v, err := models.ParseValueForType(val, ft) - - if err != nil { + var err error + if v, err = models.ParseValueForType(val, ft); err != nil { if ft == models.Boolean { return nil, models.Invalid, errInvalidBool } return nil, models.Invalid, errInvalidPercentile } + } else { + v, ft = models.ParseValueAndFeatureType(val) - if ft == models.Percentile { - if err := validatePercentile(v); err != nil { - return nil, models.Invalid, err - } + if ft == models.String { + return nil, models.Invalid, errTypeRequiredForString } - - return v, ft, nil - } - - v, ft := models.ParseValueAndFeatureType(val) - - if ft == models.String { - return nil, models.Invalid, errTypeRequiredForString } + // shared validation for both the explicit and inferred paths if ft == models.Percentile { if err := validatePercentile(v); err != nil { return nil, models.Invalid, err From d5e51da623b3af58877070f0d5648d0f3bdba3c4 Mon Sep 17 00:00:00 2001 From: Alex Karev Date: Mon, 8 Jun 2026 18:55:23 -0700 Subject: [PATCH 5/7] clean --- models/feature.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/models/feature.go b/models/feature.go index b8985d0..72ea1f9 100644 --- a/models/feature.go +++ b/models/feature.go @@ -49,9 +49,7 @@ func ParseValueAndFeatureType(v string) (interface{}, FeatureType) { return i, Percentile } - // Any value that is not a bool or a number is treated as a free-form - // string feature, so this function never returns Invalid. The Invalid - // constant is retained for callers that compare against it explicitly. + // Default to a free-form string for any non-boolean, non-numeric value. return v, String } From d40c01a1f0d452f79bc1fe660a64466cb2710c9e Mon Sep 17 00:00:00 2001 From: Alex Karev Date: Tue, 9 Jun 2026 16:37:05 -0700 Subject: [PATCH 6/7] clean --- cli/controller/controller.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/cli/controller/controller.go b/cli/controller/controller.go index 05ba479..3180e57 100644 --- a/cli/controller/controller.go +++ b/cli/controller/controller.go @@ -306,10 +306,10 @@ func (cc *Controller) ParseContext(ctx climax.Context) (*models.Feature, error) return f, nil } -// parseValue resolves the value and feature type for a `set` command. When -// `typ` is provided the value is parsed strictly for that type; when omitted -// the type is inferred for boolean/percentile only, and a string value is -// rejected so the caller is forced to pass -type=string explicitly. +// parseValue resolves the value and feature type for a `set` command. +// - When -type is provided, the value is parsed strictly for that type. +// - When -type is omitted, the type is inferred: boolean and percentile are +// accepted, but an inferred string is rejected (see below). func parseValue(val string, typ string) (interface{}, models.FeatureType, error) { var v interface{} var ft models.FeatureType @@ -324,15 +324,20 @@ func parseValue(val string, typ string) (interface{}, models.FeatureType, error) var err error if v, err = models.ParseValueForType(val, ft); err != nil { - if ft == models.Boolean { + switch ft { + case models.Boolean: return nil, models.Invalid, errInvalidBool + case models.Percentile: + return nil, models.Invalid, errInvalidPercentile + default: + return nil, models.Invalid, err } - - return nil, models.Invalid, errInvalidPercentile } } else { v, ft = models.ParseValueAndFeatureType(val) + // An inferred string is ambiguous (e.g. a typo'd bool/number), so + // require the caller to opt in explicitly with -type=string. if ft == models.String { return nil, models.Invalid, errTypeRequiredForString } From 52a77133b253518aebb992f4c1a69d13d75eabd8 Mon Sep 17 00:00:00 2001 From: Alex Karev Date: Wed, 10 Jun 2026 15:53:52 -0700 Subject: [PATCH 7/7] trim whitespaces in strings --- cli/controller/controller.go | 3 +++ cli/controller/controller_test.go | 12 ++++++++++++ client/mock/mock_client.go | 6 ++++++ client/mock/mock_client_test.go | 4 ++++ models/feature.go | 13 +++++++++++-- models/feature_test.go | 13 ++++++++++++- 6 files changed, 48 insertions(+), 3 deletions(-) diff --git a/cli/controller/controller.go b/cli/controller/controller.go index 3180e57..b87ac08 100644 --- a/cli/controller/controller.go +++ b/cli/controller/controller.go @@ -27,6 +27,7 @@ var ( errTypeRequiredForString = errors.New("-type=string is required for non-numeric, non-boolean values") errInvalidBool = errors.New("invalid -value for boolean. use -value=[true|false]") errInvalidPercentile = errors.New("invalid -value for percentile. must be a number") + errEmptyString = errors.New("invalid -value for string. must not be empty or whitespace-only") errInvalidRange = errors.New("invalid -value for percentile. use -value=[0.0-1.0]") errNameRequired = errors.New("-name is required") ) @@ -329,6 +330,8 @@ func parseValue(val string, typ string) (interface{}, models.FeatureType, error) return nil, models.Invalid, errInvalidBool case models.Percentile: return nil, models.Invalid, errInvalidPercentile + case models.String: + return nil, models.Invalid, errEmptyString default: return nil, models.Invalid, err } diff --git a/cli/controller/controller_test.go b/cli/controller/controller_test.go index e652d27..0badd98 100644 --- a/cli/controller/controller_test.go +++ b/cli/controller/controller_test.go @@ -152,6 +152,13 @@ func TestParseContextExplicitTypes(t *testing.T) { assert.Equal(t, models.String, f.FeatureType) assert.Equal(t, "true", f.Value) + // surrounding whitespace is trimmed from string values + f, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "padded", "value": " debug ", "type": "string", + })) + assert.NoError(t, err) + assert.Equal(t, "debug", f.Value) + f, err = ctl.ParseContext(parseCtx(map[string]string{ "name": "flag", "value": "true", "type": "boolean", })) @@ -189,6 +196,11 @@ func TestParseContextExplicitTypeErrors(t *testing.T) { "name": "flag", "value": "2.0", "type": "percentile", })) assert.Equal(t, errInvalidRange, err) + + _, err = ctl.ParseContext(parseCtx(map[string]string{ + "name": "flag", "value": " ", "type": "string", + })) + assert.Equal(t, errEmptyString, err) } func TestParseContextInference(t *testing.T) { diff --git a/client/mock/mock_client.go b/client/mock/mock_client.go index 8953871..3d87517 100644 --- a/client/mock/mock_client.go +++ b/client/mock/mock_client.go @@ -51,6 +51,12 @@ func (d *Client) SetPercentileFeature(feature string, val float64) { d.MergeScopes() } +// SetStringFeature set a string feature to an arbitrary value +func (d *Client) SetStringFeature(feature string, val string) { + d.Client.FeatureMap().Dcdr.Defaults()[feature] = val + d.MergeScopes() +} + // EnablePercentileFeature set a percentile feature to true func (d *Client) EnablePercentileFeature(feature string) { d.SetPercentileFeature(feature, 1.0) diff --git a/client/mock/mock_client_test.go b/client/mock/mock_client_test.go index e8944fe..8e9d9c8 100644 --- a/client/mock/mock_client_test.go +++ b/client/mock/mock_client_test.go @@ -20,4 +20,8 @@ func TestMockClient(t *testing.T) { assert.True(t, d.IsAvailableForID("float", 2)) d.DisablePercentileFeature("float") assert.False(t, d.IsAvailableForID("float", 8)) + + d.SetStringFeature("str", "debug") + assert.Equal(t, "debug", d.GetString("str")) + assert.Equal(t, "", d.GetString("missing")) } diff --git a/models/feature.go b/models/feature.go index 72ea1f9..0b4d468 100644 --- a/models/feature.go +++ b/models/feature.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "strconv" + "strings" ) // Features a Feature result set @@ -71,7 +72,9 @@ func ParseFeatureType(s string) (FeatureType, bool) { // ParseValueForType parses `val` into the concrete value for an explicitly // provided `ft`. Unlike ParseValueAndFeatureType it does not infer the type, -// so a String stores the literal value verbatim (e.g. "true" or "0.5"). +// so a String stores the literal value (e.g. "true" or "0.5"). String values +// are trimmed of surrounding whitespace, mirroring how strconv rejects padded +// bool/percentile input, so the stored value is canonical for all consumers. func ParseValueForType(val string, ft FeatureType) (interface{}, error) { switch ft { case Boolean: @@ -79,7 +82,13 @@ func ParseValueForType(val string, ft FeatureType) (interface{}, error) { case Percentile: return strconv.ParseFloat(val, 64) case String: - return val, nil + v := strings.TrimSpace(val) + + if v == "" { + return nil, fmt.Errorf("empty string value") + } + + return v, nil default: return nil, fmt.Errorf("unsupported feature type %q", ft) } diff --git a/models/feature_test.go b/models/feature_test.go index 300b725..46b2363 100644 --- a/models/feature_test.go +++ b/models/feature_test.go @@ -66,12 +66,23 @@ func TestParseValueForType(t *testing.T) { _, err = ParseValueForType("notanumber", Percentile) assert.Error(t, err) - // String stores the literal value verbatim, even bool/number-looking input. + // String stores the literal value, even bool/number-looking input. for _, s := range []string{"debug", "true", "0.5"} { v, err = ParseValueForType(s, String) assert.NoError(t, err) assert.Equal(t, s, v) } + + // String values are trimmed of surrounding whitespace. + v, err = ParseValueForType(" debug\t", String) + assert.NoError(t, err) + assert.Equal(t, "debug", v) + + // Empty or whitespace-only strings are rejected. + for _, s := range []string{"", " ", "\t\n"} { + _, err = ParseValueForType(s, String) + assert.Error(t, err, "%q", s) + } } func TestMarshaling(t *testing.T) {