Copilot commented on code in PR #3252:
URL: https://github.com/apache/dubbo-go/pull/3252#discussion_r2943071620
##########
cluster/router/tag/router.go:
##########
@@ -102,6 +102,26 @@ func (p *PriorityRouter) Notify(invokers []base.Invoker) {
p.Process(&config_center.ConfigChangeEvent{Key: key, Value: value,
ConfigType: remoting.EventTypeAdd})
}
+// SetStaticConfig applies a RouterConfig directly, bypassing YAML parsing.
+// This is the correct entry point for static (code-configured) rules;
+// Process is designed for dynamic config-center updates that arrive as YAML
text.
+func (p *PriorityRouter) SetStaticConfig(cfg *global.RouterConfig) {
+ if cfg == nil || len(cfg.Tags) == 0 {
+ return
+ }
+ cfgCopy := cfg.Clone()
+ cfgCopy.Valid = new(bool)
+ *cfgCopy.Valid = len(cfgCopy.Tags) > 0
+ if cfgCopy.Enabled == nil {
+ cfgCopy.Enabled = new(bool)
+ *cfgCopy.Enabled = true
+ }
+ // Derive storage key the same way Notify() does: application + suffix
+ key := strings.Join([]string{cfg.Key, constant.TagRouterRuleSuffix}, "")
+ p.routerConfigs.Store(key, *cfgCopy)
+ logger.Infof("[tag router] Applied static tag router config: key=%s",
key)
Review Comment:
In SetStaticConfig(), the storage key is derived from cfg.Key +
TagRouterRuleSuffix, but Route() looks up routerConfigs using
invokers[0].GetURL().GetParam(constant.Tagkey, "") (provider tag), while
Notify()/subscription uses constant.ApplicationKey (application name). As a
result, injected static tag configs will never be found during Route unless
provider URLs happen to have dubbo.tag == application name. Please align key
derivation/lookup across Route(), Notify(), and SetStaticConfig (likely use
ApplicationKey/application name consistently) so static injection is actually
applied.
##########
cluster/router/tag/router_test.go:
##########
@@ -399,3 +399,60 @@ tags:
assert.Nil(t, value)
})
}
+
+func TestSetStaticConfig(t *testing.T) {
+ t.Run("empty tag config is ignored", func(t *testing.T) {
+ p, err := NewTagPriorityRouter()
+ require.NoError(t, err)
+
+ p.SetStaticConfig(&global.RouterConfig{Key: "test-app"})
+
+ value, ok := p.routerConfigs.Load("test-app" +
constant.TagRouterRuleSuffix)
+ assert.False(t, ok)
+ assert.Nil(t, value)
+ })
+
+ t.Run("static tag config is cloned and stored with default enabled",
func(t *testing.T) { // NOSONAR
+ p, err := NewTagPriorityRouter()
+ require.NoError(t, err)
+
+ cfg := &global.RouterConfig{
+ Key: "test-app",
+ Tags: []global.Tag{{
+ Name: "gray",
+ Addresses: []string{"192.168.0.1:20000"}, //
NOSONAR
+ }},
+ }
+
+ p.SetStaticConfig(cfg)
+ cfg.Tags[0].Addresses[0] = "192.168.0.9:20000" // NOSONAR
+
+ value, ok := p.routerConfigs.Load("test-app" +
constant.TagRouterRuleSuffix)
+ require.True(t, ok)
+ routerCfg := value.(global.RouterConfig)
+ assert.True(t, *routerCfg.Enabled)
+ assert.True(t, *routerCfg.Valid)
+ assert.Equal(t, "192.168.0.1:20000",
routerCfg.Tags[0].Addresses[0]) // NOSONAR
+ })
Review Comment:
The new TestSetStaticConfig() only asserts that the config is cloned/stored,
but it doesn’t verify that a statically injected tag RouterConfig is actually
applied during Route() (i.e., dynamicTag path is taken and routing result
changes). Adding a Route-level test that sets static config, prepares invokers
with different tags/addresses, and asserts the filtered invokers would better
cover the new feature and would catch key/flag mismatches.
##########
cluster/router/tag/router.go:
##########
@@ -102,6 +102,26 @@ func (p *PriorityRouter) Notify(invokers []base.Invoker) {
p.Process(&config_center.ConfigChangeEvent{Key: key, Value: value,
ConfigType: remoting.EventTypeAdd})
}
+// SetStaticConfig applies a RouterConfig directly, bypassing YAML parsing.
+// This is the correct entry point for static (code-configured) rules;
+// Process is designed for dynamic config-center updates that arrive as YAML
text.
+func (p *PriorityRouter) SetStaticConfig(cfg *global.RouterConfig) {
+ if cfg == nil || len(cfg.Tags) == 0 {
+ return
+ }
+ cfgCopy := cfg.Clone()
+ cfgCopy.Valid = new(bool)
+ *cfgCopy.Valid = len(cfgCopy.Tags) > 0
+ if cfgCopy.Enabled == nil {
+ cfgCopy.Enabled = new(bool)
+ *cfgCopy.Enabled = true
+ }
Review Comment:
SetStaticConfig() doesn't ensure cfgCopy.Force is non-nil, but
dynamicTag()/requestTag() later dereference *cfg.Force. If a user provides a
static tag config without Force set (common case), this can panic at runtime.
Consider defaulting Force to false (and, if needed, also ensuring Enabled/Valid
are always non-nil) when storing the cloned config, or updating the tag routing
logic to treat nil pointers as defaults safely.
--
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]
