Copilot commented on code in PR #3171:
URL: https://github.com/apache/dubbo-go/pull/3171#discussion_r2703374282
##########
filter/generic/generalizer/map.go:
##########
@@ -54,10 +47,14 @@ type MapGeneralizer struct{}
func (g *MapGeneralizer) Generalize(obj any) (gobj any, err error) {
gobj = objToMap(obj)
+ if !getGenericIncludeClass() {
+ gobj = removeClass(gobj)
+ }
return
Review Comment:
The integration of the `generic.include.class` configuration lacks test
coverage. Tests should be added to verify that: 1) When the config is true
(default), the 'class' key is preserved during Generalize and properly
processed during Realize, 2) When the config is false, the 'class' key is
removed during Generalize and Realize operations.
##########
filter/generic/generalizer/map.go:
##########
@@ -84,6 +81,41 @@ func (g *MapGeneralizer) GetType(obj any) (typ string, err
error) {
return
}
+func getGenericIncludeClass() bool {
+ return true
+}
Review Comment:
The function `getGenericIncludeClass()` always returns `true` and does not
actually read from environment configuration as described in the PR
description. According to the PR description, this function should read the
environment config `generic.include.class` with a default value of `true`.
Consider implementing the actual configuration reading logic using the constant
`GenericIncludeClassKey` from `common/constant/key.go`.
##########
filter/generic/generalizer/map.go:
##########
@@ -84,6 +81,41 @@ func (g *MapGeneralizer) GetType(obj any) (typ string, err
error) {
return
}
+func getGenericIncludeClass() bool {
+ return true
+}
+
+func removeClass(obj any) any {
+ switch v := obj.(type) {
+ case map[string]any:
+ m := make(map[string]any, len(v))
+ for k, val := range v {
+ if k == "class" {
+ continue
+ }
+ m[k] = removeClass(val)
+ }
+ return m
+ case map[any]any:
+ m := make(map[any]any, len(v))
+ for k, val := range v {
+ if k == "class" {
+ continue
+ }
+ m[k] = removeClass(val)
+ }
+ return m
+ case []any:
+ s := make([]any, 0, len(v))
+ for _, val := range v {
+ s = append(s, removeClass(val))
+ }
+ return s
+ default:
+ return obj
+ }
+}
Review Comment:
The new functions `getGenericIncludeClass` and `removeClass` lack
documentation comments. Add documentation to explain their purpose, parameters,
return values, and any important behavior details.
##########
filter/generic/generalizer/map.go:
##########
@@ -84,6 +81,41 @@ func (g *MapGeneralizer) GetType(obj any) (typ string, err
error) {
return
}
+func getGenericIncludeClass() bool {
+ return true
+}
+
+func removeClass(obj any) any {
+ switch v := obj.(type) {
+ case map[string]any:
+ m := make(map[string]any, len(v))
+ for k, val := range v {
+ if k == "class" {
+ continue
+ }
+ m[k] = removeClass(val)
+ }
+ return m
+ case map[any]any:
+ m := make(map[any]any, len(v))
+ for k, val := range v {
+ if k == "class" {
+ continue
+ }
+ m[k] = removeClass(val)
+ }
+ return m
+ case []any:
+ s := make([]any, 0, len(v))
+ for _, val := range v {
+ s = append(s, removeClass(val))
+ }
+ return s
+ default:
+ return obj
+ }
+}
Review Comment:
The new `removeClass` function lacks test coverage. Since the repository has
comprehensive test coverage for the map generalizer (as seen in map_test.go),
tests should be added to verify the `removeClass` function correctly handles
all supported map and slice types, including nested structures.
##########
filter/generic/generalizer/map.go:
##########
@@ -54,10 +47,14 @@ type MapGeneralizer struct{}
func (g *MapGeneralizer) Generalize(obj any) (gobj any, err error) {
gobj = objToMap(obj)
+ if !getGenericIncludeClass() {
+ gobj = removeClass(gobj)
+ }
return
}
func (g *MapGeneralizer) Realize(obj any, typ reflect.Type) (any, error) {
+ obj = removeClass(obj)
Review Comment:
The `removeClass` function is called unconditionally in the `Realize`
method, which is inconsistent with the behavior in the `Generalize` method
where it's only called when `!getGenericIncludeClass()`. This creates an
asymmetry: when `generic.include.class` is `true`, the `Generalize` method will
keep the 'class' key, but the `Realize` method will remove it before
processing. Consider making this conditional by wrapping it in an `if
!getGenericIncludeClass()` check for consistency.
```suggestion
if !getGenericIncludeClass() {
obj = removeClass(obj)
}
```
##########
filter/generic/generalizer/map.go:
##########
@@ -84,6 +81,41 @@ func (g *MapGeneralizer) GetType(obj any) (typ string, err
error) {
return
}
+func getGenericIncludeClass() bool {
+ return true
+}
+
+func removeClass(obj any) any {
+ switch v := obj.(type) {
+ case map[string]any:
+ m := make(map[string]any, len(v))
+ for k, val := range v {
+ if k == "class" {
+ continue
+ }
+ m[k] = removeClass(val)
+ }
+ return m
+ case map[any]any:
+ m := make(map[any]any, len(v))
+ for k, val := range v {
+ if k == "class" {
Review Comment:
In the `map[any]any` case, the code checks if `k == "class"`, but `k` is of
type `any` and might not be a string. This comparison might not work as
expected when keys are not strings. Consider adding a type assertion to ensure
`k` is a string before comparing: `if str, ok := k.(string); ok && str ==
"class"`.
```suggestion
if key, ok := k.(string); ok && key == "class" {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
