Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #2517 by adding proper handling for typed nil pointers in the Hessian encoder. Previously, typed nil pointers like (*int32)(nil) would not be correctly identified as nil, causing encoding issues. The fix adds an early check using reflection to detect nil pointers of any type and encode them as Hessian null.
- Added reflection-based nil pointer detection before the type switch
- Added comprehensive tests for typed nil pointers on basic types
- Ensures typed nil pointers are encoded as byte 'N' (Hessian null)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| encode.go | Added early nil pointer check using reflect.ValueOf() to detect and encode typed nil pointers as Hessian null before entering the type switch |
| encode_test.go | Added TestTypedNilPointerEncode with subtests for nil pointers to int32, string, bool, and float64 types |
Comments suppressed due to low confidence (1)
encode.go:111
- The
case nil:branch is now unreachable. Ifv == nil, it would have been caught by the check at lines 96-98. The typed nil pointer check at lines 102-106 handles all other nil scenarios. Consider removing this redundant case.
case nil:
e.buffer = EncNull(e.buffer)
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@yumosx unit test TestMultipleLevelRecursiveDep2 failed, u can update the test and re-run the CI to check the reason: decoded := fmt.Sprintf("%v", obj)
origin := fmt.Sprintf("%v", data)
if decoded != origin {
t.Errorf("deserialize mismatched, origin: %s, decoded: %s", origin, decoded)
} |
|
@wongoo @AlexStocks The CI failure issue has been fully resolved by #387 |
AlexStocks
left a comment
There was a problem hiding this comment.
Code Review: handle typed nil pointers in encode
评价
与 PR #376 解决同一问题,修复方案一致。本 PR 的测试覆盖更清晰。
[P1] 规范级问题
1. 与 PR #376 的关系
本 PR 与 #376 功能完全重叠,建议:
- 关闭其中一个
- 或合并两者的改动
2. 测试组织
测试用例组织清晰,使用了 table-driven 测试模式。但建议添加:
t.Run("nilStructPointer", func(t *testing.T) {
type TestStruct struct{ Field int }
var nilStruct *TestStruct
data, err := encodeTarget(nilStruct)
assert.Nil(t, err)
assert.Equal(t, []byte{'N'}, data)
})总体评价:修复正确,建议与 #376 合并。

What this PR does:
Which issue(s) this PR fixes:
Fixes apache/dubbo-go#2517
Special notes for your reviewer:
Does this PR introduce a user-facing change?: