From db1ecc8c3a56c1bec299e177655bfebb51287af4 Mon Sep 17 00:00:00 2001 From: Aleksandr Maus Date: Thu, 15 Feb 2024 08:50:45 -0500 Subject: [PATCH] Fix numeric configuration keys handling (#198) * Fix numeric configuration keys handling * Update NOTICE.txt --- getset.go | 4 +- merge.go | 45 +++++++++---------- opts.go | 26 +++++++++++ path.go | 35 +++++++++++---- reify.go | 112 +++++++++++++++++++++++----------------------- ucfg.go | 4 +- ucfg_test.go | 90 +++++++++++++++++++++++++++++++++++++ variables.go | 20 ++++----- variables_test.go | 6 +-- 9 files changed, 237 insertions(+), 105 deletions(-) diff --git a/getset.go b/getset.go index a0a71f9..b6b456d 100644 --- a/getset.go +++ b/getset.go @@ -252,7 +252,7 @@ func (c *Config) SetChild(name string, idx int, value *Config, opts ...Option) e // getField supports the options: PathSep, Env, Resolve, ResolveEnv func (c *Config) getField(name string, idx int, opts *options) (value, Error) { - p := parsePathIdx(name, opts.pathSep, idx) + p := parsePathIdx(name, idx, opts) v, err := p.GetValue(c, opts) if err != nil { return v, err @@ -267,7 +267,7 @@ func (c *Config) getField(name string, idx int, opts *options) (value, Error) { // setField supports the options: PathSep, MetaData func (c *Config) setField(name string, idx int, v value, options []Option) Error { opts := makeOptions(options) - p := parsePathIdx(name, opts.pathSep, idx) + p := parsePathIdx(name, idx, opts) err := p.SetValue(c, opts, v) if err != nil { diff --git a/merge.go b/merge.go index 11b5252..1b9b10a 100644 --- a/merge.go +++ b/merge.go @@ -34,34 +34,33 @@ import ( // Merge supports the options: PathSep, MetaData, StructTag, VarExp, ReplaceValues, AppendValues, PrependValues // // Merge uses the type-dependent default encodings: -// - Boolean values are encoded as booleans. -// - Integer are encoded as int64 values, unsigned integer values as uint64 and -// floats as float64 values. -// - Strings are copied into string values. -// If the VarExp is set, string fields will be parsed into -// variable expansion expressions. The expression can reference any -// other setting by absolute name. -// - Array and slices are copied into new Config objects with index accessors only. -// - Struct values and maps with key type string are encoded as Config objects with -// named field accessors. -// - Config objects will be copied and added to the current hierarchy. +// - Boolean values are encoded as booleans. +// - Integer are encoded as int64 values, unsigned integer values as uint64 and +// floats as float64 values. +// - Strings are copied into string values. +// If the VarExp is set, string fields will be parsed into +// variable expansion expressions. The expression can reference any +// other setting by absolute name. +// - Array and slices are copied into new Config objects with index accessors only. +// - Struct values and maps with key type string are encoded as Config objects with +// named field accessors. +// - Config objects will be copied and added to the current hierarchy. // // The `config` struct tag (configurable via StructTag option) can be used to // set the field name and enable additional merging settings per field: // -// // field appears in Config as key "myName" -// Field int `config:"myName"` +// // field appears in Config as key "myName" +// Field int `config:"myName"` // -// // field appears in sub-Config "mySub" as key "myName" (requires PathSep(".")) -// Field int `config:"mySub.myName"` +// // field appears in sub-Config "mySub" as key "myName" (requires PathSep(".")) +// Field int `config:"mySub.myName"` // -// // field is processed as if keys are part of outer struct (type can be a -// // struct, a slice, an array, a map or of type *Config) -// Field map[string]interface{} `config:",inline"` -// -// // field is ignored by Merge -// Field string `config:",ignore"` +// // field is processed as if keys are part of outer struct (type can be a +// // struct, a slice, an array, a map or of type *Config) +// Field map[string]interface{} `config:",inline"` // +// // field is ignored by Merge +// Field string `config:",ignore"` // // Returns an error if merging fails to normalize and validate the from value. // If duplicate setting names are detected in the input, merging fails as well. @@ -379,7 +378,7 @@ func normalizeSetField( return err } - p := parsePath(name, opts.pathSep) + p := parsePathWithOpts(name, opts) old, err := p.GetValue(cfg, opts) if err != nil { if err.Reason() != ErrMissing { @@ -515,7 +514,7 @@ func normalizeString(ctx context, opts *options, str string) (value, Error) { return newString(ctx, opts.meta, str), nil } - varexp, err := parseSplice(str, opts.pathSep) + varexp, err := parseSplice(str, opts.pathSep, opts.maxIdx, opts.enableNumKeys) if err != nil { return nil, raiseParseSplice(ctx, opts.meta, err) } diff --git a/opts.go b/opts.go index f6957cc..2508b42 100644 --- a/opts.go +++ b/opts.go @@ -25,6 +25,10 @@ import ( "github.com/elastic/go-ucfg/parse" ) +// Some sane value for the index fields such as input.0.foo +// in order to protect against cases where user specifies input.9223372036854.foo for the key +const defaultMaxIdx = 1024 + // Option type implementing additional options to be passed // to go-ucfg library functions. type Option func(*options) @@ -39,6 +43,9 @@ type options struct { varexp bool noParse bool + maxIdx int64 // Max index field value allowed + enableNumKeys bool // Enables numeric keys, example "123" + configValueHandling configHandling fieldHandlingTree *fieldHandlingTree @@ -118,6 +125,24 @@ func Resolve(fn func(name string) (string, parse.Config, error)) Option { } } +// MaxIdx overwrites max index field value allowed. +// By default it is limited to defaultMaxIdx value. +func MaxIdx(maxIdx int64) Option { + return func(o *options) { + o.maxIdx = maxIdx + } +} + +// EnableNumKeys enables numeric keys, such as "1234" in the configuration. +// The numeric key values are converted to array's index otherwise by default. +// This feature is disabled by default for backwards compatibility. +// This is useful when it's needed to support and preserve the configuration numeric string keys. +func EnableNumKeys(enableNumKeys bool) Option { + return func(o *options) { + o.enableNumKeys = enableNumKeys + } +} + // ResolveEnv option adds a look up callback looking up values in the available // OS environment variables. var ResolveEnv Option = doResolveEnv @@ -236,6 +261,7 @@ func makeOptions(opts []Option) *options { pathSep: "", // no separator by default parsed: map[string]spliceValue{}, activeFields: newFieldSet(nil), + maxIdx: defaultMaxIdx, } for _, opt := range opts { opt(&o) diff --git a/path.go b/path.go index ff7e630..a89bb47 100644 --- a/path.go +++ b/path.go @@ -43,15 +43,15 @@ type idxField struct { i int } -func parsePathIdx(in, sep string, idx int) cfgPath { +func parsePathIdx(in string, idx int, opts *options) cfgPath { if in == "" { return cfgPath{ - sep: sep, + sep: opts.pathSep, fields: []field{idxField{idx}}, } } - p := parsePath(in, sep) + p := parsePathWithOpts(in, opts) if idx >= 0 { p.fields = append(p.fields, idxField{idx}) } @@ -59,25 +59,42 @@ func parsePathIdx(in, sep string, idx int) cfgPath { return p } -func parsePath(in, sep string) cfgPath { +func parsePath(in, sep string, maxIdx int64, enableNumKeys bool) cfgPath { if sep == "" { return cfgPath{ sep: sep, - fields: []field{parseField(in)}, + fields: []field{parseField(in, maxIdx, enableNumKeys)}, } } elems := strings.Split(in, sep) fields := make([]field, 0, len(elems)) + // If property is the name with separators, for example "inputs.0.i" + // fall back to original implementation + if len(elems) > 1 { + enableNumKeys = false + } for _, elem := range elems { - fields = append(fields, parseField(elem)) + fields = append(fields, parseField(elem, maxIdx, enableNumKeys)) } return cfgPath{fields: fields, sep: sep} } -func parseField(in string) field { - if idx, err := strconv.ParseInt(in, 0, 64); err == nil { - return idxField{int(idx)} +func parsePathWithOpts(in string, opts *options) cfgPath { + return parsePath(in, opts.pathSep, opts.maxIdx, opts.enableNumKeys) +} + +func parseField(in string, maxIdx int64, enableNumKeys bool) field { + // If numeric keys are not enabled, fallback to the original implementation + if !enableNumKeys { + idx, err := strconv.ParseInt(in, 0, 64) + // Limit index value to the configurable max. + // If the idx > opts.maxIdx treat it as a regular named field. + // This preserves the current behavour for small index fields values (<= opts.maxIdx) + // and prevents large memory allocations or OOM if the string is large numeric value + if err == nil && idx <= int64(maxIdx) { + return idxField{int(idx)} + } } return namedField{in} } diff --git a/reify.go b/reify.go index 29c0e5d..ee415f1 100644 --- a/reify.go +++ b/reify.go @@ -33,55 +33,55 @@ import ( // value implements the Unpacker interface. Otherwise, Unpack tries to convert // the internal value into the target type: // -// # Primitive types +// # Primitive types // -// bool: requires setting of type bool or string which parses into a -// boolean value (true, false, on, off) -// int(8, 16, 32, 64): requires any number type convertible to int or a string -// parsing to int. Fails if the target value would overflow. -// uint(8, 16, 32, 64): requires any number type convertible to int or a string -// parsing to int. Fails if the target value is negative or would overflow. -// float(32, 64): requires any number type convertible to float or a string -// parsing to float. Fails if the target value is negative or would overflow. -// string: requires any primitive value which is serialized into a string. +// bool: requires setting of type bool or string which parses into a +// boolean value (true, false, on, off) +// int(8, 16, 32, 64): requires any number type convertible to int or a string +// parsing to int. Fails if the target value would overflow. +// uint(8, 16, 32, 64): requires any number type convertible to int or a string +// parsing to int. Fails if the target value is negative or would overflow. +// float(32, 64): requires any number type convertible to float or a string +// parsing to float. Fails if the target value is negative or would overflow. +// string: requires any primitive value which is serialized into a string. // -// # Special types: +// # Special types: // -// time.Duration: requires a number setting converted to seconds or a string -// parsed into time.Duration via time.ParseDuration. -// *regexp.Regexp: requires a string being compiled into a regular expression -// using regexp.Compile. -// *Config: requires a Config object to be stored by pointer into the target -// value. Can be used to capture a sub-Config without interpreting -// the settings yet. +// time.Duration: requires a number setting converted to seconds or a string +// parsed into time.Duration via time.ParseDuration. +// *regexp.Regexp: requires a string being compiled into a regular expression +// using regexp.Compile. +// *Config: requires a Config object to be stored by pointer into the target +// value. Can be used to capture a sub-Config without interpreting +// the settings yet. // -// # Arrays/Slices: +// # Arrays/Slices: // -// Requires a Config object with indexed entries. Named entries will not be -// unpacked into the Array/Slice. Primitive values will be handled like arrays -// of length 1. +// Requires a Config object with indexed entries. Named entries will not be +// unpacked into the Array/Slice. Primitive values will be handled like arrays +// of length 1. // -// # Map +// # Map // -// Requires a Config object with all named top-level entries being unpacked into -// the map. +// Requires a Config object with all named top-level entries being unpacked into +// the map. // -// # Struct +// # Struct // -// Requires a Config object. All named values in the Config object will be unpacked -// into the struct its fields, if the name is available in the struct. -// A field its name is set using the `config` struct tag (configured by StructTag) -// If tag is missing or no field name is configured in the tag, the field name -// itself will be used. -// If the tag sets the `,ignore` flag, the field will not be overwritten. -// If the tag sets the `,inline` or `,squash` flag, Unpack will apply the current -// configuration namespace to the fields. -// If the tag option `replace` is configured, arrays and *ucfg.Config -// convertible fields are replaced by the new values. -// If the tag options `append` or `prepend` is used, arrays will be merged by -// appending/prepending the new array contents. -// The struct tag options `replace`, `append`, and `prepend` overwrites the -// global value merging strategy (e.g. ReplaceValues, AppendValues, ...) for all sub-fields. +// Requires a Config object. All named values in the Config object will be unpacked +// into the struct its fields, if the name is available in the struct. +// A field its name is set using the `config` struct tag (configured by StructTag) +// If tag is missing or no field name is configured in the tag, the field name +// itself will be used. +// If the tag sets the `,ignore` flag, the field will not be overwritten. +// If the tag sets the `,inline` or `,squash` flag, Unpack will apply the current +// configuration namespace to the fields. +// If the tag option `replace` is configured, arrays and *ucfg.Config +// convertible fields are replaced by the new values. +// If the tag options `append` or `prepend` is used, arrays will be merged by +// appending/prepending the new array contents. +// The struct tag options `replace`, `append`, and `prepend` overwrites the +// global value merging strategy (e.g. ReplaceValues, AppendValues, ...) for all sub-fields. // // When unpacking into a map, primitive, or struct Unpack will call InitDefaults if // the type implements the Initializer interface. The Initializer interface is not supported @@ -109,13 +109,13 @@ import ( // Struct field validators are set using the `validate` tag (configurable by // ValidatorTag). Default validators options are: // -// required: check value is set and not empty -// nonzero: check numeric value != 0 or string/slice not being empty -// positive: check numeric value >= 0 -// min=: check numeric value >= . If target type is time.Duration, -// can be a duration. -// max=: check numeric value <= . If target type is time.Duration, -// can be a duration. +// required: check value is set and not empty +// nonzero: check numeric value != 0 or string/slice not being empty +// positive: check numeric value >= 0 +// min=: check numeric value >= . If target type is time.Duration, +// can be a duration. +// max=: check numeric value <= . If target type is time.Duration, +// can be a duration. // // If a config value is not the convertible to the target type, or overflows the // target type, Unpack will abort immediately and return the appropriate error. @@ -126,14 +126,14 @@ import ( // When unpacking into an interface{} value, Unpack will store a value of one of // these types in the value: // -// bool for boolean values -// int64 for signed integer values -// uint64 for unsigned integer values -// float64 for floating point values -// string for string values -// []interface{} for list-only Config objects -// map[string]interface{} for Config objects -// nil for pointers if key has a nil value +// bool for boolean values +// int64 for signed integer values +// uint64 for unsigned integer values +// float64 for floating point values +// string for string values +// []interface{} for list-only Config objects +// map[string]interface{} for Config objects +// nil for pointers if key has a nil value func (c *Config) Unpack(to interface{}, options ...Option) error { opts := makeOptions(options) @@ -311,7 +311,7 @@ func reifyGetField( to reflect.Value, fieldType reflect.Type, ) Error { - p := parsePath(name, opts.opts.pathSep) + p := parsePathWithOpts(name, opts.opts) value, err := p.GetValue(cfg, opts.opts) if err != nil { if err.Reason() != ErrMissing { diff --git a/ucfg.go b/ucfg.go index f84cf90..b38f363 100644 --- a/ucfg.go +++ b/ucfg.go @@ -132,7 +132,7 @@ func (c *Config) GetFields() []string { // value is found in the middle of the traversal. func (c *Config) Has(name string, idx int, options ...Option) (bool, error) { opts := makeOptions(options) - p := parsePathIdx(name, opts.pathSep, idx) + p := parsePathIdx(name, idx, opts) return p.Has(c, opts) } @@ -167,7 +167,7 @@ func (c *Config) Remove(name string, idx int, options ...Option) (bool, error) { opts.resolvers = nil opts.noParse = true - p := parsePathIdx(name, opts.pathSep, idx) + p := parsePathIdx(name, idx, opts) return p.Remove(c, opts) } diff --git a/ucfg_test.go b/ucfg_test.go index cb43a2b..c84e03b 100644 --- a/ucfg_test.go +++ b/ucfg_test.go @@ -489,3 +489,93 @@ func TestRemove(t *testing.T) { }) } } + +func TestNewFromWithMaxIdxAndEnableNumKeys(t *testing.T) { + baseOptions := []Option{ + PathSep("."), + ResolveEnv, + VarExp, + } + + tests := []struct { + name string + opts []Option + in, want map[string]interface{} + }{ + { + name: "original", + in: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + want: map[string]interface{}{"a": []interface{}{nil, nil, nil, nil, map[string]interface{}{"foo": "bar"}}}, // original behavour + }, + { + name: "original with num keys", + opts: []Option{EnableNumKeys(true)}, + in: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + want: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + }, + { + name: "max idx", + opts: []Option{MaxIdx(3)}, // This should preserve numeric key since the max smaller that + in: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + want: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + }, + { + name: "max idx with num keys", + opts: []Option{MaxIdx(3), EnableNumKeys(true)}, + in: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + want: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + }, + { + name: "max idx equals", + opts: []Option{MaxIdx(4)}, // This should fallback on the old behavour + in: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + want: map[string]interface{}{"a": []interface{}{nil, nil, nil, nil, map[string]interface{}{"foo": "bar"}}}, + }, + { + name: "max idx equals with num keys", + opts: []Option{MaxIdx(4), EnableNumKeys(true)}, // This should fallback on the old behavour + in: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + want: map[string]interface{}{ + "a": map[string]interface{}{"4": map[string]interface{}{"foo": "bar"}}, + }, + }, + } + + for _, tc := range tests { + // Combine base opts with test opts + opts := append(baseOptions, tc.opts...) + + // Create config from test map + cfg, err := NewFrom(tc.in, opts...) + if err != nil { + t.Fatal(err) + } + + // Unpack config into map + var m map[string]interface{} + err = cfg.Unpack(&m, opts...) + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, tc.want, m) + } +} diff --git a/variables.go b/variables.go index 791bee3..b559d6e 100644 --- a/variables.go +++ b/variables.go @@ -232,7 +232,7 @@ func (e *expansionSingle) eval(cfg *Config, opts *options) (string, error) { return "", err } - ref := newReference(parsePath(path, e.pathSep)) + ref := newReference(parsePathWithOpts(path, opts)) return ref.eval(cfg, opts) } @@ -241,7 +241,7 @@ func (e *expansionDefault) eval(cfg *Config, opts *options) (string, error) { if err != nil || path == "" { return e.right.eval(cfg, opts) } - ref := newReference(parsePath(path, e.pathSep)) + ref := newReference(parsePath(path, e.pathSep, opts.maxIdx, opts.enableNumKeys)) v, err := ref.eval(cfg, opts) if err != nil || v == "" { return e.right.eval(cfg, opts) @@ -255,7 +255,7 @@ func (e *expansionAlt) eval(cfg *Config, opts *options) (string, error) { return "", nil } - ref := newReference(parsePath(path, e.pathSep)) + ref := newReference(parsePath(path, e.pathSep, opts.maxIdx, opts.enableNumKeys)) tmp, err := ref.resolve(cfg, opts) if err != nil || tmp == nil { return "", nil @@ -267,7 +267,7 @@ func (e *expansionAlt) eval(cfg *Config, opts *options) (string, error) { func (e *expansionErr) eval(cfg *Config, opts *options) (string, error) { path, err := e.left.eval(cfg, opts) if err == nil && path != "" { - ref := newReference(parsePath(path, e.pathSep)) + ref := newReference(parsePath(path, e.pathSep, opts.maxIdx, opts.enableNumKeys)) str, err := ref.eval(cfg, opts) if err == nil && str != "" { return str, nil @@ -281,7 +281,7 @@ func (e *expansionErr) eval(cfg *Config, opts *options) (string, error) { return "", errors.New(errStr) } -func (st parseState) finalize(pathSep string) (varEvaler, error) { +func (st parseState) finalize(pathSep string, maxIdx int64, enableNumKeys bool) (varEvaler, error) { if !st.isvar { return nil, errors.New("fatal: processing non-variable state") } @@ -298,7 +298,7 @@ func (st parseState) finalize(pathSep string) (varEvaler, error) { if len(pieces) == 1 { if str, ok := pieces[0].(constExp); ok { - return newReference(parsePath(string(str), pathSep)), nil + return newReference(parsePath(string(str), pathSep, maxIdx, enableNumKeys)), nil } } @@ -334,7 +334,7 @@ func makeOpExpansion(l, r varEvaler, op, pathSep string) varEvaler { panic(fmt.Sprintf("Unknown operator: %v", op)) } -func parseSplice(in, pathSep string) (varEvaler, error) { +func parseSplice(in, pathSep string, maxIdx int64, enableNumKeys bool) (varEvaler, error) { lex, errs := lexer(in) drainLex := func() { for range lex { @@ -344,7 +344,7 @@ func parseSplice(in, pathSep string) (varEvaler, error) { // drain lexer on return so go-routine won't leak defer drainLex() - pieces, perr := parseVarExp(lex, pathSep) + pieces, perr := parseVarExp(lex, pathSep, maxIdx, enableNumKeys) if perr != nil { return nil, perr } @@ -448,7 +448,7 @@ func lexer(in string) (<-chan token, <-chan error) { return lex, errors } -func parseVarExp(lex <-chan token, pathSep string) (varEvaler, error) { +func parseVarExp(lex <-chan token, pathSep string, maxIdx int64, enableNumKeys bool) (varEvaler, error) { stack := []parseState{{st: stLeft}} // parser loop @@ -458,7 +458,7 @@ func parseVarExp(lex <-chan token, pathSep string) (varEvaler, error) { stack = append(stack, parseState{st: stLeft, isvar: true}) case tokClose: // finalize and pop state - piece, err := stack[len(stack)-1].finalize(pathSep) + piece, err := stack[len(stack)-1].finalize(pathSep, maxIdx, enableNumKeys) stack = stack[:len(stack)-1] if err != nil { return nil, err diff --git a/variables_test.go b/variables_test.go index dcd966b..6f54004 100644 --- a/variables_test.go +++ b/variables_test.go @@ -26,7 +26,7 @@ import ( func TestVarExpParserSuccess(t *testing.T) { str := func(s string) varEvaler { return constExp(s) } - ref := func(s string) *reference { return newReference(parsePath(s, ".")) } + ref := func(s string) *reference { return newReference(parsePath(s, ".", defaultMaxIdx, false)) } cat := func(e ...varEvaler) *splice { return &splice{e} } nested := func(n ...varEvaler) varEvaler { return &expansionSingle{&splice{n}, "."} @@ -73,7 +73,7 @@ func TestVarExpParserSuccess(t *testing.T) { for _, test := range tests { t.Run(fmt.Sprintf("%s %s", test.title, test.exp), func(t *testing.T) { - actual, err := parseSplice(test.exp, ".") + actual, err := parseSplice(test.exp, ".", defaultMaxIdx, false) if assert.NoError(t, err) { assert.Equal(t, test.expected, actual) } @@ -89,7 +89,7 @@ func TestVarExpParseErrors(t *testing.T) { for _, test := range tests { t.Run(fmt.Sprintf("test %v: %v", test.title, test.exp), func(t *testing.T) { - res, err := parseSplice(test.exp, ".") + res, err := parseSplice(test.exp, ".", defaultMaxIdx, false) assert.True(t, err != nil) assert.Error(t, err, fmt.Sprintf("result: %v, error: %v", res, err)) })