Copilot commented on code in PR #3200:
URL: https://github.com/apache/dubbo-go/pull/3200#discussion_r2776688580
##########
cluster/router/tag/match.go:
##########
@@ -34,6 +34,18 @@ import (
type predicate func(invoker base.Invoker, tag any) bool
+func getInstanceTag(invoker base.Invoker) string {
+ invUrl := invoker.GetURL()
+ if attr, ok := invUrl.GetAttribute(constant.ServiceInstanceKey); ok {
+ if instance, ok := attr.(interface{ GetMetadata()
map[string]string }); ok {
+ if tag, ok := instance.GetMetadata()[constant.Tagkey];
ok {
+ return tag
+ }
+ }
+ }
Review Comment:
`getInstanceTag` relies on a URL attribute (`constant.ServiceInstanceKey`)
to access the `ServiceInstance` metadata, but there is currently no code in the
repo that sets this attribute on provider URLs (no `SetAttribute/WithAttribute`
usage found for this key). As a result, this branch will never be taken and the
router will always fall back to `invUrl.GetParam(constant.Tagkey, "")`, so tags
that exist only in `ServiceInstance.Metadata` still won't be recognized.
Consider wiring the `ServiceInstance` into the invoker URL attributes when
synthesizing provider URLs (or alternatively, populate the URL param from
instance metadata/tag during URL creation).
```suggestion
```
##########
cluster/router/tag/match.go:
##########
@@ -34,6 +34,18 @@ import (
type predicate func(invoker base.Invoker, tag any) bool
+func getInstanceTag(invoker base.Invoker) string {
+ invUrl := invoker.GetURL()
+ if attr, ok := invUrl.GetAttribute(constant.ServiceInstanceKey); ok {
+ if instance, ok := attr.(interface{ GetMetadata()
map[string]string }); ok {
+ if tag, ok := instance.GetMetadata()[constant.Tagkey];
ok {
+ return tag
+ }
+ }
Review Comment:
The new behavior in `getInstanceTag` (reading tag from
`ServiceInstance.Metadata` via URL attribute) is not covered by unit tests. Add
a test case that sets
`invoker.GetURL().SetAttribute(constant.ServiceInstanceKey, <mock instance with
metadata containing constant.Tagkey>)` and verifies static tag routing
selects/excludes the invoker based on that metadata tag.
##########
client/client.go:
##########
@@ -79,6 +79,19 @@ func (conn *Connection) call(ctx context.Context, reqs
[]any, resp any, methodNa
if err != nil {
return nil, err
}
+
+ if attaRaw := ctx.Value(constant.AttachmentKey); attaRaw != nil {
+ if userAtta, ok := attaRaw.(map[string]string); ok {
+ for key, val := range userAtta {
+ inv.SetAttachment(key, val)
+ }
+ } else if userAtta, ok := attaRaw.(map[string]any); ok {
+ for key, val := range userAtta {
+ inv.SetAttachment(key, val)
+ }
+ }
+ }
Review Comment:
This change adds propagation of `constant.AttachmentKey` from
`context.Context` into the generated invocation, which is critical for routing
decisions (e.g., tag/force-tag). There is no unit test asserting that
`Connection.call` copies attachments (both `map[string]string` and
`map[string]any`) into `inv.Attachments()`. Add a test using the existing
`fakeInvoker` to verify the invocation received by the invoker contains the
expected attachments.
--
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]
