From e4619fab31de5a2dc80551e6bd6e0e1d0481e68d Mon Sep 17 00:00:00 2001 From: wln32 Date: Mon, 23 Sep 2024 07:01:58 +0800 Subject: [PATCH 1/2] =?UTF-8?q?1.=E4=BF=AE=E5=A4=8D=E7=94=B1=E4=BA=8Eslice?= =?UTF-8?q?=E6=89=A9=E5=AE=B9=E6=9C=BA=E5=88=B6=E5=BC=95=E8=B5=B7=E7=9A=84?= =?UTF-8?q?bug=202.=E6=B7=BB=E5=8A=A0http=E5=92=8Cgconv=E7=9A=84=E6=B5=8B?= =?UTF-8?q?=E8=AF=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- net/ghttp/ghttp_z_unit_issue_test.go | 50 +++++++++++++++++++ util/gconv/gconv_z_unit_issue_test.go | 32 ++++++++++++ .../structcache/structcache_cached.go | 12 +++-- 3 files changed, 91 insertions(+), 3 deletions(-) diff --git a/net/ghttp/ghttp_z_unit_issue_test.go b/net/ghttp/ghttp_z_unit_issue_test.go index 3f56dd9525f..75991c57901 100644 --- a/net/ghttp/ghttp_z_unit_issue_test.go +++ b/net/ghttp/ghttp_z_unit_issue_test.go @@ -568,3 +568,53 @@ func Test_Issue3245(t *testing.T) { t.Assert(c.GetContent(ctx, "/hello?nickname=oldme"), expect) }) } + +type ItemSecondThird struct { + SecondID uint64 `json:"secondId,string"` + ThirdID uint64 `json:"thirdId,string"` +} +type ItemFirst struct { + ID uint64 `json:"id,string"` + ItemSecondThird +} +type ItemInput struct { + ItemFirst +} +type Issue3789Req struct { + g.Meta `path:"/hello" method:"GET"` + ItemInput +} +type Issue3789Res struct { + ItemInput +} + +type Issue3789 struct{} + +func (Issue3789) Say(ctx context.Context, req *Issue3789Req) (res *Issue3789Res, err error) { + g.Log().Debugf(ctx, `receive say: %+v`, req) + res = &Issue3789Res{ + ItemInput: req.ItemInput, + } + return +} + +// https://github.com/gogf/gf/issues/3789 +func TestIssue3789(t *testing.T) { + gtest.C(t, func(t *gtest.T) { + s := g.Server() + s.Use(ghttp.MiddlewareHandlerResponse) + s.Group("/", func(group *ghttp.RouterGroup) { + group.Bind( + new(Issue3789), + ) + }) + s.Start() + defer s.Shutdown() + time.Sleep(100 * time.Millisecond) + + c := g.Client() + c.SetPrefix(fmt.Sprintf("http://127.0.0.1:%d", s.GetListenedPort())) + expect := `{"code":0,"message":"","data":{"id":"0","secondId":"2","thirdId":"3"}}` + t.Assert(c.GetContent(ctx, "/hello?id=&secondId=2&thirdId=3"), expect) + }) +} diff --git a/util/gconv/gconv_z_unit_issue_test.go b/util/gconv/gconv_z_unit_issue_test.go index 442f15c8c8e..0aade8596df 100644 --- a/util/gconv/gconv_z_unit_issue_test.go +++ b/util/gconv/gconv_z_unit_issue_test.go @@ -414,3 +414,35 @@ func TestIssue3764(t *testing.T) { t.AssertEQ(*tt.FalsePtr, falseValue) }) } + +// https://github.com/gogf/gf/issues/3789 +func TestIssue3789(t *testing.T) { + type ItemSecondThird struct { + SecondID uint64 `json:"secondId,string"` + ThirdID uint64 `json:"thirdId,string"` + } + type ItemFirst struct { + ID uint64 `json:"id,string"` + ItemSecondThird + } + type ItemInput struct { + ItemFirst + } + type HelloReq struct { + g.Meta `path:"/hello" method:"GET"` + ItemInput + } + gtest.C(t, func(t *gtest.T) { + m := map[string]interface{}{ + "id": 1, + "secondId": 2, + "thirdId": 3, + } + var dest HelloReq + err := gconv.Scan(m, &dest) + t.AssertNil(err) + t.Assert(dest.ID, uint64(1)) + t.Assert(dest.SecondID, uint64(2)) + t.Assert(dest.ThirdID, uint64(3)) + }) +} diff --git a/util/gconv/internal/structcache/structcache_cached.go b/util/gconv/internal/structcache/structcache_cached.go index 553396774e4..110c58e888e 100644 --- a/util/gconv/internal/structcache/structcache_cached.go +++ b/util/gconv/internal/structcache/structcache_cached.go @@ -113,8 +113,14 @@ func parseStruct( continue } - // store field - structInfo.AddField(structField, append(fieldIndexes, i), priorityTagArray) + 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 { @@ -128,7 +134,7 @@ func parseStruct( if structField.Tag != "" { // TODO: If it's an anonymous field with a tag, doesn't it need to be recursive? } - parseStruct(fieldType, append(fieldIndexes, i), structInfo, priorityTagArray) + parseStruct(fieldType, append(copyFieldIndexes, i), structInfo, priorityTagArray) } } } From 1d984d4afd0fa3b78479149f33c22fb28108efe9 Mon Sep 17 00:00:00 2001 From: John Guo Date: Mon, 23 Sep 2024 18:12:53 +0800 Subject: [PATCH 2/2] some comment and variable name update for package gconv for more easy understanding (#5) * some comment and variable name update for package gconv for more easy understanding --- net/ghttp/ghttp_z_unit_issue_test.go | 2 +- util/gconv/gconv_struct.go | 152 +++++++++--------- .../structcache/structcache_cached.go | 4 + .../structcache_cached_field_info.go | 17 +- 4 files changed, 96 insertions(+), 79 deletions(-) diff --git a/net/ghttp/ghttp_z_unit_issue_test.go b/net/ghttp/ghttp_z_unit_issue_test.go index 75991c57901..35bf20ccd8b 100644 --- a/net/ghttp/ghttp_z_unit_issue_test.go +++ b/net/ghttp/ghttp_z_unit_issue_test.go @@ -591,7 +591,6 @@ type Issue3789Res struct { type Issue3789 struct{} func (Issue3789) Say(ctx context.Context, req *Issue3789Req) (res *Issue3789Res, err error) { - g.Log().Debugf(ctx, `receive say: %+v`, req) res = &Issue3789Res{ ItemInput: req.ItemInput, } @@ -608,6 +607,7 @@ func TestIssue3789(t *testing.T) { new(Issue3789), ) }) + s.SetDumpRouterMap(false) s.Start() defer s.Shutdown() time.Sleep(100 * time.Millisecond) diff --git a/util/gconv/gconv_struct.go b/util/gconv/gconv_struct.go index f7708347a80..8fca143fd21 100644 --- a/util/gconv/gconv_struct.go +++ b/util/gconv/gconv_struct.go @@ -80,7 +80,7 @@ func doStruct( paramsInterface interface{} // DO NOT use `params` directly as it might be type `reflect.Value` pointerReflectValue reflect.Value pointerReflectKind reflect.Kind - pointerElemReflectValue reflect.Value // The pointed element. + pointerElemReflectValue reflect.Value // The reflection value to struct element. ) if v, ok := params.(reflect.Value); ok { paramsReflectValue = v @@ -183,33 +183,37 @@ func doStruct( var ( // Indicates that those values have been used and cannot be reused. usedParamsKeyOrTagNameMap = structcache.GetUsedParamsKeyOrTagNameMapFromPool() + cachedFieldInfo *structcache.CachedFieldInfo + paramsValue interface{} ) defer structcache.PutUsedParamsKeyOrTagNameMapToPool(usedParamsKeyOrTagNameMap) // Firstly, search according to custom mapping rules. // If a possible direct assignment is found, reduce the number of subsequent map searches. for paramKey, fieldName := range paramKeyToAttrMap { - fieldInfo := cachedStructInfo.GetFieldInfo(fieldName) - if fieldInfo != nil { - if paramsValue, ok := paramsMap[paramKey]; ok { - fieldValue := fieldInfo.GetFieldReflectValue(pointerElemReflectValue) - if err = bindVarToStructField( - fieldValue, - paramsValue, - fieldInfo, - paramKeyToAttrMap, + paramsValue, ok = paramsMap[paramKey] + if !ok { + continue + } + cachedFieldInfo = cachedStructInfo.GetFieldInfo(fieldName) + if cachedFieldInfo != nil { + fieldValue := cachedFieldInfo.GetFieldReflectValueFrom(pointerElemReflectValue) + if err = bindVarToStructField( + fieldValue, + paramsValue, + cachedFieldInfo, + paramKeyToAttrMap, + ); err != nil { + return err + } + if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { + if err = setOtherSameNameField( + cachedFieldInfo, paramsValue, pointerReflectValue, paramKeyToAttrMap, ); err != nil { return err } - if len(fieldInfo.OtherSameNameFieldIndex) > 0 { - if err = setOtherSameNameField( - fieldInfo, paramsValue, pointerReflectValue, paramKeyToAttrMap, - ); err != nil { - return err - } - } - usedParamsKeyOrTagNameMap[paramKey] = struct{}{} } + usedParamsKeyOrTagNameMap[paramKey] = struct{}{} } } // Already done converting for given `paramsMap`. @@ -228,15 +232,15 @@ func doStruct( } func setOtherSameNameField( - fieldInfo *structcache.CachedFieldInfo, + cachedFieldInfo *structcache.CachedFieldInfo, srcValue any, structValue reflect.Value, paramKeyToAttrMap map[string]string, ) (err error) { // loop the same field name of all sub attributes. - for i := range fieldInfo.OtherSameNameFieldIndex { - fieldValue := fieldInfo.GetOtherFieldReflectValue(structValue, i) - if err = bindVarToStructField(fieldValue, srcValue, fieldInfo, paramKeyToAttrMap); err != nil { + for i := range cachedFieldInfo.OtherSameNameFieldIndex { + fieldValue := cachedFieldInfo.GetOtherFieldReflectValueFrom(structValue, i) + if err = bindVarToStructField(fieldValue, srcValue, cachedFieldInfo, paramKeyToAttrMap); err != nil { return err } } @@ -251,36 +255,36 @@ func bindStructWithLoopParamsMap( cachedStructInfo *structcache.CachedStructInfo, ) (err error) { var ( - fieldName string - fieldInfo *structcache.CachedFieldInfo - fuzzLastKey string - fieldValue reflect.Value - paramKey string - paramValue any - ok bool + fieldName string + cachedFieldInfo *structcache.CachedFieldInfo + fuzzLastKey string + fieldValue reflect.Value + paramKey string + paramValue any + ok bool ) for paramKey, paramValue = range paramsMap { if _, ok = usedParamsKeyOrTagNameMap[paramKey]; ok { continue } - fieldInfo = cachedStructInfo.GetFieldInfo(paramKey) - if fieldInfo != nil { - fieldName = fieldInfo.FieldName() + cachedFieldInfo = cachedStructInfo.GetFieldInfo(paramKey) + if cachedFieldInfo != nil { + fieldName = cachedFieldInfo.FieldName() // already converted using its field name? // the field name has the more priority than tag name. _, ok = usedParamsKeyOrTagNameMap[fieldName] - if ok && fieldInfo.IsField { + if ok && cachedFieldInfo.IsField { continue } - fieldValue = fieldInfo.GetFieldReflectValue(structValue) + fieldValue = cachedFieldInfo.GetFieldReflectValueFrom(structValue) if err = bindVarToStructField( - fieldValue, paramValue, fieldInfo, paramKeyToAttrMap, + fieldValue, paramValue, cachedFieldInfo, paramKeyToAttrMap, ); err != nil { return err } // handle same field name in nested struct. - if len(fieldInfo.OtherSameNameFieldIndex) > 0 { - if err = setOtherSameNameField(fieldInfo, paramValue, structValue, paramKeyToAttrMap); err != nil { + if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { + if err = setOtherSameNameField(cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap); err != nil { return err } } @@ -289,40 +293,40 @@ func bindStructWithLoopParamsMap( } // fuzzy matching. - for _, fieldInfo = range cachedStructInfo.FieldConvertInfos { - fieldName = fieldInfo.FieldName() + for _, cachedFieldInfo = range cachedStructInfo.FieldConvertInfos { + fieldName = cachedFieldInfo.FieldName() if _, ok = usedParamsKeyOrTagNameMap[fieldName]; ok { continue } - fuzzLastKey = fieldInfo.LastFuzzyKey.Load().(string) + fuzzLastKey = cachedFieldInfo.LastFuzzyKey.Load().(string) paramValue, ok = paramsMap[fuzzLastKey] if !ok { if strings.EqualFold( - fieldInfo.RemoveSymbolsFieldName, utils.RemoveSymbols(paramKey), + cachedFieldInfo.RemoveSymbolsFieldName, utils.RemoveSymbols(paramKey), ) { paramValue, ok = paramsMap[paramKey] // If it is found this time, update it based on what was not found last time. - fieldInfo.LastFuzzyKey.Store(paramKey) + cachedFieldInfo.LastFuzzyKey.Store(paramKey) } } if ok { - fieldValue = fieldInfo.GetFieldReflectValue(structValue) + fieldValue = cachedFieldInfo.GetFieldReflectValueFrom(structValue) if paramValue != nil { if err = bindVarToStructField( - fieldValue, paramValue, fieldInfo, paramKeyToAttrMap, + fieldValue, paramValue, cachedFieldInfo, paramKeyToAttrMap, ); err != nil { return err } // handle same field name in nested struct. - if len(fieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { if err = setOtherSameNameField( - fieldInfo, paramValue, structValue, paramKeyToAttrMap, + cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap, ); err != nil { return err } } } - usedParamsKeyOrTagNameMap[fieldInfo.FieldName()] = struct{}{} + usedParamsKeyOrTagNameMap[cachedFieldInfo.FieldName()] = struct{}{} break } } @@ -338,16 +342,16 @@ func bindStructWithLoopFieldInfos( cachedStructInfo *structcache.CachedStructInfo, ) (err error) { var ( - fieldInfo *structcache.CachedFieldInfo - fuzzLastKey string - fieldValue reflect.Value - paramKey string - paramValue any - matched bool - ok bool + cachedFieldInfo *structcache.CachedFieldInfo + fuzzLastKey string + fieldValue reflect.Value + paramKey string + paramValue any + matched bool + ok bool ) - for _, fieldInfo = range cachedStructInfo.FieldConvertInfos { - for _, fieldTag := range fieldInfo.PriorityTagAndFieldName { + for _, cachedFieldInfo = range cachedStructInfo.FieldConvertInfos { + for _, fieldTag := range cachedFieldInfo.PriorityTagAndFieldName { if paramValue, ok = paramsMap[fieldTag]; !ok { continue } @@ -355,16 +359,16 @@ func bindStructWithLoopFieldInfos( matched = true break } - fieldValue = fieldInfo.GetFieldReflectValue(structValue) + fieldValue = cachedFieldInfo.GetFieldReflectValueFrom(structValue) if err = bindVarToStructField( - fieldValue, paramValue, fieldInfo, paramKeyToAttrMap, + fieldValue, paramValue, cachedFieldInfo, paramKeyToAttrMap, ); err != nil { return err } // handle same field name in nested struct. - if len(fieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { if err = setOtherSameNameField( - fieldInfo, paramValue, structValue, paramKeyToAttrMap, + cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap, ); err != nil { return err } @@ -378,26 +382,26 @@ func bindStructWithLoopFieldInfos( continue } - fuzzLastKey = fieldInfo.LastFuzzyKey.Load().(string) + fuzzLastKey = cachedFieldInfo.LastFuzzyKey.Load().(string) if paramValue, ok = paramsMap[fuzzLastKey]; !ok { paramKey, paramValue = fuzzyMatchingFieldName( - fieldInfo.RemoveSymbolsFieldName, paramsMap, usedParamsKeyOrTagNameMap, + cachedFieldInfo.RemoveSymbolsFieldName, paramsMap, usedParamsKeyOrTagNameMap, ) ok = paramKey != "" - fieldInfo.LastFuzzyKey.Store(paramKey) + cachedFieldInfo.LastFuzzyKey.Store(paramKey) } if ok { - fieldValue = fieldInfo.GetFieldReflectValue(structValue) + fieldValue = cachedFieldInfo.GetFieldReflectValueFrom(structValue) if paramValue != nil { if err = bindVarToStructField( - fieldValue, paramValue, fieldInfo, paramKeyToAttrMap, + fieldValue, paramValue, cachedFieldInfo, paramKeyToAttrMap, ); err != nil { return err } // handle same field name in nested struct. - if len(fieldInfo.OtherSameNameFieldIndex) > 0 { + if len(cachedFieldInfo.OtherSameNameFieldIndex) > 0 { if err = setOtherSameNameField( - fieldInfo, paramValue, structValue, paramKeyToAttrMap, + cachedFieldInfo, paramValue, structValue, paramKeyToAttrMap, ); err != nil { return err } @@ -432,7 +436,7 @@ func fuzzyMatchingFieldName( func bindVarToStructField( fieldValue reflect.Value, srcValue interface{}, - fieldInfo *structcache.CachedFieldInfo, + cachedFieldInfo *structcache.CachedFieldInfo, paramKeyToAttrMap map[string]string, ) (err error) { if !fieldValue.IsValid() { @@ -445,7 +449,7 @@ func bindVarToStructField( defer func() { if exception := recover(); exception != nil { if err = bindVarToReflectValue(fieldValue, srcValue, paramKeyToAttrMap); err != nil { - err = gerror.Wrapf(err, `error binding srcValue to attribute "%s"`, fieldInfo.FieldName()) + err = gerror.Wrapf(err, `error binding srcValue to attribute "%s"`, cachedFieldInfo.FieldName()) } } }() @@ -460,7 +464,7 @@ func bindVarToStructField( customConverterInput reflect.Value ok bool ) - if fieldInfo.IsCustomConvert { + if cachedFieldInfo.IsCustomConvert { if customConverterInput, ok = srcValue.(reflect.Value); !ok { customConverterInput = reflect.ValueOf(srcValue) } @@ -468,19 +472,19 @@ func bindVarToStructField( return } } - if fieldInfo.IsCommonInterface { + if cachedFieldInfo.IsCommonInterface { if ok, err = bindVarToReflectValueWithInterfaceCheck(fieldValue, srcValue); ok || err != nil { return } } // Common types use fast assignment logic - if fieldInfo.ConvertFunc != nil { - fieldInfo.ConvertFunc(srcValue, fieldValue) + if cachedFieldInfo.ConvertFunc != nil { + cachedFieldInfo.ConvertFunc(srcValue, fieldValue) return nil } doConvertWithReflectValueSet(fieldValue, doConvertInput{ FromValue: srcValue, - ToTypeName: fieldInfo.StructField.Type.String(), + ToTypeName: cachedFieldInfo.StructField.Type.String(), ReferValue: fieldValue, }) return nil diff --git a/util/gconv/internal/structcache/structcache_cached.go b/util/gconv/internal/structcache/structcache_cached.go index 110c58e888e..576dfeed02e 100644 --- a/util/gconv/internal/structcache/structcache_cached.go +++ b/util/gconv/internal/structcache/structcache_cached.go @@ -53,9 +53,12 @@ func GetCachedStructInfo( // check if it has been cached. structInfo, ok := getCachedConvertStructInfo(structType) if ok { + // directly returns the cached struct info if already exists. return structInfo } + // else create one. + // it parses and generates a cache info for given struct type. structInfo = &CachedStructInfo{ tagOrFiledNameToFieldInfoMap: make(map[string]*CachedFieldInfo), @@ -115,6 +118,7 @@ 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 diff --git a/util/gconv/internal/structcache/structcache_cached_field_info.go b/util/gconv/internal/structcache/structcache_cached_field_info.go index 96bece627ea..98432e64b4b 100644 --- a/util/gconv/internal/structcache/structcache_cached_field_info.go +++ b/util/gconv/internal/structcache/structcache_cached_field_info.go @@ -96,20 +96,26 @@ func (cfi *CachedFieldInfo) FieldName() string { return cfi.PriorityTagAndFieldName[len(cfi.PriorityTagAndFieldName)-1] } -// GetFieldReflectValue retrieves and returns the reflect.Value of given struct value, +// GetFieldReflectValueFrom retrieves and returns the `reflect.Value` of given struct field, // which is used for directly value assignment. -func (cfi *CachedFieldInfo) GetFieldReflectValue(structValue reflect.Value) reflect.Value { +// +// Note that, the input parameter `structValue` might be initialized internally. +func (cfi *CachedFieldInfo) GetFieldReflectValueFrom(structValue reflect.Value) reflect.Value { if len(cfi.FieldIndexes) == 1 { + // no nested struct. return structValue.Field(cfi.FieldIndexes[0]) } return cfi.fieldReflectValue(structValue, cfi.FieldIndexes) } -// GetOtherFieldReflectValue retrieves and returns the reflect.Value of given struct value with nested index +// GetOtherFieldReflectValueFrom retrieves and returns the `reflect.Value` of given struct field with nested index // by `fieldLevel`, which is used for directly value assignment. -func (cfi *CachedFieldInfo) GetOtherFieldReflectValue(structValue reflect.Value, fieldLevel int) reflect.Value { +// +// 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] if len(fieldIndex) == 1 { + // no nested struct. return structValue.Field(fieldIndex[0]) } return cfi.fieldReflectValue(structValue, fieldIndex) @@ -118,9 +124,11 @@ func (cfi *CachedFieldInfo) GetOtherFieldReflectValue(structValue reflect.Value, func (cfi *CachedFieldInfo) fieldReflectValue(v reflect.Value, fieldIndexes []int) reflect.Value { for i, x := range fieldIndexes { if i > 0 { + // it means nested struct. switch v.Kind() { case reflect.Pointer: if v.IsNil() { + // Initialization. v.Set(reflect.New(v.Type().Elem())) } v = v.Elem() @@ -133,6 +141,7 @@ func (cfi *CachedFieldInfo) fieldReflectValue(v reflect.Value, fieldIndexes []in // maybe *struct or other types v = v.Elem() } + default: } } v = v.Field(x)