AlexStocks commented on code in PR #3687: URL: https://github.com/apache/dubbo-go/pull/3687#discussion_r3986408319
########## common/extension/config_loader.go: ########## @@ -0,0 +1,411 @@ +/* + * 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 extension + +import ( + "errors" + "fmt" + "reflect" + "sort" + "strings" +) + +import ( + "github.com/mitchellh/mapstructure" +) + +// Initialize creates and initializes the extensions active for one lifecycle +// scope. rawConfigs is the map below dubbo.extensions, while options contains +// typed options declared by the corresponding entry point. +// +// Each active extension receives a fresh Config. Its configuration precedence +// is defaults from Config.New, selected YAML, typed options, and finally +// Config.Init. Filter names are validated before they are returned, so callers +// can merge them into their filter configuration atomically after all +// extensions have initialized successfully. +func Initialize(rawConfigs map[string]any, options []Option, scope Scope) ([]string, error) { + if !scope.valid() { + return nil, fmt.Errorf("extension: invalid scope %d", scope) + } + + registered := configs.Snapshot() + optionsByPrefix, activePrefixes, err := groupOptionsByPrefix(options, registered) + if err != nil { + return nil, err + } + rawByPrefix, err := collectRawConfigs(rawConfigs, scope, registered, activePrefixes) + if err != nil { + return nil, err + } + + return initializeConfigs(registered, rawByPrefix, optionsByPrefix, activePrefixes, scope) +} + +func groupOptionsByPrefix(options []Option, registered map[string]Config) (map[string][]Option, map[string]struct{}, error) { + optionsByPrefix := make(map[string][]Option) + activePrefixes := make(map[string]struct{}) + + for index, option := range options { + prefix, err := validateOptionPrefix(option, index, registered) + if err != nil { + return nil, nil, err + } + optionsByPrefix[prefix] = append(optionsByPrefix[prefix], option) + activePrefixes[prefix] = struct{}{} + } + + return optionsByPrefix, activePrefixes, nil +} + +func validateOptionPrefix(option Option, index int, registered map[string]Config) (string, error) { + if optionIsNil(option) { + return "", fmt.Errorf("extension: option %d is nil", index) + } + rawPrefix := option.Prefix() + prefix := strings.TrimSpace(rawPrefix) + if prefix == "" { + return "", fmt.Errorf("extension: option %d has an empty prefix", index) + } + if prefix != rawPrefix { + return "", fmt.Errorf("extension: option %d prefix %q must not contain surrounding whitespace", index, rawPrefix) + } + if _, ok := registered[prefix]; !ok { + return "", fmt.Errorf("extension %q: config is not registered", prefix) + } + return prefix, nil +} + +func collectRawConfigs(rawConfigs map[string]any, scope Scope, registered map[string]Config, activePrefixes map[string]struct{}) (map[string]map[string]any, error) { + rawByPrefix := make(map[string]map[string]any) + for prefix, value := range rawConfigs { + selected, active, err := selectRawConfig(value, scope) + if err != nil { + return nil, fmt.Errorf("extension %q: invalid YAML config: %w", prefix, err) + } + if !active { + continue + } + if _, ok := registered[prefix]; !ok { + return nil, fmt.Errorf("extension %q: config is not registered", prefix) + } + rawByPrefix[prefix] = selected + activePrefixes[prefix] = struct{}{} + } + return rawByPrefix, nil +} + +func initializeConfigs(registered map[string]Config, rawByPrefix map[string]map[string]any, optionsByPrefix map[string][]Option, activePrefixes map[string]struct{}, scope Scope) ([]string, error) { + prefixes := make([]string, 0, len(activePrefixes)) + for prefix := range activePrefixes { + prefixes = append(prefixes, prefix) + } + sort.Strings(prefixes) + + filterNames := make([]string, 0) + seenFilters := make(map[string]struct{}) + prepared := make([]preparedConfig, 0, len(prefixes)) + for _, prefix := range prefixes { + config, err := prepareConfig(registered[prefix], rawByPrefix[prefix], optionsByPrefix[prefix], prefix, scope, seenFilters) + if err != nil { + return nil, err + } + prepared = append(prepared, config) + filterNames = append(filterNames, config.filterNames...) + } + + initialized := make([]preparedConfig, 0, len(prepared)) + for _, config := range prepared { + initialized = append(initialized, config) + if err := config.config.Init(scope); err != nil { + initErr := fmt.Errorf("extension %q: initialize scope %d: %w", config.prefix, scope, err) + if rollbackErr := rollbackConfigs(initialized, scope); rollbackErr != nil { + return nil, errors.Join(initErr, rollbackErr) + } + return nil, initErr + } + } + + return filterNames, nil +} + +func rollbackConfigs(configs []preparedConfig, scope Scope) error { + var rollbackErr error + for index := len(configs) - 1; index >= 0; index-- { + config := configs[index] + rollbacker, ok := config.config.(Rollbacker) + if !ok { + continue + } + if err := rollbacker.Rollback(scope); err != nil { + rollbackErr = errors.Join(rollbackErr, + fmt.Errorf("extension %q: rollback scope %d: %w", config.prefix, scope, err)) + } + } + return rollbackErr +} + +type preparedConfig struct { + config Config + prefix string + filterNames []string +} + +func prepareConfig(prototype Config, raw map[string]any, options []Option, prefix string, scope Scope, seenFilters map[string]struct{}) (preparedConfig, error) { + config := prototype.New() + if err := validateNewConfig(config, prefix); err != nil { + return preparedConfig{}, err + } + + if err := decodeExtensionConfig(raw, config, prefix); err != nil { + return preparedConfig{}, err + } + if err := applyOptions(config, options, prefix); err != nil { + return preparedConfig{}, err + } + filterNames, err := collectFilterNames(config, prefix, scope, seenFilters) + if err != nil { + return preparedConfig{}, err + } + return preparedConfig{config: config, prefix: prefix, filterNames: filterNames}, nil +} + +func validateNewConfig(config Config, prefix string) error { + if configIsNil(config) { + return fmt.Errorf("extension %q: new config returned nil", prefix) + } + if configPrefix := config.Prefix(); configPrefix != prefix { + return fmt.Errorf("extension %q: new config returned prefix %q", prefix, configPrefix) + } + return nil +} + +func decodeExtensionConfig(raw map[string]any, config Config, prefix string) error { + if raw == nil { + return nil + } + if err := decodeConfig(raw, config); err != nil { + return fmt.Errorf("extension %q: decode YAML config: %w", prefix, err) + } + return nil +} + +func applyOptions(config Config, options []Option, prefix string) error { + for index, option := range options { + if err := option.Apply(config); err != nil { + return fmt.Errorf("extension %q: apply option %d: %w", prefix, index, err) + } + } + return nil +} + +func collectFilterNames(config Config, prefix string, scope Scope, seenFilters map[string]struct{}) ([]string, error) { + if scope == InstanceScope { + return nil, nil + } + + filterNames := make([]string, 0) + for index, name := range config.FilterNames(scope) { + name = strings.TrimSpace(name) + if name == "" { + return nil, fmt.Errorf("extension %q: filter name %d is empty", prefix, index) + } + if !HasFilter(name) { + return nil, fmt.Errorf("extension %q: filter %q is not registered", prefix, name) + } + if _, duplicate := seenFilters[name]; duplicate { + continue + } + seenFilters[name] = struct{}{} + filterNames = append(filterNames, name) + } + return filterNames, nil +} + +// MergeFilterNames appends extension filters to an existing filter list while +// preserving declaration order and honoring an explicit -name suppression. Review Comment: `-name` 抑制语义在 provider 侧只实现了「命中」分支，未命中的记号会被保留并进入导出链；provider 路径不会剥掉它，与 consumer 侧语义不一致。 实测（`0d935f6d` 源码 + 独立探针）： 1. `MergeFilterNames("-absent,a", []string{"contrib"})` 返回 `-absent,a,contrib`，未命中的 `-absent` 被原样保留。`common/extension/config_loader_test.go:307` 的 `assert.Equal(t, "-default,a,extension", MergeFilterNames("-default,a", []string{"extension"}))` 将这一行为固化为期望值。 2. provider 侧 `server/action.go:376` 把结果直接写入 URL，由 `protocolwrapper.BuildInvokerChain` 消费。该函数 `flt, _ := extension.GetFilter(strings.TrimSpace(filterName))` 忽略 `ok`，未注册名字得到 `nil`，构造出 `FilterInvoker{filter: nil}`，`Invoke` 调用 `fi.filter.Invoke` 时 panic。实测：`GetFilter("-probe-head3-absent") -> ok=false`；`BuildInvokerChain(...) -> *FilterInvoker=true, filter==nil: true`；`Invoke panicked: runtime error: invalid memory address or nil pointer dereference`。 3. 端到端：`server.WithServerFilter("-probe-head3-absent")` 配合一个活跃扩展，导出 `service.filter = "-probe-head3-absent,server-entry-filter"`。 4. 命中分支的导出结果也值得确认：`server.WithServerFilter("-server-entry-filter")`（该名字由扩展贡献）导出 `service.filter = ""`，即整条默认链消失，而不是「默认链减去该 filter」。 5. consumer 侧无此问题：`client/action.go:444` 经 `commonCfg.MergeValue`，其内部 `removeMinus`（`common/config/utils.go:93`）会剥掉 `-name`。`removeMinus` 在仓内仅由 `MergeValue` 调用，provider 路径不经过它。 建议：合并阶段丢弃未命中 additions 的 `-name` 记号（保留记号只对能理解它的 consumer 路径有意义），或在 provider 侧复用 `removeMinus`。 ########## common/extension/config.go: ########## @@ -17,14 +17,118 @@ package extension -var ( - configs = NewRegistry[Config]("config") +import ( + "fmt" + "reflect" + "strings" +) + +// Scope identifies the lifecycle in which an extension is initialized. +// Each initialization receives exactly one concrete scope. +type Scope uint8 + +const ( + // InstanceScope is the lifecycle of a dubbo.Instance. + InstanceScope Scope = iota + 1 + // ClientScope is the lifecycle of a client/consumer. + ClientScope + // ServerScope is the lifecycle of a server/provider. + ServerScope ) +func (s Scope) valid() bool { + return s == InstanceScope || s == ClientScope || s == ServerScope +} + +// Config is the configuration and initialization contract implemented by an +// external extension. A registered Config is an immutable prototype; New must +// return an independent configuration initialized with the extension defaults. type Config interface { Prefix() string + New() Config + // Init initializes the extension for one concrete lifecycle scope. If it + // returns an error after acquiring resources, the implementation must leave + // those resources released; Initialize invokes Rollbacker when available + // for cleanup shared with later extension failures. + Init(scope Scope) error + // FilterNames contributes filters for client and server lifecycles. It is + // not called for InstanceScope. + FilterNames(scope Scope) []string +} + +// Rollbacker is an optional failure cleanup contract for an extension whose +// Init has started. Initialize calls Rollback in reverse order when a later +// extension fails during the same initialization attempt. Implementations +// should make Rollback idempotent and release resources acquired by Init. +type Rollbacker interface { + Rollback(scope Scope) error +} + +// Option applies typed configuration to one extension. The core groups +// options by Prefix and applies them in declaration order. +type Option interface { + Prefix() string + Apply(config Config) error +} + +var ( + configs = NewRegistry[Config]("config") +) + +// RegisterConfig registers an immutable Config prototype. Duplicate prefixes Review Comment: 此处移除了已发布的导出 API `extension.SetConfig`，并同时给 `extension.Config` 接口新增 `New` / `Init` / `FilterNames` 三个方法，属于扩展 SPI 的破坏性变更。 证据：`SetConfig` 存在于已发布 tag `v3.3.0`、`v3.3.1`、`v3.3.2` 的 `common/extension/config.go`。仓内已无调用点（`go build ./...` 通过），但通过 `extension.SetConfig` 注册 `Config` 的外部扩展将编译失败，且需补齐三个新方法。 `CHANGELOG.md`、`README.md`、`README_CN.md`、`doc/` 中检索 `SetConfig|RegisterConfig|MustRegisterConfig|extension.Config` 无命中，即该变更未在文档中记录。 建议在 CHANGELOG 或迁移说明中标注 breaking 与迁移路径（`SetConfig(c)` → `MustRegisterConfig(c)`，并实现 `New/Init/FilterNames`），或保留 `SetConfig` 作为 deprecated 包装。 ########## common/extension/config_loader.go: ########## @@ -0,0 +1,411 @@ +/* + * 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 extension + +import ( + "errors" + "fmt" + "reflect" + "sort" + "strings" +) + +import ( + "github.com/mitchellh/mapstructure" +) + +// Initialize creates and initializes the extensions active for one lifecycle +// scope. rawConfigs is the map below dubbo.extensions, while options contains +// typed options declared by the corresponding entry point. +// +// Each active extension receives a fresh Config. Its configuration precedence +// is defaults from Config.New, selected YAML, typed options, and finally +// Config.Init. Filter names are validated before they are returned, so callers +// can merge them into their filter configuration atomically after all +// extensions have initialized successfully. +func Initialize(rawConfigs map[string]any, options []Option, scope Scope) ([]string, error) { + if !scope.valid() { + return nil, fmt.Errorf("extension: invalid scope %d", scope) + } + + registered := configs.Snapshot() + optionsByPrefix, activePrefixes, err := groupOptionsByPrefix(options, registered) + if err != nil { + return nil, err + } + rawByPrefix, err := collectRawConfigs(rawConfigs, scope, registered, activePrefixes) + if err != nil { + return nil, err + } + + return initializeConfigs(registered, rawByPrefix, optionsByPrefix, activePrefixes, scope) +} + +func groupOptionsByPrefix(options []Option, registered map[string]Config) (map[string][]Option, map[string]struct{}, error) { + optionsByPrefix := make(map[string][]Option) + activePrefixes := make(map[string]struct{}) + + for index, option := range options { + prefix, err := validateOptionPrefix(option, index, registered) + if err != nil { + return nil, nil, err + } + optionsByPrefix[prefix] = append(optionsByPrefix[prefix], option) + activePrefixes[prefix] = struct{}{} + } + + return optionsByPrefix, activePrefixes, nil +} + +func validateOptionPrefix(option Option, index int, registered map[string]Config) (string, error) { + if optionIsNil(option) { + return "", fmt.Errorf("extension: option %d is nil", index) + } + rawPrefix := option.Prefix() + prefix := strings.TrimSpace(rawPrefix) + if prefix == "" { + return "", fmt.Errorf("extension: option %d has an empty prefix", index) + } + if prefix != rawPrefix { + return "", fmt.Errorf("extension: option %d prefix %q must not contain surrounding whitespace", index, rawPrefix) + } + if _, ok := registered[prefix]; !ok { + return "", fmt.Errorf("extension %q: config is not registered", prefix) + } + return prefix, nil +} + +func collectRawConfigs(rawConfigs map[string]any, scope Scope, registered map[string]Config, activePrefixes map[string]struct{}) (map[string]map[string]any, error) { + rawByPrefix := make(map[string]map[string]any) + for prefix, value := range rawConfigs { + selected, active, err := selectRawConfig(value, scope) + if err != nil { + return nil, fmt.Errorf("extension %q: invalid YAML config: %w", prefix, err) + } + if !active { + continue + } + if _, ok := registered[prefix]; !ok { + return nil, fmt.Errorf("extension %q: config is not registered", prefix) Review Comment: 未注册的扩展前缀会直接使启动失败（fail-fast）。当多服务共用同一份 YAML，或扩展按部署逐步启用时，配置中残留的、本服务未编译进的其他扩展块会导致本服务无法启动。若为有意设计，建议在文档中明确该契约；若希望容忍，建议提供忽略未注册前缀的开关。 ########## server/options_test.go: ########## @@ -29,11 +30,178 @@ import ( import ( "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/common/extension" + "dubbo.apache.org/dubbo-go/v3/filter" "dubbo.apache.org/dubbo-go/v3/global" "dubbo.apache.org/dubbo-go/v3/protocol" "dubbo.apache.org/dubbo-go/v3/registry" ) +type serverEntryConfig struct { + prefix string + Value int `yaml:"value"` + requiredScope extension.Scope + initialized extension.Scope + onInit func(*serverEntryConfig) +} + +func (c *serverEntryConfig) Prefix() string { + return c.prefix +} + +func (c *serverEntryConfig) New() extension.Config { + return &serverEntryConfig{ + prefix: c.prefix, + Value: 1, + requiredScope: c.requiredScope, + onInit: c.onInit, + } +} + +func (c *serverEntryConfig) Init(scope extension.Scope) error { + if c.requiredScope != 0 && scope != c.requiredScope { + return errors.New("server scope is required") + } + c.initialized = scope + if c.onInit != nil { + c.onInit(c) + } + return nil +} + +func (c *serverEntryConfig) FilterNames(extension.Scope) []string { + return []string{"server-entry-filter"} +} + +type serverEntryOption struct { + prefix string + value int +} + +func (o serverEntryOption) Prefix() string { + return o.prefix +} + +func (o serverEntryOption) Apply(config extension.Config) error { + config.(*serverEntryConfig).Value = o.value + return nil +} + +func TestWithExtensionBuildsServerConfigAndMergesFilter(t *testing.T) { + const prefix = "server-entry" + const filterName = "server-entry-filter" + + extension.UnregisterConfig(prefix) + extension.UnregisterFilter(filterName) + t.Cleanup(func() { + extension.UnregisterConfig(prefix) + extension.UnregisterFilter(filterName) + }) + + var initialized *serverEntryConfig + require.NoError(t, extension.RegisterConfig(&serverEntryConfig{ + prefix: prefix, + requiredScope: extension.ServerScope, + onInit: func(config *serverEntryConfig) { + initialized = config + }, + })) + extension.SetFilter(filterName, func() filter.Filter { return nil }) + + srv, err := NewServer( + SetServerExtensionConfigs(map[string]any{ + prefix: map[string]any{ + "provider": map[string]any{"value": 7}, + }, + }), + WithServerFilter("explicit"), + WithExtension(serverEntryOption{prefix: prefix, value: 9}), + ) + require.NoError(t, err) + require.NotNil(t, srv) + require.NotNil(t, initialized) + assert.Equal(t, 9, initialized.Value) + assert.Equal(t, extension.ServerScope, initialized.initialized) + assert.Equal(t, "explicit", srv.cfg.Provider.Filter) + + svcOpts := defaultServiceOptions() + svcOpts.Provider = srv.cfg.Provider + svcOpts.Application = srv.cfg.Application + svcOpts.Registries = srv.cfg.Registries + svcOpts.Protocols = srv.cfg.Protocols + require.NoError(t, svcOpts.init(srv, WithInterface("com.example.ServerEntry"))) + assert.Equal(t, "explicit,"+filterName, svcOpts.getUrlMap().Get(constant.ServiceFilterKey)) +} + +func TestWithExtensionPreservesDefaultServerFilters(t *testing.T) { + const prefix = "server-default-filter" + const filterName = "server-entry-filter" + extension.UnregisterConfig(prefix) + extension.UnregisterFilter(filterName) + t.Cleanup(func() { + extension.UnregisterConfig(prefix) + extension.UnregisterFilter(filterName) + }) + + require.NoError(t, extension.RegisterConfig(&serverEntryConfig{ + prefix: prefix, + requiredScope: extension.ServerScope, + })) + extension.SetFilter(filterName, func() filter.Filter { return nil }) + + srv, err := NewServer(WithExtension(serverEntryOption{prefix: prefix, value: 1})) + require.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Provider = srv.cfg.Provider + svcOpts.Application = srv.cfg.Application + svcOpts.Registries = srv.cfg.Registries + svcOpts.Protocols = srv.cfg.Protocols + require.NoError(t, svcOpts.init(srv, WithInterface("com.example.ServerDefaultFilter"))) + assert.Equal(t, constant.DefaultServiceFilters+","+filterName, + svcOpts.getUrlMap().Get(constant.ServiceFilterKey)) +} + +func TestWithExtensionRejectsUnsupportedServerScope(t *testing.T) { + const prefix = "server-entry-unsupported" + extension.UnregisterConfig(prefix) + t.Cleanup(func() { extension.UnregisterConfig(prefix) }) + + require.NoError(t, extension.RegisterConfig(&serverEntryConfig{ + prefix: prefix, + requiredScope: extension.InstanceScope, + })) + extension.SetFilter("server-entry-filter", func() filter.Filter { return nil }) + t.Cleanup(func() { extension.UnregisterFilter("server-entry-filter") }) + _, err := NewServer(WithExtension(serverEntryOption{prefix: prefix, value: 1})) + require.Error(t, err) + assert.Contains(t, err.Error(), "server scope is required") +} + +func TestWithExtensionHonorsExplicitFilterSuppression(t *testing.T) { + const prefix = "server-entry-suppressed" + const filterName = "server-entry-filter" + extension.UnregisterConfig(prefix) + extension.UnregisterFilter(filterName) + t.Cleanup(func() { + extension.UnregisterConfig(prefix) + extension.UnregisterFilter(filterName) + }) + + require.NoError(t, extension.RegisterConfig(&serverEntryConfig{ + prefix: prefix, + requiredScope: extension.ServerScope, + })) + extension.SetFilter(filterName, func() filter.Filter { return nil }) + + srv, err := NewServer( + WithServerFilter("-"+filterName), + WithExtension(serverEntryOption{prefix: prefix, value: 1}), + ) + require.NoError(t, err) + assert.Equal(t, "-"+filterName, srv.cfg.Provider.Filter) Review Comment: 该用例断言的是中间态 `Provider.Filter`，未断言生效的 `service.filter`，因此无法证明抑制确实生效，也覆盖不到该语义缺口。 实测（同一构造方式，`server/zz_probe_head3_test.go`）： - `WithServerFilter("-server-entry-filter")` + 扩展：`Provider.Filter = "-server-entry-filter"`，而 `svcOpts.getUrlMap().Get(constant.ServiceFilterKey) = ""`。 - 对照 `client/options_test.go:152`，consumer 侧用例断言的是 `refOpts.getURLMap().Get(constant.ReferenceFilterKey)`，覆盖到了生效链。 建议改为断言 `getUrlMap().Get(constant.ServiceFilterKey)` 的取值。 -- 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]
