Copilot commented on code in PR #3210:
URL: https://github.com/apache/dubbo-go/pull/3210#discussion_r2793457565
##########
filter/polaris/limit/limiter.go:
##########
@@ -78,27 +78,26 @@ func (pl *polarisTpsLimiter) buildQuotaRequest(url
*common.URL, invocation base.
ns := remotingpolaris.GetNamespace()
applicationMode := false
- // TODO: only for compatibility with old config, able to directly
remove after config is deleted
- for _, item := range config.GetRootConfig().Registries {
- if item.Protocol == constant.PolarisKey {
- applicationMode = item.RegistryType ==
constant.ServiceKey
+ registries, ok := url.GetAttribute(constant.RegistriesConfigKey)
+ if ok {
+ for _, item := range
registries.(map[string]*global.RegistryConfig) {
+ if item != nil && item.Protocol == constant.PolarisKey
&& item.RegistryType == constant.ServiceKey {
+ applicationMode = true
+ break
Review Comment:
`url.GetAttribute(constant.RegistriesConfigKey)` can legally return a `nil`
value with `ok==true` (e.g., if a caller sets the attribute to `nil`). The
direct type assertion `registries.(map[string]*global.RegistryConfig)` would
then panic. Please guard with a checked type assertion (and/or nil check)
before ranging over the map, and treat non-matching or nil values as “no
registries” to keep limiter behavior safe.
```suggestion
if registryMap, ok :=
registries.(map[string]*global.RegistryConfig); ok && registryMap != nil {
for _, item := range registryMap {
if item != nil && item.Protocol ==
constant.PolarisKey && item.RegistryType == constant.ServiceKey {
applicationMode = true
break
}
```
##########
config_center/apollo/impl_test.go:
##########
@@ -182,19 +187,16 @@ func initMockApollo(t *testing.T) *apolloConfiguration {
// Register the YAML format parser with concurrent safety.
extension.AddFormatParser(constant.YAML, &Parser{})
extension.AddFormatParser(constant.YML, &Parser{})
- c := &config.RootConfig{ConfigCenter: &config.CenterConfig{
- Protocol: "apollo",
- Address: "localhost:8080",
- AppID: "testApplication_yang",
- Cluster: "dev",
- Namespace: "mockDubbogo.yaml",
- Params: map[string]string{
- "config-center.isBackupConfig": "false",
- },
- }}
+
+ params := url.Values{}
+ params.Set("config-center.namespace", "mockDubbogo.yaml")
+ params.Set("config-center.appId", "testApplication_yang")
+ params.Set("config-center.cluster", "dev")
+ params.Set("config-center.isBackupConfig", "false")
Review Comment:
The config-center parameter keys are hard-coded as string literals here.
Using the existing constants (e.g., `constant.ConfigNamespaceKey`,
`constant.ConfigAppIDKey`, `constant.ConfigClusterKey`,
`constant.ConfigBackupConfigKey`) would prevent drift if the key names ever
change and keeps the test aligned with production parsing.
```suggestion
params.Set(constant.ConfigNamespaceKey, "mockDubbogo.yaml")
params.Set(constant.ConfigAppIDKey, "testApplication_yang")
params.Set(constant.ConfigClusterKey, "dev")
params.Set(constant.ConfigBackupConfigKey, "false")
```
##########
config_center/apollo/impl_test.go:
##########
@@ -182,19 +187,16 @@ func initMockApollo(t *testing.T) *apolloConfiguration {
// Register the YAML format parser with concurrent safety.
extension.AddFormatParser(constant.YAML, &Parser{})
extension.AddFormatParser(constant.YML, &Parser{})
- c := &config.RootConfig{ConfigCenter: &config.CenterConfig{
- Protocol: "apollo",
- Address: "localhost:8080",
- AppID: "testApplication_yang",
- Cluster: "dev",
- Namespace: "mockDubbogo.yaml",
- Params: map[string]string{
- "config-center.isBackupConfig": "false",
- },
- }}
+
+ params := url.Values{}
+ params.Set("config-center.namespace", "mockDubbogo.yaml")
+ params.Set("config-center.appId", "testApplication_yang")
+ params.Set("config-center.cluster", "dev")
+ params.Set("config-center.isBackupConfig", "false")
+
apollo := initApollo()
apolloUrl := strings.ReplaceAll(apollo.URL, "http", "apollo")
- url, err := common.NewURL(apolloUrl,
common.WithParams(c.ConfigCenter.GetUrlMap()))
+ url, err := common.NewURL(apolloUrl, common.WithParams(params))
require.NoError(t, err)
configuration, err := newApolloConfiguration(url)
Review Comment:
`url` is used both as the imported `net/url` package (for `url.Values{}`)
and then as a local variable name (`url, err := common.NewURL(...)`). Renaming
the local variable (e.g., `u`/`apolloURL`) would avoid name shadowing and make
the test easier to read/maintain.
```suggestion
apolloCfgURL, err := common.NewURL(apolloUrl, common.WithParams(params))
require.NoError(t, err)
configuration, err := newApolloConfiguration(apolloCfgURL)
```
--
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]
