From 5aa321dbde9a00cf58539c018c3cca4943b021ab Mon Sep 17 00:00:00 2001 From: wln32 <49137144+wln32@users.noreply.github.com> Date: Wed, 25 Sep 2024 19:22:15 +0800 Subject: [PATCH] fix(util/gconv): unstable converting when there is an external attribute with the same name as the internal structure (#3799) --- util/gconv/gconv_struct.go | 16 +- util/gconv/gconv_z_unit_issue_test.go | 261 ++++++++++++++++++ util/gconv/gconv_z_unit_struct_test.go | 34 +++ .../structcache/structcache_cached.go | 53 ++-- .../structcache_cached_field_info.go | 30 +- .../structcache_cached_struct_info.go | 62 +++-- 6 files changed, 388 insertions(+), 68 deletions(-) diff --git a/util/gconv/gconv_struct.go b/util/gconv/gconv_struct.go index 8fca143fd..71fd76cfd 100644 --- a/util/gconv/gconv_struct.go +++ b/util/gconv/gconv_struct.go @@ -206,7 +206,7 @@ func doStruct( ); err != nil { return err } - if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameField) > 0 { if err = setOtherSameNameField( cachedFieldInfo, paramsValue, pointerReflectValue, paramKeyToAttrMap, ); err != nil { @@ -238,9 +238,9 @@ func setOtherSameNameField( paramKeyToAttrMap map[string]string, ) (err error) { // loop the same field name of all sub attributes. - for i := range cachedFieldInfo.OtherSameNameFieldIndex { - fieldValue := cachedFieldInfo.GetOtherFieldReflectValueFrom(structValue, i) - if err = bindVarToStructField(fieldValue, srcValue, cachedFieldInfo, paramKeyToAttrMap); err != nil { + for _, otherFieldInfo := range cachedFieldInfo.OtherSameNameField { + fieldValue := cachedFieldInfo.GetOtherFieldReflectValueFrom(structValue, otherFieldInfo.FieldIndexes) + if err = bindVarToStructField(fieldValue, srcValue, otherFieldInfo, paramKeyToAttrMap); err != nil { return err } } @@ -283,7 +283,7 @@ func bindStructWithLoopParamsMap( return err } // handle same field name in nested struct. - if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameField) > 0 { if err = setOtherSameNameField(cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap); err != nil { return err } @@ -318,7 +318,7 @@ func bindStructWithLoopParamsMap( return err } // handle same field name in nested struct. - if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameField) > 0 { if err = setOtherSameNameField( cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap, ); err != nil { @@ -366,7 +366,7 @@ func bindStructWithLoopFieldInfos( return err } // handle same field name in nested struct. - if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameField) > 0 { if err = setOtherSameNameField( cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap, ); err != nil { @@ -399,7 +399,7 @@ func bindStructWithLoopFieldInfos( return err } // handle same field name in nested struct. - if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameField) > 0 { if err = setOtherSameNameField( cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap, ); err != nil { diff --git a/util/gconv/gconv_z_unit_issue_test.go b/util/gconv/gconv_z_unit_issue_test.go index 3719ede4d..dbb361e28 100644 --- a/util/gconv/gconv_z_unit_issue_test.go +++ b/util/gconv/gconv_z_unit_issue_test.go @@ -446,3 +446,264 @@ func Test_Issue3789(t *testing.T) { t.Assert(dest.ThirdID, uint64(3)) }) } + +// https://github.com/gogf/gf/issues/3797 +func Test_Issue3797(t *testing.T) { + type Option struct { + F1 int + F2 string + } + type Rule struct { + ID int64 `json:"id"` + Rule []*Option `json:"rule"` + } + type Res1 struct { + g.Meta + Rule + } + gtest.C(t, func(t *gtest.T) { + var r = &Rule{ + ID: 100, + } + var res = &Res1{} + for i := 0; i < 10000; i++ { + err := gconv.Scan(r, res) + t.AssertNil(err) + t.Assert(res.ID, 100) + t.AssertEQ(res.Rule.Rule, nil) + } + }) +} + +// https://github.com/gogf/gf/issues/3800 +func Test_Issue3800(t *testing.T) { + // might be random assignment in converting, + // it here so runs multiple times to reproduce the issue. + for i := 0; i < 1000; i++ { + doTestIssue3800(t) + } +} + +func doTestIssue3800(t *testing.T) { + type NullID string + + type StructA struct { + Superior string `json:"superior"` + UpdatedTick int `json:"updated_tick"` + } + type StructB struct { + Superior *NullID `json:"superior"` + UpdatedTick *int `json:"updated_tick"` + } + + type StructC struct { + Superior string `json:"superior"` + UpdatedTick int `json:"updated_tick"` + } + type StructD struct { + StructC + Superior *NullID `json:"superior"` + UpdatedTick *int `json:"updated_tick"` + } + + type StructE struct { + Superior string `json:"superior"` + UpdatedTick int `json:"updated_tick"` + } + type StructF struct { + Superior *NullID `json:"superior"` + UpdatedTick *int `json:"updated_tick"` + StructE + } + + type StructG struct { + Superior string `json:"superior"` + UpdatedTick int `json:"updated_tick"` + } + type StructH struct { + Superior *string `json:"superior"` + UpdatedTick *int `json:"updated_tick"` + StructG + } + + type StructI struct { + Master struct { + Superior *NullID `json:"superior"` + UpdatedTick int `json:"updated_tick"` + } `json:"master"` + } + type StructJ struct { + StructA + Superior *NullID `json:"superior"` + UpdatedTick *int `json:"updated_tick"` + } + + type StructK struct { + Master struct { + Superior *NullID `json:"superior"` + UpdatedTick int `json:"updated_tick"` + } `json:"master"` + } + type StructL struct { + Superior *NullID `json:"superior"` + UpdatedTick *int `json:"updated_tick"` + StructA + } + + // case 0 + // NullID should not be initialized. + gtest.C(t, func(t *gtest.T) { + structA := g.Map{ + "UpdatedTick": 10, + } + structB := StructB{} + err := gconv.Scan(structA, &structB) + t.AssertNil(err) + t.AssertNil(structB.Superior) + t.Assert(*structB.UpdatedTick, structA["UpdatedTick"]) + }) + + // case 1 + gtest.C(t, func(t *gtest.T) { + structA := StructA{ + Superior: "superior100", + UpdatedTick: 20, + } + structB := StructB{} + err := gconv.Scan(structA, &structB) + t.AssertNil(err) + t.Assert(*structB.Superior, structA.Superior) + }) + + // case 2 + gtest.C(t, func(t *gtest.T) { + structA1 := StructA{ + Superior: "100", + UpdatedTick: 20, + } + structB1 := StructB{} + err := gconv.Scan(structA1, &structB1) + t.AssertNil(err) + t.Assert(*structB1.Superior, structA1.Superior) + t.Assert(*structB1.UpdatedTick, structA1.UpdatedTick) + }) + + // case 3 + gtest.C(t, func(t *gtest.T) { + structC := StructC{ + Superior: "superior100", + UpdatedTick: 20, + } + structD := StructD{} + err := gconv.Scan(structC, &structD) + t.AssertNil(err) + t.Assert(structD.StructC.Superior, structC.Superior) + t.Assert(*structD.Superior, structC.Superior) + t.Assert(*structD.UpdatedTick, structC.UpdatedTick) + }) + + // case 4 + gtest.C(t, func(t *gtest.T) { + structC1 := StructC{ + Superior: "100", + UpdatedTick: 20, + } + structD1 := StructD{} + err := gconv.Scan(structC1, &structD1) + t.AssertNil(err) + t.Assert(structD1.StructC.Superior, structC1.Superior) + t.Assert(structD1.StructC.UpdatedTick, structC1.UpdatedTick) + t.Assert(*structD1.Superior, structC1.Superior) + t.Assert(*structD1.UpdatedTick, structC1.UpdatedTick) + }) + + // case 5 + gtest.C(t, func(t *gtest.T) { + structE := StructE{ + Superior: "superior100", + UpdatedTick: 20, + } + structF := StructF{} + err := gconv.Scan(structE, &structF) + t.AssertNil(err) + t.Assert(structF.StructE.Superior, structE.Superior) + t.Assert(structF.StructE.UpdatedTick, structE.UpdatedTick) + t.Assert(*structF.Superior, structE.Superior) + t.Assert(*structF.UpdatedTick, structE.UpdatedTick) + }) + + // case 6 + gtest.C(t, func(t *gtest.T) { + structE1 := StructE{ + Superior: "100", + UpdatedTick: 20, + } + structF1 := StructF{} + err := gconv.Scan(structE1, &structF1) + t.AssertNil(err) + t.Assert(*structF1.Superior, structE1.Superior) + t.Assert(*structF1.UpdatedTick, structE1.UpdatedTick) + t.Assert(structF1.StructE.Superior, structE1.Superior) + t.Assert(structF1.StructE.UpdatedTick, structE1.UpdatedTick) + }) + + // case 7 + gtest.C(t, func(t *gtest.T) { + structG := StructG{ + Superior: "superior100", + UpdatedTick: 20, + } + structH := StructH{} + err := gconv.Scan(structG, &structH) + t.AssertNil(err) + t.Assert(*structH.Superior, structG.Superior) + t.Assert(*structH.UpdatedTick, structG.UpdatedTick) + t.Assert(structH.StructG.Superior, structG.Superior) + t.Assert(structH.StructG.UpdatedTick, structG.UpdatedTick) + }) + + // case 8 + gtest.C(t, func(t *gtest.T) { + structG1 := StructG{ + Superior: "100", + UpdatedTick: 20, + } + structH1 := StructH{} + err := gconv.Scan(structG1, &structH1) + t.AssertNil(err) + t.Assert(*structH1.Superior, structG1.Superior) + t.Assert(*structH1.UpdatedTick, structG1.UpdatedTick) + t.Assert(structH1.StructG.Superior, structG1.Superior) + t.Assert(structH1.StructG.UpdatedTick, structG1.UpdatedTick) + }) + + // case 9 + gtest.C(t, func(t *gtest.T) { + structI := StructI{} + xxx := NullID("superior100") + structI.Master.Superior = &xxx + structI.Master.UpdatedTick = 30 + structJ := StructJ{} + err := gconv.Scan(structI.Master, &structJ) + t.AssertNil(err) + t.Assert(*structJ.Superior, structI.Master.Superior) + t.Assert(*structJ.UpdatedTick, structI.Master.UpdatedTick) + t.Assert(structJ.StructA.Superior, structI.Master.Superior) + t.Assert(structJ.StructA.UpdatedTick, structI.Master.UpdatedTick) + }) + + // case 10 + gtest.C(t, func(t *gtest.T) { + structK := StructK{} + yyy := NullID("superior100") + structK.Master.Superior = &yyy + structK.Master.UpdatedTick = 40 + structL := StructL{} + err := gconv.Scan(structK.Master, &structL) + t.AssertNil(err) + t.Assert(*structL.Superior, structK.Master.Superior) + t.Assert(*structL.UpdatedTick, structK.Master.UpdatedTick) + t.Assert(structL.StructA.Superior, structK.Master.Superior) + t.Assert(structL.StructA.UpdatedTick, structK.Master.UpdatedTick) + }) +} diff --git a/util/gconv/gconv_z_unit_struct_test.go b/util/gconv/gconv_z_unit_struct_test.go index a5fdcf3d0..1dcb190c2 100644 --- a/util/gconv/gconv_z_unit_struct_test.go +++ b/util/gconv/gconv_z_unit_struct_test.go @@ -7,6 +7,7 @@ package gconv_test import ( + "strconv" "testing" "time" @@ -182,6 +183,39 @@ func TestStruct(t *testing.T) { }) } +func TestStructDuplicateField(t *testing.T) { + gtest.C(t, func(t *gtest.T) { + m := map[string]any{ + "ID": 100, + } + type Nested1 struct { + ID string + } + type Nested2 struct { + ID uint + } + type Nested3 struct { + ID int + } + type Dest struct { + ID int + Nested1 + Nested2 + Nested3 + } + var ( + err error + dest = new(Dest) + ) + err = gconv.Struct(m, dest) + t.AssertNil(err) + t.Assert(dest.ID, m["ID"]) + t.Assert(dest.Nested1.ID, strconv.Itoa(m["ID"].(int))) + t.Assert(dest.Nested2.ID, m["ID"]) + t.Assert(dest.Nested3.ID, m["ID"]) + }) +} + func TestStructErr(t *testing.T) { gtest.C(t, func(t *gtest.T) { type Score struct { diff --git a/util/gconv/internal/structcache/structcache_cached.go b/util/gconv/internal/structcache/structcache_cached.go index 576dfeed0..1c7d4cf32 100644 --- a/util/gconv/internal/structcache/structcache_cached.go +++ b/util/gconv/internal/structcache/structcache_cached.go @@ -43,24 +43,22 @@ func RegisterCommonConverter(commonConverter CommonConverter) { } // GetCachedStructInfo retrieves or parses and returns a cached info for certain struct type. -func GetCachedStructInfo( - structType reflect.Type, - priorityTag string, -) *CachedStructInfo { +// The given `structType` should be type of struct. +func GetCachedStructInfo(structType reflect.Type, priorityTag string) *CachedStructInfo { if structType.Kind() != reflect.Struct { return nil } // check if it has been cached. - structInfo, ok := getCachedConvertStructInfo(structType) + cachedStructInfo, ok := getCachedConvertStructInfo(structType) if ok { // directly returns the cached struct info if already exists. - return structInfo + return cachedStructInfo } // else create one. // it parses and generates a cache info for given struct type. - structInfo = &CachedStructInfo{ + cachedStructInfo = &CachedStructInfo{ tagOrFiledNameToFieldInfoMap: make(map[string]*CachedFieldInfo), } var ( @@ -72,14 +70,14 @@ func GetCachedStructInfo( } else { priorityTagArray = gtag.StructTagPriority } - parseStruct(structType, parentIndex, structInfo, priorityTagArray) - setCachedConvertStructInfo(structType, structInfo) - return structInfo + parseStructToCachedStructInfo(structType, parentIndex, cachedStructInfo, priorityTagArray) + storeCachedStructInfo(structType, cachedStructInfo) + return cachedStructInfo } -func setCachedConvertStructInfo(structType reflect.Type, info *CachedStructInfo) { +func storeCachedStructInfo(structType reflect.Type, cachedStructInfo *CachedStructInfo) { // Temporarily enabled as an experimental feature - cachedStructsInfoMap.Store(structType, info) + cachedStructsInfoMap.Store(structType, cachedStructInfo) } func getCachedConvertStructInfo(structType reflect.Type) (*CachedStructInfo, bool) { @@ -91,10 +89,12 @@ func getCachedConvertStructInfo(structType reflect.Type) (*CachedStructInfo, boo return nil, false } -func parseStruct( +// parseStructToCachedStructInfo parses given struct reflection type and stores its fields info into given CachedStructInfo. +// It stores nothing into CachedStructInfo if given struct reflection type has no fields. +func parseStructToCachedStructInfo( structType reflect.Type, fieldIndexes []int, - structInfo *CachedStructInfo, + cachedStructInfo *CachedStructInfo, priorityTagArray []string, ) { var ( @@ -119,13 +119,6 @@ func parseStruct( copyFieldIndexes := make([]int, len(fieldIndexes)) copy(copyFieldIndexes, fieldIndexes) - // Do not directly use append(fieldIndexes, i) - // When the structure is nested deeply, it may lead to bugs, - // which are caused by the slice expansion mechanism - // So it is necessary to allocate a separate index for each field - // See details https://github.com/gogf/gf/issues/3789 - structInfo.AddField(structField, append(copyFieldIndexes, i), priorityTagArray) - // normal basic attributes. if structField.Anonymous { // handle struct attributes, it might be struct/*struct embedded.. @@ -135,10 +128,22 @@ func parseStruct( if fieldType.Kind() != reflect.Struct { continue } - if structField.Tag != "" { - // TODO: If it's an anonymous field with a tag, doesn't it need to be recursive? + // Skip the embedded structure of the 0 field, + if fieldType.NumField() == 0 { + continue } - parseStruct(fieldType, append(copyFieldIndexes, i), structInfo, priorityTagArray) + if structField.Tag != "" { + // Do not add anonymous structures without tags + cachedStructInfo.AddField(structField, append(copyFieldIndexes, i), priorityTagArray) + } + parseStructToCachedStructInfo(fieldType, append(copyFieldIndexes, i), cachedStructInfo, priorityTagArray) + continue } + // Do not directly use append(fieldIndexes, i) + // When the structure is nested deeply, it may lead to bugs, + // which are caused by the slice expansion mechanism + // So it is necessary to allocate a separate index for each field + // See details https://github.com/gogf/gf/issues/3789 + cachedStructInfo.AddField(structField, append(copyFieldIndexes, i), priorityTagArray) } } diff --git a/util/gconv/internal/structcache/structcache_cached_field_info.go b/util/gconv/internal/structcache/structcache_cached_field_info.go index 98432e64b..1009295e2 100644 --- a/util/gconv/internal/structcache/structcache_cached_field_info.go +++ b/util/gconv/internal/structcache/structcache_cached_field_info.go @@ -56,25 +56,22 @@ type CachedFieldInfoBase struct { // StructField is the type info of this field. StructField reflect.StructField - // OtherSameNameFieldIndex holds the sub attributes of the same field name. + // OtherSameNameField stores fields with the same name and type or different types of nested structures. + // // For example: - // type Name struct{ - // LastName string - // FirstName string + // type ID struct{ + // ID1 string + // ID2 int // } - // type User struct{ - // Name - // LastName string - // FirstName string + // type Card struct{ + // ID + // ID1 uint64 + // ID2 int64 // } // - // As the `LastName` in `User`, its internal attributes: - // FieldIndexes = []int{0,1} - // // item length 1, as there's only one repeat item with the same field name. - // OtherSameNameFieldIndex = [][]int{[]int{1}} - // - // In value assignment, the value will be assigned to index {0,1} and {1}. - OtherSameNameFieldIndex [][]int + // We will cache each ID1 and ID2 separately, + // even if their types are different and their indexes are different + OtherSameNameField []*CachedFieldInfo // ConvertFunc is the converting function for this field. ConvertFunc func(from any, to reflect.Value) @@ -112,8 +109,7 @@ func (cfi *CachedFieldInfo) GetFieldReflectValueFrom(structValue reflect.Value) // by `fieldLevel`, which is used for directly value assignment. // // Note that, the input parameter `structValue` might be initialized internally. -func (cfi *CachedFieldInfo) GetOtherFieldReflectValueFrom(structValue reflect.Value, fieldLevel int) reflect.Value { - fieldIndex := cfi.OtherSameNameFieldIndex[fieldLevel] +func (cfi *CachedFieldInfo) GetOtherFieldReflectValueFrom(structValue reflect.Value, fieldIndex []int) reflect.Value { if len(fieldIndex) == 1 { // no nested struct. return structValue.Field(fieldIndex[0]) diff --git a/util/gconv/internal/structcache/structcache_cached_struct_info.go b/util/gconv/internal/structcache/structcache_cached_struct_info.go index 4a56b206c..c04ea8ac1 100644 --- a/util/gconv/internal/structcache/structcache_cached_struct_info.go +++ b/util/gconv/internal/structcache/structcache_cached_struct_info.go @@ -41,20 +41,10 @@ func (csi *CachedStructInfo) GetFieldInfo(fieldName string) *CachedFieldInfo { func (csi *CachedStructInfo) AddField(field reflect.StructField, fieldIndexes []int, priorityTags []string) { alreadyExistFieldInfo, ok := csi.tagOrFiledNameToFieldInfoMap[field.Name] if !ok { - priorityTagAndFieldName := csi.genPriorityTagAndFieldName(field, priorityTags) - newFieldInfoBase := &CachedFieldInfoBase{ - IsCommonInterface: checkTypeIsCommonInterface(field), - StructField: field, - FieldIndexes: fieldIndexes, - ConvertFunc: csi.genFieldConvertFunc(field.Type.String()), - IsCustomConvert: csi.checkTypeHasCustomConvert(field.Type), - PriorityTagAndFieldName: priorityTagAndFieldName, - RemoveSymbolsFieldName: utils.RemoveSymbols(field.Name), - } - newFieldInfoBase.LastFuzzyKey.Store(field.Name) - for _, tagOrFieldName := range priorityTagAndFieldName { + cachedFieldInfo := csi.makeCachedFieldInfo(field, fieldIndexes, priorityTags) + for _, tagOrFieldName := range cachedFieldInfo.PriorityTagAndFieldName { newFieldInfo := &CachedFieldInfo{ - CachedFieldInfoBase: newFieldInfoBase, + CachedFieldInfoBase: cachedFieldInfo.CachedFieldInfoBase, IsField: tagOrFieldName == field.Name, } csi.tagOrFiledNameToFieldInfoMap[tagOrFieldName] = newFieldInfo @@ -64,14 +54,48 @@ func (csi *CachedStructInfo) AddField(field reflect.StructField, fieldIndexes [] } return } - if alreadyExistFieldInfo.OtherSameNameFieldIndex == nil { - alreadyExistFieldInfo.OtherSameNameFieldIndex = make([][]int, 0, 2) + // If the field name and type are the same + if alreadyExistFieldInfo.StructField.Type == field.Type { + alreadyExistFieldInfo.OtherSameNameField = append( + alreadyExistFieldInfo.OtherSameNameField, + csi.copyCachedInfoWithFieldIndexes(alreadyExistFieldInfo, fieldIndexes), + ) + return } - alreadyExistFieldInfo.OtherSameNameFieldIndex = append( - alreadyExistFieldInfo.OtherSameNameFieldIndex, - fieldIndexes, + // If the types are different, some information needs to be reset + alreadyExistFieldInfo.OtherSameNameField = append( + alreadyExistFieldInfo.OtherSameNameField, + csi.makeCachedFieldInfo(field, fieldIndexes, priorityTags), ) - return +} + +// copyCachedInfoWithFieldIndexes copies and returns a new CachedFieldInfo based on given CachedFieldInfo, but different +// FieldIndexes. Mainly used for copying fields with the same name and type. +func (csi *CachedStructInfo) copyCachedInfoWithFieldIndexes(cfi *CachedFieldInfo, fieldIndexes []int) *CachedFieldInfo { + base := CachedFieldInfoBase{} + base = *cfi.CachedFieldInfoBase + base.FieldIndexes = fieldIndexes + return &CachedFieldInfo{ + CachedFieldInfoBase: &base, + } +} + +func (csi *CachedStructInfo) makeCachedFieldInfo( + field reflect.StructField, fieldIndexes []int, priorityTags []string, +) *CachedFieldInfo { + base := &CachedFieldInfoBase{ + IsCommonInterface: checkTypeIsCommonInterface(field), + StructField: field, + FieldIndexes: fieldIndexes, + ConvertFunc: csi.genFieldConvertFunc(field.Type.String()), + IsCustomConvert: csi.checkTypeHasCustomConvert(field.Type), + PriorityTagAndFieldName: csi.genPriorityTagAndFieldName(field, priorityTags), + RemoveSymbolsFieldName: utils.RemoveSymbols(field.Name), + } + base.LastFuzzyKey.Store(field.Name) + return &CachedFieldInfo{ + CachedFieldInfoBase: base, + } } func (csi *CachedStructInfo) genFieldConvertFunc(fieldType string) (convertFunc func(from any, to reflect.Value)) {