Copilot commented on code in PR #3207: URL: https://github.com/apache/dubbo-go/pull/3207#discussion_r2791266617
########## internal/config.go: ########## @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Package internal contains dubbo-go-internal code, to avoid polluting +// the top-level dubbo-go package. It must not import any dubbo-go symbols +// except internal symbols to avoid circular dependencies. Review Comment: The package comment says the internal package "must not import any dubbo-go symbols except internal symbols", but this file imports common/constant/global. Either relax/clarify this comment (e.g., "must not import server/config to avoid cycles") or move this helper into a package whose import rules match its dependencies to avoid misleading future contributors. ```suggestion // the top-level dubbo-go package. It should avoid importing packages // (such as server/config) that would introduce circular dependencies. ``` ########## server/options.go: ########## @@ -888,17 +865,6 @@ func WithRegistry(opts ...registry.Option) ServiceOption { } } Review Comment: WithMethod() was removed from the server package options, which is a breaking change for downstream users that configured per-method service settings via server.WithMethod(...). If the goal is only to remove the dependency on the config package, consider providing an equivalent server-level API (e.g., accepting *global.MethodConfig or a server-local MethodOption type) and documenting/migrating the old usage rather than removing the option outright. ```suggestion // MethodOption represents a configuration option for a single RPC method. // It allows callers to configure per-method settings without depending on the // legacy config package. type MethodOption func(*global.MethodConfig) // WithMethod configures per-method settings for the given method name. // It is intended as a replacement for the removed config-based WithMethod API // and applies the provided MethodOptions to a *global.MethodConfig stored in // the service configuration. func WithMethod(method string, methodOptions ...MethodOption) ServiceOption { return func(opts *ServiceOptions) { if opts.Service == nil { return } if opts.Service.Methods == nil { opts.Service.Methods = make(map[string]*global.MethodConfig) } mc, ok := opts.Service.Methods[method] if !ok || mc == nil { mc = &global.MethodConfig{} opts.Service.Methods[method] = mc } for _, mo := range methodOptions { if mo != nil { mo(mc) } } } } ``` ########## internal/config.go: ########## @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Package internal contains dubbo-go-internal code, to avoid polluting +// the top-level dubbo-go package. It must not import any dubbo-go symbols +// except internal symbols to avoid circular dependencies. +package internal + +import ( + "net/url" + "strconv" + "strings" +) + +import ( + "github.com/dubbogo/gost/log/logger" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/global" +) + +func LoadRegistries(registryIds []string, registries map[string]*global.RegistryConfig, roleType common.RoleType) []*common.URL { + var registryURLs []*common.URL + + for k, registryConf := range registries { + target := false + + if len(registryIds) == 0 || (len(registryIds) == 1 && registryIds[0] == "") { + target = true + } else { + for _, tr := range registryIds { + if tr == k { + target = true + break + } + } + } + + if target { + if urls, err := toURLs(registryConf, roleType); err != nil { + logger.Errorf("The registry id: %s url is invalid, error: %#v", k, err) + panic(err) + } else { Review Comment: internal.LoadRegistries/toURLs duplicates behavior from config.LoadRegistries/RegistryConfig.toURLs. Since this is new code that is easy to drift from the config implementation, it should have dedicated unit tests (including address forms with schemes, RegistryType=all/service/interface, empty/N/A address, and params propagation) to lock in behavior for server exports. ########## server/action.go: ########## @@ -141,7 +141,7 @@ func (svcOpts *ServiceOptions) Export() error { regUrls := make([]*common.URL, 0) if !svcConf.NotRegister { - regUrls = config.LoadRegistries(svcConf.RegistryIDs, svcOpts.registriesCompat, common.PROVIDER) + regUrls = internal.LoadRegistries(svcConf.RegistryIDs, svcOpts.Registries, common.PROVIDER) Review Comment: Service export now loads registry URLs from svcOpts.Registries, which can ignore service-level registry overrides stored on svcConf.RCRegistriesMap (populated during init and potentially coming from provider.services). This changes behavior compared to the previous implementation (which used svc.RCRegistriesMap) and can cause the wrong registries to be used. Consider calling LoadRegistries with svcConf.RCRegistriesMap (or otherwise the effective registry map) instead of svcOpts.Registries. ```suggestion regUrls = internal.LoadRegistries(svcConf.RegistryIDs, svcConf.RCRegistriesMap, common.PROVIDER) ``` ########## server/options.go: ########## @@ -532,20 +527,11 @@ func (svcOpts *ServiceOptions) init(srv *Server, opts ...ServiceOption) error { application := svcOpts.Application if application != nil { - svcOpts.applicationCompat = compatApplicationConfig(application) - if err := svcOpts.applicationCompat.Init(); err != nil { - return err - } - // todo(DMwangnima): make this clearer - // this statement is responsible for setting rootConfig.Application - // since many modules would retrieve this information directly. - config.GetRootConfig().Application = svcOpts.applicationCompat - svcOpts.metadataType = svcOpts.applicationCompat.MetadataType if svc.Group == "" { - svc.Group = svcOpts.applicationCompat.Group + svc.Group = application.Group } if svc.Version == "" { - svc.Version = svcOpts.applicationCompat.Version + svc.Version = application.Version } } Review Comment: svcOpts.metadataType is still used when building the exported URL (constant.MetadataTypeKey), but it is no longer initialized in ServiceOptions.init after removing the compat config path. This will leave the metadata type empty even when Application.MetadataType is set (or defaulted). Set svcOpts.metadataType from svcOpts.Application.MetadataType (after defaults.Set) or remove the separate field and use Application.MetadataType directly where needed. ########## internal/config.go: ########## @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Package internal contains dubbo-go-internal code, to avoid polluting +// the top-level dubbo-go package. It must not import any dubbo-go symbols +// except internal symbols to avoid circular dependencies. +package internal + +import ( + "net/url" + "strconv" + "strings" +) + +import ( + "github.com/dubbogo/gost/log/logger" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/global" +) + +func LoadRegistries(registryIds []string, registries map[string]*global.RegistryConfig, roleType common.RoleType) []*common.URL { + var registryURLs []*common.URL + + for k, registryConf := range registries { + target := false + + if len(registryIds) == 0 || (len(registryIds) == 1 && registryIds[0] == "") { + target = true + } else { + for _, tr := range registryIds { + if tr == k { + target = true + break + } + } + } + + if target { + if urls, err := toURLs(registryConf, roleType); err != nil { + logger.Errorf("The registry id: %s url is invalid, error: %#v", k, err) + panic(err) + } else { + for _, u := range urls { + u.AddParam(constant.RegistryIdKey, k) + } + registryURLs = append(registryURLs, urls...) + } + } + } + + return registryURLs +} + +func toURLs(registriesConfig *global.RegistryConfig, roleType common.RoleType) ([]*common.URL, error) { + address := translateRegistryAddress(registriesConfig) + var urls []*common.URL + var err error + var registryURL *common.URL + + if address == "" || address == constant.NotAvailable { + logger.Infof("Empty or N/A registry address found, the process will work with no registry enabled " + + "which means that the address of this instance will not be registered and not able to be found by other consumer instances.") + return urls, nil + } + switch registriesConfig.RegistryType { + case constant.RegistryTypeService: + // service discovery protocol + if registryURL, err = createNewURL(registriesConfig, constant.ServiceRegistryProtocol, address, roleType); err == nil { + urls = append(urls, registryURL) + } + case constant.RegistryTypeInterface: + if registryURL, err = createNewURL(registriesConfig, constant.RegistryProtocol, address, roleType); err == nil { + urls = append(urls, registryURL) + } + case constant.RegistryTypeAll: + if registryURL, err = createNewURL(registriesConfig, constant.ServiceRegistryProtocol, address, roleType); err == nil { + urls = append(urls, registryURL) + } + if registryURL, err = createNewURL(registriesConfig, constant.RegistryProtocol, address, roleType); err == nil { + urls = append(urls, registryURL) + } + default: + if registryURL, err = createNewURL(registriesConfig, constant.ServiceRegistryProtocol, address, roleType); err == nil { + urls = append(urls, registryURL) + } + } + return urls, err +} + +func createNewURL(registriesConfig *global.RegistryConfig, protocol string, address string, roleType common.RoleType) (*common.URL, error) { + return common.NewURL(protocol+"://"+address, + common.WithParams(getUrlMap(registriesConfig, roleType)), + common.WithParamsValue(constant.RegistrySimplifiedKey, strconv.FormatBool(registriesConfig.Simplified)), + common.WithParamsValue(constant.RegistryKey, registriesConfig.Protocol), + common.WithParamsValue(constant.RegistryNamespaceKey, registriesConfig.Namespace), + common.WithParamsValue(constant.RegistryTimeoutKey, registriesConfig.Timeout), + common.WithUsername(registriesConfig.Username), + common.WithPassword(registriesConfig.Password), + common.WithLocation(registriesConfig.Address), + ) +} + +func translateRegistryAddress(registriesConfig *global.RegistryConfig) string { + if strings.Contains(registriesConfig.Address, "://") { + u, err := url.Parse(registriesConfig.Address) + if err != nil { + logger.Errorf("The registry url is invalid, error: %#v", err) + panic(err) + } + registriesConfig.Protocol = u.Scheme + registriesConfig.Address = strings.Join([]string{u.Host, u.Path}, "") + } + return registriesConfig.Address Review Comment: translateRegistryAddress mutates the passed *global.RegistryConfig (it rewrites Protocol and Address when Address contains a scheme). Previously, server converted to a separate config.RegistryConfig before parsing, so the original global config wasn't modified. Consider avoiding mutation by working on a copy (e.g., clone the config or parse into locals) so callers don't observe surprising changes after LoadRegistries. ```suggestion addr := registriesConfig.Address if strings.Contains(addr, "://") { u, err := url.Parse(addr) if err != nil { logger.Errorf("The registry url is invalid, error: %#v", err) panic(err) } addr = strings.Join([]string{u.Host, u.Path}, "") } return addr ``` -- 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]
