Copilot commented on code in PR #3506:
URL: https://github.com/apache/dubbo-go/pull/3506#discussion_r3637568308
##########
protocol/triple/triple.go:
##########
@@ -231,15 +231,20 @@ func (tp *TripleProtocol) HostHTTPHandler(url
*common.URL, handler http.Handler)
return nil
}
-// isGenericCall checks if the generic parameter indicates a generic call
+// isGenericCall reports whether generic is a supported generic mode.
+//
+// Triple accepts true, gson, protobuf-json, and bean. It also accepts protobuf
+// as a legacy compatibility alias; callers should use protobuf-json for new
+// configurations.
func isGenericCall(generic string) bool {
if generic == "" {
return false
}
return strings.EqualFold(generic, constant.GenericSerializationDefault)
||
strings.EqualFold(generic, constant.GenericSerializationGson) ||
strings.EqualFold(generic,
constant.GenericSerializationProtobuf) ||
- strings.EqualFold(generic,
constant.GenericSerializationProtobufJson)
+ strings.EqualFold(generic,
constant.GenericSerializationProtobufJson) ||
+ strings.EqualFold(generic, constant.GenericSerializationBean)
Review Comment:
isGenericCall() now treats generic=protobuf as a supported (legacy) generic
mode, but filter/generic/isGeneric() does not (it only accepts
true/gson/protobuf-json/bean). This inconsistency can cause URLs configured
with generic=protobuf to select NewTripleInvoker while the generic filter won’t
treat the invocation/service as generic, leading to incorrect generic behavior
(e.g., wrong generalizer selection or missing generic handling). Please align
the accepted generic-mode set across Triple and filter/generic: either add
protobuf as a legacy alias in filter/generic (and any other validation points),
or drop protobuf support here if it’s intentionally Triple-only.
##########
common/url.go:
##########
@@ -1163,28 +1216,128 @@ func IsEquals(left *URL, right *URL, excludes
...string) bool {
return false
}
- leftMap := left.ToMap()
- rightMap := right.ToMap()
- for _, exclude := range excludes {
- delete(leftMap, exclude)
- delete(rightMap, exclude)
+ excluded := newKeySet(excludes)
+ return equalReservedKeys(left, right, excluded) &&
+ equalParams(left, right, excluded)
+}
Review Comment:
This change rewrites URL equality (IsEquals) and adds new helper logic
(reserved keys, parseLocation, streaming param comparison). That’s a
significant behavioral/performance change in a core type and appears unrelated
to the PR’s stated goal (Triple generic mode alignment). Consider splitting
these URL changes into a separate PR (or at least updating the PR description
to call out the URL equality semantics change explicitly) so it can be reviewed
and released independently.
--
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]
