From a93f6b49f59113333445421f4eaeaa52ae623b01 Mon Sep 17 00:00:00 2001 From: Cory Bennett Date: Wed, 18 Jun 2025 15:33:47 +0000 Subject: [PATCH 1/3] fix merge struct case for two different options --- figtree.go | 61 +++++++++++++++++++++++++++++++++++++++++++++---- figtree_test.go | 20 ++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/figtree.go b/figtree.go index 0ca8476..fd1ec5e 100644 --- a/figtree.go +++ b/figtree.go @@ -493,6 +493,49 @@ func inlineField(field reflect.StructField) bool { return false } +func mergeOptions(a reflect.Type, b reflect.Type) (reflect.Value, bool) { + if !a.Implements(reflect.TypeOf((*option)(nil)).Elem()) { + // try to see if pointer type of A implements option + a := reflect.New(a).Type() + if !a.Implements(reflect.TypeOf((*option)(nil)).Elem()) { + return reflect.Value{}, false + } + } + if !b.Implements(reflect.TypeOf((*option)(nil)).Elem()) { + // try to see if pointer type of B implements option + b := reflect.New(b).Type() + if !b.Implements(reflect.TypeOf((*option)(nil)).Elem()) { + return reflect.Value{}, false + } + } + + av, ok := a.FieldByName("Value") + if !ok { + return reflect.Value{}, false + } + avt := av.Type + + bv, ok := b.FieldByName("Value") + if !ok { + return reflect.Value{}, false + } + bvt := bv.Type + + if bvt.AssignableTo(avt) { + return reflect.New(a).Elem(), true + } + if avt.AssignableTo(bvt) { + return reflect.New(b).Elem(), true + } + if bvt.ConvertibleTo(avt) { + return reflect.New(a).Elem(), true + } + if avt.ConvertibleTo(bvt) { + return reflect.New(b).Elem(), true + } + return reflect.Value{}, false +} + func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { foundFields := map[string]reflect.StructField{} for i := 0; i < len(values); i++ { @@ -515,11 +558,19 @@ func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { if f, ok := foundFields[field.Name]; ok { if f.Type.Kind() == reflect.Struct && field.Type.Kind() == reflect.Struct { if fName, fieldName := f.Type.Name(), field.Type.Name(); fName == "" || fieldName == "" || fName != fieldName { - // we have 2 fields with the same name and they are both structs, so we need - // to merge the existing struct with the new one in case they are different - newval := m.makeMergeStruct(reflect.New(f.Type).Elem(), reflect.New(field.Type).Elem()).Elem() - f.Type = newval.Type() - foundFields[field.Name] = f + // do we have two options? + if newval, ok := mergeOptions(f.Type, field.Type); ok { + // we have two fields with the same name and they are both options, so we need + // to merge the existing option with the new one in case they are different + f.Type = newval.Type() + foundFields[field.Name] = f + } else { + // we have 2 fields with the same name and they are both structs, so we need + // to merge the existing struct with the new one in case they are different + newval := m.makeMergeStruct(reflect.New(f.Type).Elem(), reflect.New(field.Type).Elem()).Elem() + f.Type = newval.Type() + foundFields[field.Name] = f + } } } // field already found, skip diff --git a/figtree_test.go b/figtree_test.go index c661578..4134a72 100644 --- a/figtree_test.go +++ b/figtree_test.go @@ -3443,3 +3443,23 @@ func TestMergeWithDstStructArg(t *testing.T) { err := Merge(dest, src) require.Error(t, err) } + +func TestMergeOptionAny(t *testing.T) { + input1 := struct { + StructField Option[string] `yaml:"struct-field"` + }{} + + input2 := struct { + StructField Option[any] `yaml:"struct-field"` + }{} + + merge := MakeMergeStruct(input1, input2) + require.Equal(t, &struct { + StructField Option[any] `yaml:"struct-field"` + }{}, merge) + + merge = MakeMergeStruct(input2, input1) + require.Equal(t, &struct { + StructField Option[any] `yaml:"struct-field"` + }{}, merge) +} From b12e4da56f3e4e9a677ee2b5c7f8a6c1a1490ea3 Mon Sep 17 00:00:00 2001 From: Cory Bennett Date: Wed, 18 Jun 2025 17:11:50 +0000 Subject: [PATCH 2/3] when merging structs preserve tags * prefer `figtree:"name=value"` tag for field names. * prevent duplicate conflicts between yaml names and struct field names. --- figtree.go | 144 +++++++++++++++++++++++++++++++++++++++++++----- figtree_test.go | 73 ++++++++++++++++++++---- go.mod | 2 +- go.sum | 4 +- 4 files changed, 193 insertions(+), 30 deletions(-) diff --git a/figtree.go b/figtree.go index fd1ec5e..f83526b 100644 --- a/figtree.go +++ b/figtree.go @@ -400,10 +400,11 @@ func camelCase(name string) string { } type Merger struct { - sourceFile string - preserveMap map[string]struct{} - Config ConfigOptions `json:"config,omitempty" yaml:"config,omitempty"` - ignore []string + sourceFile string + preserveMap map[string]struct{} + preserveTags map[string]struct{} + Config ConfigOptions `json:"config,omitempty" yaml:"config,omitempty"` + ignore []string } type MergeOption func(*Merger) @@ -422,10 +423,23 @@ func PreserveMap(keys ...string) MergeOption { } } +func PreserveTags(names ...string) MergeOption { + return func(m *Merger) { + for _, tag := range names { + m.preserveTags[tag] = struct{}{} + } + } +} + func NewMerger(options ...MergeOption) *Merger { m := &Merger{ sourceFile: "merge", preserveMap: make(map[string]struct{}), + preserveTags: map[string]struct{}{ + "json": {}, + "yaml": {}, + "figtree": {}, + }, } for _, opt := range options { opt(m) @@ -536,6 +550,50 @@ func mergeOptions(a reflect.Type, b reflect.Type) (reflect.Value, bool) { return reflect.Value{}, false } +func (m *Merger) mergeTags(a reflect.StructField, b reflect.StructField) string { + allTags := []string{} + for k := range m.preserveTags { + allTags = append(allTags, k) + } + var resultTag []string + sort.Strings(allTags) + for _, tag := range allTags { + aTag, aOk := a.Tag.Lookup(tag) + bTag, bOk := b.Tag.Lookup(tag) + switch { + case aOk && bOk: + // if both defined, pick the first one + resultTag = append(resultTag, fmt.Sprintf("%s:%q", tag, aTag)) + case aOk: + resultTag = append(resultTag, fmt.Sprintf("%s:%q", tag, aTag)) + case bOk: + resultTag = append(resultTag, fmt.Sprintf("%s:%q", tag, bTag)) + } + } + return strings.Join(resultTag, " ") +} + +// findField will look for the field in the map. It will first look for +// the field name as defined by CanonicalFieldName, then it will look for +// a field based on the yaml tag name. This helps ensure that we dont add +// duplicate fields based on either struct field name or the yaml tag name, +// both of which will cause failures. +func findField(fields map[string]reflect.StructField, field reflect.StructField) (key string, found reflect.StructField, ok bool) { + fieldName := CanonicalFieldName(field) + if f, found := fields[fieldName]; found { + return fieldName, f, true + } + + yamlName := yamlFieldName(field) + for k, f := range fields { + if yamlName == yamlFieldName(f) { + return k, f, true + } + } + + return "", reflect.StructField{}, false +} + func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { foundFields := map[string]reflect.StructField{} for i := 0; i < len(values); i++ { @@ -554,8 +612,21 @@ func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { } field.Name = CanonicalFieldName(field) + if fieldKey, f, ok := findField(foundFields, field); ok { + newTag := m.mergeTags(f, field) + if newTag != string(f.Tag) { + f.Tag = reflect.StructTag(newTag) + foundFields[fieldKey] = f + } + + // the canonical name is influenced by the `figtree` tag + // so make sure the merged field has the correct name after + // merging the tags. + if fieldName := CanonicalFieldName(f); f.Name != fieldName { + f.Name = fieldName + foundFields[fieldKey] = f + } - if f, ok := foundFields[field.Name]; ok { if f.Type.Kind() == reflect.Struct && field.Type.Kind() == reflect.Struct { if fName, fieldName := f.Type.Name(), field.Type.Name(); fName == "" || fieldName == "" || fName != fieldName { // do we have two options? @@ -563,13 +634,13 @@ func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { // we have two fields with the same name and they are both options, so we need // to merge the existing option with the new one in case they are different f.Type = newval.Type() - foundFields[field.Name] = f + foundFields[fieldKey] = f } else { // we have 2 fields with the same name and they are both structs, so we need // to merge the existing struct with the new one in case they are different newval := m.makeMergeStruct(reflect.New(f.Type).Elem(), reflect.New(field.Type).Elem()).Elem() f.Type = newval.Type() - foundFields[field.Name] = f + foundFields[fieldKey] = f } } } @@ -608,14 +679,35 @@ func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { Type: t, Tag: reflect.StructTag(fmt.Sprintf(`json:"%s" yaml:"%s"`, key.String(), key.String())), } - if f, ok := foundFields[field.Name]; ok { + // the yaml and json names are just taken directly from the map key + if fieldKey, f, ok := findField(foundFields, field); ok { + newTag := m.mergeTags(f, field) + if newTag != string(f.Tag) { + f.Tag = reflect.StructTag(newTag) + foundFields[fieldKey] = f + } + // the canonical name is influenced by the `figtree` tag + // so make sure the merged field has the correct name after + // merging the tags. + if fieldName := CanonicalFieldName(f); f.Name != fieldName { + f.Name = fieldName + foundFields[fieldKey] = f + } if f.Type.Kind() == reflect.Struct && t.Kind() == reflect.Struct { if fName, tName := f.Type.Name(), t.Name(); fName == "" || tName == "" || fName != tName { - // we have 2 fields with the same name and they are both structs, so we need - // to merge the existig struct with the new one in case they are different - newval := m.makeMergeStruct(reflect.New(f.Type).Elem(), reflect.New(t).Elem()).Elem() - f.Type = newval.Type() - foundFields[field.Name] = f + // do we have two options? + if newval, ok := mergeOptions(f.Type, field.Type); ok { + // we have two fields with the same name and they are both options, so we need + // to merge the existing option with the new one in case they are different + f.Type = newval.Type() + foundFields[fieldKey] = f + } else { + // we have 2 fields with the same name and they are both structs, so we need + // to merge the existig struct with the new one in case they are different + newval := m.makeMergeStruct(reflect.New(f.Type).Elem(), reflect.New(t).Elem()).Elem() + f.Type = newval.Type() + foundFields[fieldKey] = f + } } } // field already found, skip @@ -627,8 +719,23 @@ func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { } fields := []reflect.StructField{} + seen := map[string]reflect.StructField{} + yamlSeen := map[string]reflect.StructField{} for _, value := range foundFields { fields = append(fields, value) + if prev, ok := seen[value.Name]; ok { + // we have a duplicate field name, this should not happen + // but if it does, we will just skip it + fmt.Fprintf(os.Stderr, "Duplicate field name %q in merge struct.\n\tOld: %s %s `%s`\n\tNew: %s %s `%s`\n", value.Name, prev.Name, prev.Type.String(), prev.Tag, value.Name, value.Type.String(), value.Tag) + } + seen[value.Name] = value + yamlName := yamlTagName(value.Tag) + if prev, ok := yamlSeen[yamlName]; yamlName != "" && ok { + // we have a duplicate field name, this should not happen + // but if it does, we will just skip it + fmt.Fprintf(os.Stderr, "Duplicate YAML tag %q in merge struct.\n\tOld: %s %s `%s`\n\tNew: %s %s `%s`\n", yamlName, prev.Name, prev.Type.String(), prev.Tag, value.Name, value.Type.String(), value.Tag) + } + yamlSeen[yamlName] = value } sort.Slice(fields, func(i, j int) bool { return fields[i].Name < fields[j].Name @@ -710,8 +817,8 @@ type ConfigOptions struct { Overwrite []string `json:"overwrite,omitempty" yaml:"overwrite,omitempty"` } -func yamlFieldName(sf reflect.StructField) string { - if tag, ok := sf.Tag.Lookup("yaml"); ok { +func yamlTagName(tag reflect.StructTag) string { + if tag, ok := tag.Lookup("yaml"); ok { // with yaml:"foobar,omitempty" // we just want to the "foobar" part parts := strings.Split(tag, ",") @@ -719,6 +826,13 @@ func yamlFieldName(sf reflect.StructField) string { return parts[0] } } + return "" +} + +func yamlFieldName(sf reflect.StructField) string { + if name := yamlTagName(sf.Tag); name != "" { + return name + } // guess the field name from reversing camel case // so "FooBar" becomes "foo-bar" parts := camelcase.Split(sf.Name) diff --git a/figtree_test.go b/figtree_test.go index 4134a72..f716302 100644 --- a/figtree_test.go +++ b/figtree_test.go @@ -591,15 +591,15 @@ func TestMergeStructWithMap(t *testing.T) { expected := struct { Map struct { - Mapkey string + Mapkey string `json:"mapkey" yaml:"mapkey"` Nullkey interface{} `json:"nullkey" yaml:"nullkey"` StructField string - } - Mapkey string + } `json:"map" yaml:"map"` + Mapkey string `json:"mapkey" yaml:"mapkey"` StructField string }{ Map: struct { - Mapkey string + Mapkey string `json:"mapkey" yaml:"mapkey"` Nullkey interface{} `json:"nullkey" yaml:"nullkey"` StructField string }{ @@ -647,15 +647,15 @@ func TestMergeStructWithMapArbitraryNaming(t *testing.T) { expected := struct { Map struct { - Mapkey string `yaml:"mapkey"` + Mapkey string `json:"mapkey" yaml:"mapkey"` Nullkey interface{} `json:"nullkey" yaml:"nullkey"` StructField string `yaml:"struct-field"` - } `yaml:"map"` - Mapkey string `yaml:"mapkey"` + } `json:"map" yaml:"map"` + Mapkey string `json:"mapkey" yaml:"mapkey"` StructField string `yaml:"struct-field"` }{ Map: struct { - Mapkey string `yaml:"mapkey"` + Mapkey string `json:"mapkey" yaml:"mapkey"` Nullkey interface{} `json:"nullkey" yaml:"nullkey"` StructField string `yaml:"struct-field"` }{ @@ -1069,7 +1069,9 @@ func TestMakeMergeStructWithDups(t *testing.T) { err = Merge(got, &s) require.NoError(t, err) - assert.Equal(t, &struct{ Mapkey string }{"mapval2"}, got) + assert.Equal(t, &struct { + Mapkey string `json:"mapkey" yaml:"mapkey"` + }{"mapval2"}, got) } func TestMakeMergeStructWithInline(t *testing.T) { @@ -1831,7 +1833,7 @@ func TestMergeBoolString(t *testing.T) { require.NoError(t, err) expected := &struct { - EnableThing BoolOption + EnableThing BoolOption `json:"enable-thing" yaml:"enable-thing"` }{BoolOption{Source: NewSource("merge"), Defined: true, Value: true}} assert.Equal(t, expected, dest) @@ -1853,7 +1855,7 @@ func TestMergeStringBool(t *testing.T) { require.NoError(t, err) expected := &struct { - EnableThing StringOption + EnableThing StringOption `json:"enable-thing" yaml:"enable-thing"` }{StringOption{Source: NewSource("merge"), Defined: true, Value: "true"}} assert.Equal(t, expected, dest) @@ -1875,7 +1877,7 @@ func TestMergeStringFloat64(t *testing.T) { require.NoError(t, err) expected := &struct { - SomeThing StringOption + SomeThing StringOption `json:"some-thing" yaml:"some-thing"` }{StringOption{Source: NewSource("merge"), Defined: true, Value: "42"}} assert.Equal(t, expected, dest) @@ -3463,3 +3465,50 @@ func TestMergeOptionAny(t *testing.T) { StructField Option[any] `yaml:"struct-field"` }{}, merge) } + +// TestMergeSameYAMLTag checks that merging two structs with the same YAML tag +// but different casing for the field name does not cause a panic or unexpected +// behavior. It ensures that the `figtree` tag is preserved correctly. +func TestMergeSameYAMLTag(t *testing.T) { + input1 := struct { + SomeID Option[string] `yaml:"some-id" figtree:"name=SomeID"` + }{} + + input2 := struct { + SomeId Option[string] `yaml:"some-id"` + }{} + + merge := MakeMergeStruct(input1, input2) + require.Equal(t, &struct { + SomeID Option[string] `figtree:"name=SomeID" yaml:"some-id"` + }{}, merge) + + merge = MakeMergeStruct(input2, input1) + require.Equal(t, &struct { + SomeID Option[string] `figtree:"name=SomeID" yaml:"some-id"` + }{}, merge) +} + +// TestMergeNameCollision checks that merging two maps with different +// input casing that will result in the same struct field name does not cause +// a panic or unexpected behavior. +func TestMergeNameCollision(t *testing.T) { + input1 := map[string]any{ + "some-key": "value1", + } + input2 := map[string]any{ + "SomeKey": "value2", + } + merge := MakeMergeStruct(input1, input2) + require.Equal(t, &struct { + SomeKey string `json:"some-key" yaml:"some-key"` + }{}, merge) + + // Reversing the order, they are merged in first-to-last prirority order + // on this causes `SomeKey` to be the json/yaml name (keys from maps + // are preserved by order) + merge = MakeMergeStruct(input2, input1) + require.Equal(t, &struct { + SomeKey string `json:"SomeKey" yaml:"SomeKey"` + }{}, merge) +} diff --git a/go.mod b/go.mod index 54bb486..a01544a 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/stretchr/testify v1.7.0 gopkg.in/alecthomas/kingpin.v2 v2.2.6 gopkg.in/op/go-logging.v1 v1.0.0-20160211212156-b2cb9fa56473 - gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b + gopkg.in/yaml.v3 v3.0.0-20220521103104-8f96da9f5d5e ) require ( diff --git a/go.sum b/go.sum index b9ec709..bcce27e 100644 --- a/go.sum +++ b/go.sum @@ -33,5 +33,5 @@ gopkg.in/op/go-logging.v1 v1.0.0-20160211212156-b2cb9fa56473 h1:6D+BvnJ/j6e222UW gopkg.in/op/go-logging.v1 v1.0.0-20160211212156-b2cb9fa56473/go.mod h1:N1eN2tsCx0Ydtgjl4cqmbRCsY4/+z4cYDeqwZTk6zog= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo= -gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.0-20220521103104-8f96da9f5d5e h1:3i3ny04XV6HbZ2N1oIBw1UBYATHAOpo4tfTF83JM3Z0= +gopkg.in/yaml.v3 v3.0.0-20220521103104-8f96da9f5d5e/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= From 1f6f21c717b79310b036f336a09994f00ebfc6cd Mon Sep 17 00:00:00 2001 From: Cory Bennett Date: Thu, 19 Jun 2025 02:46:15 +0000 Subject: [PATCH 3/3] update github workflows --- .github/workflows/build.yml | 12 +++++------- figtree.go | 11 +++-------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e94af64..a23cddc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -9,17 +9,15 @@ jobs: go: [ '1.18' ] os: [ 'ubuntu-latest' ] steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Setup go - uses: actions/setup-go@v2 + uses: actions/setup-go@v5 with: go-version: ${{ matrix.go }} - - uses: actions/cache@v2 + - uses: actions/cache@v4 with: path: ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} - restore-keys: | - ${{ runner.os }}-go- - name: Go Test run: go test -v ./... lint: @@ -27,10 +25,10 @@ jobs: runs-on: ubuntu-latest steps: - name: Setup go - uses: actions/setup-go@v2 + uses: actions/setup-go@v5 with: go-version: 1.18 - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: install golangci-lint run: go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.46.2 - name: run golangci-lint diff --git a/figtree.go b/figtree.go index f83526b..e3de322 100644 --- a/figtree.go +++ b/figtree.go @@ -561,10 +561,7 @@ func (m *Merger) mergeTags(a reflect.StructField, b reflect.StructField) string aTag, aOk := a.Tag.Lookup(tag) bTag, bOk := b.Tag.Lookup(tag) switch { - case aOk && bOk: - // if both defined, pick the first one - resultTag = append(resultTag, fmt.Sprintf("%s:%q", tag, aTag)) - case aOk: + case aOk: // if a has a tag or both have the tag, use a resultTag = append(resultTag, fmt.Sprintf("%s:%q", tag, aTag)) case bOk: resultTag = append(resultTag, fmt.Sprintf("%s:%q", tag, bTag)) @@ -725,15 +722,13 @@ func (m *Merger) makeMergeStruct(values ...reflect.Value) reflect.Value { fields = append(fields, value) if prev, ok := seen[value.Name]; ok { // we have a duplicate field name, this should not happen - // but if it does, we will just skip it - fmt.Fprintf(os.Stderr, "Duplicate field name %q in merge struct.\n\tOld: %s %s `%s`\n\tNew: %s %s `%s`\n", value.Name, prev.Name, prev.Type.String(), prev.Tag, value.Name, value.Type.String(), value.Tag) + panic(fmt.Sprintf("Duplicate field name %q in merge struct.\n\tOld: %s %s `%s`\n\tNew: %s %s `%s`\n", value.Name, prev.Name, prev.Type.String(), prev.Tag, value.Name, value.Type.String(), value.Tag)) } seen[value.Name] = value yamlName := yamlTagName(value.Tag) if prev, ok := yamlSeen[yamlName]; yamlName != "" && ok { // we have a duplicate field name, this should not happen - // but if it does, we will just skip it - fmt.Fprintf(os.Stderr, "Duplicate YAML tag %q in merge struct.\n\tOld: %s %s `%s`\n\tNew: %s %s `%s`\n", yamlName, prev.Name, prev.Type.String(), prev.Tag, value.Name, value.Type.String(), value.Tag) + panic(fmt.Sprintf("Duplicate YAML tag %q in merge struct.\n\tOld: %s %s `%s`\n\tNew: %s %s `%s`\n", yamlName, prev.Name, prev.Type.String(), prev.Tag, value.Name, value.Type.String(), value.Tag)) } yamlSeen[yamlName] = value }