fix bigint unsigned null column scan to err type int64 #1612
Conversation
WalkthroughThe update introduces Jiabin Zhang as a new contributor in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- AUTHORS (1 hunks)
- fields.go (2 hunks)
Files skipped from review due to trivial changes (1)
- AUTHORS
Additional comments not posted (2)
fields.go (2)
131-131: LGTM!The declaration of
scanTypeNullUIntto handlesql.Null[uint64]{}is appropriate for addressing the issue of out-of-range errors when scanninguint64values intoNullInt64.
189-191: LGTM!The modification to the
scanTypemethod to returnscanTypeNullUIntwhen theflagUnsignedflag is set is correct. This ensures proper handling of unsigned bigint null columns.
|
We don't drop Go 1.21 support yet. So we can not use sql.Null without dirty build tag hacks. |
ok，i just notice sql.Null introduced in 1.22 |
…ause sql.Null introduce in go1.22
Can we use sql.NullString to temporarily support all supported version avoid this problem (out of range) |
|
I prefer 1 breaking change over 2 breaking changes. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
AUTHORS (1)
125-125: Maintain alphabetical ordering in AUTHORS file.The new entry "Jiabin Zhang" should be placed between "Zhang Xiang" and "Zhenye Xie" to maintain alphabetical ordering.
Move the entry to the correct position:
Zhang Xiang <angwerzx at 126.com> +Jiabin Zhang <jiabin.z at qq.com> Zhenye Xie <xiezhenye at gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
fields.go (1)
189-191: Fix indentation for consistency.The logic for handling unsigned bigint nulls is correct, but the indentation doesn't match the surrounding code style.
Apply this formatting fix:
- if mf.flags&flagUnsigned != 0 { - return scanTypeNullUint - } + if mf.flags&flagUnsigned != 0 { + return scanTypeNullUint + }
| @@ -128,6 +128,7 @@ | |||
| scanTypeInt64 = reflect.TypeOf(int64(0)) | |||
| scanTypeNullFloat = reflect.TypeOf(sql.NullFloat64{}) | |||
| scanTypeNullInt = reflect.TypeOf(sql.NullInt64{}) | |||
| scanTypeNullUint = reflect.TypeOf(sql.Null[uint64]{}) | |||
There was a problem hiding this comment.
Breaking change: sql.Null requires Go 1.22.
As mentioned in the PR discussion, Go 1.21 support hasn't been dropped yet. Using sql.Null[uint64] would break compatibility with Go 1.21 and earlier versions.
Consider one of these alternatives:
- Use build tags to conditionally include this for Go 1.22+
- Create a custom NullUint64 type that implements the required interfaces:
// NullUint64 represents a uint64 that may be null
type NullUint64 struct {
Uint64 uint64
Valid bool
}
// Implement Scanner and Valuer interfaces
func (n *NullUint64) Scan(value any) error {
if value == nil {
n.Uint64, n.Valid = 0, false
return nil
}
n.Valid = true
return convertAssign(&n.Uint64, value)
}
func (n NullUint64) Value() (driver.Value, error) {
if !n.Valid {
return nil, nil
}
return n.Uint64, nil
}| if mf.flags&flagUnsigned != 0 { | ||
| return scanTypeNullUint | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the test content examination, I can now provide a final response:
Test coverage for unsigned bigint nulls is insufficient
The codebase has tests for unsigned bigint (biguint) but only covers the non-null case with basic values (0, 65535, 42). There are no specific test cases for:
- Null unsigned bigint values
- Edge cases near uint64 maximum values
- Values exceeding int64 range
- Add test cases in
driver_test.goforscanTypeNullUintsimilar to existingbigintnulltests - Include edge cases testing uint64 values > math.MaxInt64
🔗 Analysis chain
Verify test coverage for unsigned bigint nulls.
Let's ensure the changes are properly tested, especially for edge cases where uint64 values exceed int64 range.
Let's examine the specific test content in driver_test.go since it appears in both searches.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests involving unsigned bigint columns
echo "Searching for tests with unsigned bigint columns..."
rg -l --type go "(?i)(unsigned.*bigint|uint64.*null)" *_test.go
# Search for existing null scanning tests
echo "Searching for null scanning tests..."
rg -l --type go "(?i)(nullint|nulluint|null.*scan)" *_test.go
Length of output: 755
Script:
#!/bin/bash
# Check the content of driver_test.go for unsigned bigint and null scanning tests
echo "Examining driver_test.go for unsigned bigint and null scanning tests..."
rg -A 10 -B 2 "(?i)(unsigned.*bigint|uint64.*null|nullint|nulluint|null.*scan)" driver_test.go
Length of output: 11427
Description
when i use gorm scan sql rows to map[string]any, i got an error (out of range).
gorm scan
sql.Rowstomap[string]anybyScanRowsthere has three step:
in step 2, when occur number(tiny int short long longlong )null column, this type will be all set to
scanTypeNullInt,tiny int short long type will be ok, its Maximum value will not out of range int64
but unsigned longlong may be out of range.
int64:-9223372036854775808 ~9223372036854775807,
uint64:0 ~ 18446744073709551615
a uint64 assign to NullInt64 will be out of range, because the value store into int64
so to fix it
Must add a new scan type NullUint64 to represent unsigned bigint null column.
and though flag to judge unsigned or not.
Dur to
database/sqlsupport generics Null[T] type, it will be easy to support NullUint64.Reproduce the problem
table example
code
Env
Error log
Checklist
Summary by CodeRabbit
Documentation
AUTHORSfile to include Jiabin Zhang as a contributor.Enhancements