Skip to content

feat(generic): validate generic mode and clarify config API (#3472) - #3500

Open
NeverENG wants to merge 1 commit into
apache:developfrom
NeverENG:feat/3472-generic-call-optimization
Open

feat(generic): validate generic mode and clarify config API (#3472)#3500
NeverENG wants to merge 1 commit into
apache:developfrom
NeverENG:feat/3472-generic-call-optimization

Conversation

@NeverENG

@NeverENG NeverENG commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Part of #3472 (泛化调用优化). This PR covers the client + config-validation slice: it makes an unknown generic mode fail fast at reference-creation time instead of silently falling back to the Map generalizer at runtime, and clarifies the generic-call configuration API. It also unifies the accepted generic-mode set between reference-creation validation and Triple's invoker routing so that bean is recognized consistently.

Changes

  • internal/config.go: add ValidateGenericType, accepting true / gson / protobuf-json / bean (case-insensitive; empty = non-generic). protobuf is currently kept as a legacy compatibility value. Unknown values return a clear error. Extract the accepted-mode decision into a shared IsGenericMode predicate so validation and protocol-level routing share one source of truth.
  • client/options.go: wire the validation into ReferenceOptions.init, so WithGenericType("bad-type") is rejected when the reference is created. Rewrite WithGenericType docs to distinguish generic mode (generalization format) from transport serialization (on-the-wire encoding).
  • client/client.go: document NewGenericService defaults (generic mode true/Map, transport serialization Hessian2), that the two settings are independent, and the scope of generic.include.class (Map-only, global config, no per-reference override).
  • protocol/triple/triple.go: route isGenericCall through internal.IsGenericMode so a Triple reference created with WithGenericType("bean") is recognized as generic and dispatched to the $invoke-capable NewTripleInvoker instead of NewDubbo3Invoker. The previously accepted modes (true/gson/protobuf/protobuf-json) are unchanged.
  • internal/config_test.go / protocol/triple/triple_test.go: table-driven tests covering all valid modes, case-insensitivity, empty, and unknown values, plus bean cases for isGenericCall.

Scope note (updated after review)

An earlier revision added WithGenericIncludeClass(bool). It has been removed: generic.include.class is only read by MapGeneralizer from the global config (filter/generic/generalizer/map.go:getGenericIncludeClass) and does not consult reference/URL params, so the option was a no-op. Per #3472 §4 ("如果本次不新增 API,则……仅补充注释和文档"), this PR keeps generic.include.class as global-config only and documents its scope, rather than shipping a non-functional per-reference API. A real per-reference override can be added later once the generalizer reads it.

Acceptance criteria (from #3472 §3/§4)

  • NewGenericService(..., WithGenericType("bad-type")) returns a clear error instead of falling back.
  • generic mode vs transport serialization documented and disambiguated.
  • generic.include.class scope documented (Map-only, global config).
  • Triple recognizes bean as a generic mode and routes it to the $invoke-capable NewTripleInvoker (shared with reference-creation validation via internal.IsGenericMode).
  • generic.include.class per-reference API — intentionally scoped out (see Scope note).

Follow-up: converge the last checker

The accepted-mode sets previously diverged across three checkers. This PR shares a single predicate (internal.IsGenericMode) between reference-creation validation and Triple routing, so both now agree and bean reaches the $invoke path:

checker true gson protobuf-json bean protobuf
ValidateGenericType (this PR, via IsGenericMode)
protocol/triple/triple.go:isGenericCall (this PR, via IsGenericMode)
filter/generic/util.go:isGeneric

The only remaining divergence is filter/generic/util.go:isGeneric, which omits the legacy protobuf. Routing it through the shared predicate would silently re-introduce protobuf into the filter path — a behavior change in a separate slice/owner of #3472 — so it is intentionally left out here and tracked as a follow-up together with the protobuf deprecation decision.

Verification

go build ./... and go test ./internal/ ./client/ ./protocol/triple/ pass locally.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens and clarifies the client-side generic-call configuration by validating generic mode early (at reference creation time) and improving API documentation to clearly separate generic mode (generalization format) from transport serialization (wire encoding).

Changes:

  • Added internal.ValidateGenericType + tests to define and validate the accepted generic modes (including legacy protobuf).
  • Wired generic-mode validation into ReferenceOptions.init to fail fast on unknown values.
  • Added documentation clarifying defaults and the separation between generic mode and transport serialization, and introduced WithGenericIncludeClass(bool).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
internal/config.go Adds ValidateGenericType for generic-mode validation and clearer failure behavior.
internal/config_test.go Adds table-driven tests covering valid/invalid generic modes and case-insensitivity.
client/options.go Validates generic mode during reference initialization; adds WithGenericIncludeClass option and improves docs.
client/client.go Documents NewGenericService defaults and clarifies generic mode vs transport serialization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/config.go Outdated
Comment thread internal/config.go Outdated
Comment thread client/options.go Outdated
@Alanxtl Alanxtl added ✏️ Feature 3.3.3 version 3.3.3 labels Jul 22, 2026
@Alanxtl Alanxtl linked an issue Jul 22, 2026 that may be closed by this pull request

@Alanxtl Alanxtl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread internal/config.go Outdated
@Snow-kal

Copy link
Copy Markdown
Contributor

lgtm

@codecov-commenter

codecov-commenter commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.44%. Comparing base (60d1c2a) to head (cacd17c).
⚠️ Report is 873 commits behind head on develop.

Files with missing lines Patch % Lines
client/options.go 0.00% 1 Missing and 1 partial ⚠️
internal/config.go 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3500      +/-   ##
===========================================
+ Coverage    46.76%   54.44%   +7.67%     
===========================================
  Files          295      462     +167     
  Lines        17172    35616   +18444     
===========================================
+ Hits          8031    19390   +11359     
- Misses        8287    14687    +6400     
- Partials       854     1539     +685     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@NeverENG
NeverENG force-pushed the feat/3472-generic-call-optimization branch from f314c38 to 634eecb Compare July 22, 2026 09:06
Comment thread client/client.go
//
// Whether the "class" field is kept in Map (generic=true) results is controlled by
// the global "generic.include.class" configuration (default true). It only affects
// the Map generalizer and currently has no per-reference override.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] 当前实现与本 PR 勾选的 API 验收项不一致

这里明确写成 generic.include.class 仅支持全局配置、没有 per-reference override,当前 Head 也已经删除 WithGenericIncludeClass;但 PR 的 Changes 和 Acceptance criteria 仍声称已新增该 API,并将该验收项标记完成。#3472 可以选择不提供用户侧 API,但需要明确收缩 scope;否则按当前 PR 声明,这项需求仍未实现。请二选一:实现一条能实际传递到 MapGeneralizer 的 per-reference 配置链路并补行为测试,或同步修改 PR 描述/验收范围,明确本 PR 只保留全局配置。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

采用「收缩 scope」这一选项。WithGenericIncludeClass 已删除,generic.include.class 保持仅全局配置并在 client/client.go 文档中说明其范围(Map-only、无 per-reference override)。PR 描述的 Changes / Acceptance criteria 已同步更新:per-reference API 一项标记为 scoped out,不再声称已实现;同时新增了「Triple 识别 bean 并路由到 $invoke invoker」的验收项。本 PR 未在代码中实现 per-reference 链路,留待生成器读取该参数后再补。

@NeverENG
NeverENG force-pushed the feat/3472-generic-call-optimization branch from 293f908 to 634eecb Compare July 22, 2026 14:03
NeverENG added a commit to NeverENG/dubbo-go that referenced this pull request Jul 23, 2026
…edicate

ValidateGenericType accepted "bean" but protocol/triple/triple.go:isGenericCall
did not, so a Triple reference created with WithGenericType("bean") was routed to
NewDubbo3Invoker instead of the $invoke-capable NewTripleInvoker.

Extract the accepted generic-mode decision into a shared internal.IsGenericMode
predicate and have both ValidateGenericType and Triple's isGenericCall use it, so
the accepted-mode set stays in sync. Triple's previously accepted modes
(true/gson/protobuf/protobuf-json) are unchanged; only "bean" is added.

filter/generic/util.go:isGeneric is intentionally left untouched: it omits the
legacy "protobuf", and routing it through the shared predicate would silently
change filter behavior. That convergence is tracked as a follow-up.

Addresses review feedback on apache#3500 (part of apache#3472).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
NeverENG added a commit to NeverENG/dubbo-go that referenced this pull request Jul 23, 2026
…edicate

ValidateGenericType accepted "bean" but protocol/triple/triple.go:isGenericCall
did not, so a Triple reference created with WithGenericType("bean") was routed to
NewDubbo3Invoker instead of the $invoke-capable NewTripleInvoker.

Extract the accepted generic-mode decision into a shared internal.IsGenericMode
predicate and have both ValidateGenericType and Triple's isGenericCall use it, so
the accepted-mode set stays in sync. Triple's previously accepted modes
(true/gson/protobuf/protobuf-json) are unchanged; only "bean" is added.

filter/generic/util.go:isGeneric is intentionally left untouched: it omits the
legacy "protobuf", and routing it through the shared predicate would silently
change filter behavior. That convergence is tracked as a follow-up.

Addresses review feedback on apache#3500 (part of apache#3472).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@NeverENG
NeverENG force-pushed the feat/3472-generic-call-optimization branch from bd5543d to e8ca1f7 Compare July 23, 2026 19:12
NeverENG added a commit to NeverENG/dubbo-go that referenced this pull request Jul 23, 2026
…edicate

ValidateGenericType accepted "bean" but protocol/triple/triple.go:isGenericCall
did not, so a Triple reference created with WithGenericType("bean") was routed to
NewDubbo3Invoker instead of the $invoke-capable NewTripleInvoker.

Extract the accepted generic-mode decision into a shared internal.IsGenericMode
predicate and have both ValidateGenericType and Triple's isGenericCall use it, so
the accepted-mode set stays in sync. Triple's previously accepted modes
(true/gson/protobuf/protobuf-json) are unchanged; only "bean" is added.

filter/generic/util.go:isGeneric is intentionally left untouched: it omits the
legacy "protobuf", and routing it through the shared predicate would silently
change filter behavior. That convergence is tracked as a follow-up.

Addresses review feedback on apache#3500 (part of apache#3472).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@NeverENG
NeverENG force-pushed the feat/3472-generic-call-optimization branch from e8ca1f7 to e97c376 Compare July 23, 2026 19:39
Comment thread internal/config.go
Comment thread internal/config.go
Fail fast on an unknown generic mode when the reference is created, instead
of silently falling back to the Map generalizer at runtime, and keep the
accepted generic-mode set consistent between reference-creation validation
and Triple's invoker routing.

- add internal.IsGenericMode as the single source of truth for the accepted
  generic modes (true/gson/protobuf-json/bean, plus legacy protobuf), and
  build internal.ValidateGenericType on top of it, rejecting unknown values
- wire the validation into ReferenceOptions.init
- route protocol/triple/triple.go:isGenericCall through internal.IsGenericMode
  so a reference created with WithGenericType("bean") is recognized as generic
  and dispatched to the $invoke-capable NewTripleInvoker instead of
  NewDubbo3Invoker
- clarify that generic mode and transport serialization are independent, and
  document the scope of generic.include.class, in WithGenericType and
  NewGenericService docs
- cover the validation and isGenericCall (including bean) with table-driven tests

Part of apache#3472

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: bang <3656828039@qq.com>
@NeverENG
NeverENG force-pushed the feat/3472-generic-call-optimization branch from e97c376 to cacd17c Compare July 24, 2026 06:04
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] 泛化调用优化

6 participants