Skip to content

test: verify typed nil scalar pointers encode as null - #394

Merged
AlexStocks merged 1 commit into
apache:masterfrom
mochengqian:test-typed-nil-pointer-encoding
Apr 29, 2026
Merged

test: verify typed nil scalar pointers encode as null#394
AlexStocks merged 1 commit into
apache:masterfrom
mochengqian:test-typed-nil-pointer-encoding

Conversation

@mochengqian

Copy link
Copy Markdown
Contributor

Description

Refs apache/dubbo-go#2517
Refs #376
Refs #384

Why

PR #376 and PR #384 both try to fix typed nil pointer encoding by adding a generic reflect-based nil pointer check at the beginning of Encoder.Encode.

After checking current master, the specific hessian2 case reported in apache/dubbo-go#2517 is already handled:

  • (*int32)(nil) is routed to encTypeInt32
  • encTypeInt32 already returns Hessian null (N) for nil pointers
  • the existing TestNullInt32Ptr already covers the exact (*int32)(nil) case

So the right follow-up is not another production-code change. The useful missing piece is a focused boundary test proving that typed nil scalar pointers are already encoded as Hessian null on current master.

Root Cause:

The old issue report was correct at the time: v == nil alone cannot detect typed nil pointers such as (*int32)(nil).

However, current master already handles the reported hessian2 path through the existing typed int32 encoder logic. The remaining confusion comes from the lack of an explicit boundary test covering typed nil scalar pointers together.

PR Goal:

Add a verification-only test PR that proves typed nil scalar pointers encode as Hessian null.

Provide clear evidence that #376 and #384 are no longer needed as production-code fixes.

Avoid adding a broad generic reflect nil-pointer branch to Encoder.Encode when the reported behavior is already covered by existing encoder paths.

Success Criteria:

(*int32)(nil) encodes to Hessian null (N).

Other typed nil scalar pointers covered by this test also encode to Hessian null (N):

  • (*bool)(nil)
  • (*string)(nil)
  • (*float64)(nil)

No production code is changed.

go test ./... passes.

Paths this PR should affect:

null_test.go

Revise

Adding TestNullTypedScalarPointers: proves typed nil scalar pointers are encoded as Hessian null on current master.

Keeping production code unchanged: avoids duplicating #376/#384's generic reflect check and keeps encoder behavior scoped to existing implementation paths.

Using Refs instead of Fixes: apache/dubbo-go#2517 also mentions dubbo-go protocol-layer error propagation, so this hessian2 PR should not claim to close the whole cross-repo issue by itself.

Tests

go test ./...

@mochengqian

Copy link
Copy Markdown
Contributor Author

For the dubbo-go protocol layer, current marshalRequest already returns encoder errors from request arguments instead of silently ignoring them, and TestMarshalRequestWithTypedNilPointer verifies the request path with (*int32)(nil).

So from the current codebase, both sides of apache/dubbo-go#2517 are covered. I think this issue can be closed after the hessian2 verification PR is merged. @tiltwind

@wongoo wongoo 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.

LGTM

@tiltwind tiltwind 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.

GOOD WORK

@AlexStocks
AlexStocks merged commit 0c80a33 into apache:master Apr 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants