diff --git a/README.md b/README.md index d6b2d8f..1e04f2c 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,21 @@ 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. 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" +``` + +```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 +364,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 +487,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 (--type=string is required) +dcdr set -n min-log-level -v debug -t string +``` + +```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/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 9a59f74..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] -comment "flag description"`, + Usage: `set -name flag_name -value [-type boolean|percentile|string] -comment "flag description"`, Help: ` @@ -121,6 +121,13 @@ func (c *CLI) Commands() []climax.Command { 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 1e60829..b87ac08 100644 --- a/cli/controller/controller.go +++ b/cli/controller/controller.go @@ -23,9 +23,13 @@ import ( const filePerms = 0775 var ( - errInvalidFeatureType = errors.New("invalid -value format. use -value=[0.0-1.0] or [true|false]") - 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]") + 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") ) // Controller handler for CLI commands @@ -277,6 +281,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,16 +293,11 @@ func (cc *Controller) ParseContext(ctx climax.Context) (*models.Feature, error) var ft models.FeatureType if val != "" { - v, ft = models.ParseValueAndFeatureType(val) - - if ft == models.Invalid { - return nil, errInvalidFeatureType - } + var err error + v, ft, err = parseValue(val, typ) - if ft == models.Percentile { - if v.(float64) > 1.0 || v.(float64) < 0 { - return nil, errInvalidRange - } + if err != nil { + return nil, err } } @@ -306,3 +306,61 @@ 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 -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 + + if typ != "" { + var ok bool + ft, ok = models.ParseFeatureType(typ) + + if !ok { + return nil, models.Invalid, errInvalidType + } + + var err error + if v, err = models.ParseValueForType(val, ft); err != nil { + switch ft { + case models.Boolean: + 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 + } + } + } 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 + } + } + + // shared validation for both the explicit and inferred paths + if ft == models.Percentile { + 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 25f995d..0badd98 100644 --- a/cli/controller/controller_test.go +++ b/cli/controller/controller_test.go @@ -125,3 +125,105 @@ 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) + + // 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", + })) + 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, errInvalidPercentile, err) + + _, err = ctl.ParseContext(parseCtx(map[string]string{ + "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) { + 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.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..bcc7145 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,24 @@ 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")) +} + +// 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/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 9c7f3f1..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 @@ -21,6 +22,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 +50,48 @@ func ParseValueAndFeatureType(v string) (interface{}, FeatureType) { return i, Percentile } - return nil, Invalid + // Default to a free-form string for any non-boolean, non-numeric value. + 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 (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: + return strconv.ParseBool(val) + case Percentile: + return strconv.ParseFloat(val, 64) + case String: + 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) + } } // Feature KV model for feature flags @@ -89,6 +133,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 +160,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..46b2363 100644 --- a/models/feature_test.go +++ b/models/feature_test.go @@ -14,6 +14,75 @@ 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 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, 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) { @@ -40,4 +109,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()) }