Skip to content

refactor(hessian2): use generic exception from hessian2#3378

Open
leno23 wants to merge 2 commits into
apache:developfrom
leno23:codex/migrate-generic-exception-3361
Open

refactor(hessian2): use generic exception from hessian2#3378
leno23 wants to merge 2 commits into
apache:developfrom
leno23:codex/migrate-generic-exception-3361

Conversation

@leno23

@leno23 leno23 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #3361.

This PR migrates Dubbo generic exception handling to the type provided by dubbo-go-hessian2:

  • removes the local GenericException struct from protocol/dubbo/hessian2
  • makes ToGenericException return *java_exception.DubboGenericException
  • updates response encoding paths to encode the hessian2 exception type directly
  • updates tests to assert that decoded generic exceptions use the hessian2 type

Validation

  • go test ./protocol/dubbo/hessian2 -run 'TestToGenericExceptionUsesHessianExceptionType|TestIsSupportResponseAttachment|TestVersion2Int'
  • go test ./protocol/dubbo/impl -run 'TestHessianSerializer|TestMarshalResponse|TestUnmarshalResponseBody'
  • go test ./protocol/dubbo/hessian2 ./protocol/dubbo/impl
  • git diff --check

Checklist

  • I confirm the target branch is main
  • Code has passed local testing
  • I have added tests that prove my fix is effective or that my feature works

@Alanxtl

Alanxtl commented Jun 8, 2026

Copy link
Copy Markdown
Member

you should contribute to https://github.com/apache/dubbo-go-hessian2 ,
the purpose of this issue is to move these definations to dubbo-go-hessian2 instead of define these definations in dubbo-go

@Alanxtl

Alanxtl commented Jun 9, 2026

Copy link
Copy Markdown
Member

please update all of your prs, your base branch should be develop and your target branch should be develop

@Alanxtl Alanxtl added ✏️ Feature 3.3.2 version 3.3.2 labels Jun 9, 2026
@leno23 leno23 changed the base branch from main to develop June 9, 2026 03:26
@leno23 leno23 force-pushed the codex/migrate-generic-exception-3361 branch from efb95eb to 2f19e80 Compare June 9, 2026 03:27
@leno23

leno23 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Updated this PR to target develop and rebased the branch on upstream/develop. The local validation still passes: go test ./protocol/dubbo/hessian2 ./protocol/dubbo/impl and git diff --check. The previous failed CI job failed during Codecov CLI signature verification (gpg: Can't check signature: No public key), after Go tests had completed, so there was no code/test failure in that run. Regarding the hessian2 direction: this PR removes the local dubbo-go GenericException definition and uses dubbo-go-hessian2's java_exception.DubboGenericException directly.

@codecov-commenter

codecov-commenter commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.90%. Comparing base (60d1c2a) to head (74528bb).
⚠️ Report is 825 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3378      +/-   ##
===========================================
+ Coverage    46.76%   52.90%   +6.13%     
===========================================
  Files          295      493     +198     
  Lines        17172    37953   +20781     
===========================================
+ Hits          8031    20080   +12049     
- Misses        8287    16250    +7963     
- Partials       854     1623     +769     

☔ 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.

@leno23 leno23 force-pushed the codex/migrate-generic-exception-3361 branch from 2f19e80 to 1b979bd Compare June 9, 2026 03:55
@leno23

leno23 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Added focused tests for the Codecov patch coverage feedback. The new coverage exercises ToGenericException conversions, GenericException DetailMessage formatting, and the pointer DubboGenericException encode paths in both hessian2 packResponse and impl marshalResponse. Local validation: go test ./protocol/dubbo/hessian2 ./protocol/dubbo/impl, go test -cover ./protocol/dubbo/hessian2 ./protocol/dubbo/impl, git diff --check, and the local audit script all pass.

@leno23 leno23 force-pushed the codex/migrate-generic-exception-3361 branch from 1b979bd to 0fe7af9 Compare June 9, 2026 04:02
@leno23

leno23 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up for the remaining Codecov patch coverage warning: added packResponse coverage for the value DubboGenericException and Throwabler branches. Local validation still passes: go test ./protocol/dubbo/hessian2 ./protocol/dubbo/impl, go test -cover ./protocol/dubbo/hessian2 ./protocol/dubbo/impl, git diff --check, and the local audit script.

@leno23

leno23 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on the hessian2-side direction from the review: I opened apache/dubbo-go-hessian2#396 to complete the library-side change. It makes java_exception.DubboGenericException populate and expose a stable detail message from ExceptionClass / ExceptionMessage, while preserving decoded Java DetailMessage behavior. With that hessian2 PR in place, this dubbo-go PR can stay focused on removing the local GenericException definition and using the hessian2 type.

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 addresses #3361 by refactoring Dubbo Hessian2 generic exception handling to use the dubbo-go-hessian2-provided java_exception.DubboGenericException type instead of a locally-defined GenericException, aligning exception encoding/decoding with the shared Hessian2 library.

Changes:

  • Remove the local GenericException type usage and migrate encoding paths to directly encode java_exception.DubboGenericException.
  • Update ToGenericException to return *java_exception.DubboGenericException and add helper logic for legacy/throwable conversions.
  • Update and extend tests to assert the Hessian2 generic exception type is used end-to-end.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
protocol/dubbo/impl/hessian.go Encodes DubboGenericException directly in response marshalling.
protocol/dubbo/impl/hessian_test.go Updates response marshal/unmarshal tests to assert Hessian2 exception types (including pointer/value cases).
protocol/dubbo/hessian2/hessian_response.go Refactors ToGenericException to return Hessian2 DubboGenericException and adjusts response exception encoding.
protocol/dubbo/hessian2/hessian_response_test.go Adds focused tests for ToGenericException conversions and generic-exception response packing.

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

Comment thread protocol/dubbo/hessian2/hessian_response_test.go
@sonarqubecloud

Copy link
Copy Markdown

@Alanxtl

Alanxtl commented Jun 13, 2026

Copy link
Copy Markdown
Member

@Snow-kal @xxs588 @CAICAIIs help review this and apache/dubbo-go-hessian2#396

@xxs588

xxs588 commented Jun 14, 2026

Copy link
Copy Markdown
Contributor
  1. DetailMessage 的格式化逻辑在 fix(java_exception): populate dubbo generic exception message dubbo-go-hessian2#396 构造器和这个 PR 的 newGenericException 里都写了一遍如果构造器那边已经负责设置 DetailMessage 的话,这边是否可以直接用构造器,避免两边维护同一套逻辑?

  2. Copilot 提到的测试 case(wantDetailString 为空导致断言 Error() == "")这个是否需要补回来捏?

以上仅供参考

@leno23

leno23 commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, I agree with the direction. I checked the current dependency versions: github.com/apache/dubbo-go-hessian2 v1.13.1 still has NewDubboGenericException only populate ExceptionClass / ExceptionMessage, so removing newGenericException here now would make Error() return an empty DetailMessage and break the behavior covered by these tests.

The constructor-side fix is still in apache/dubbo-go-hessian2#396 and is not available from an upstream module version yet. Once that hessian2 PR is merged and available to dubbo-go, I can update the dependency here and replace newGenericException with the constructor directly to avoid maintaining the formatting in both places.

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] Migrate GenericException from dubbo-go to dubbo-go-hessian2

6 participants