feat(rain): expand model scanning and custom type mapping#19
Conversation
Greptile SummaryThis PR refactors the row-scan pipeline in Key findings:
Confidence Score: 4/5Safe to merge after fixing the scanner-only pointer type gap in One P1 logic bug: pointer fields implementing
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fieldType := field.Type() | ||
| if fieldType.Implements(scannerType) && fieldType.Implements(valuerType) { | ||
| receiver := reflect.New(fieldType.Elem()) | ||
| if receiver.Type().Implements(scannerType) { | ||
| return receiver.Interface(), func() error { | ||
| field.Set(receiver) | ||
| return nil | ||
| } | ||
| value := holder.V | ||
| field.Set(reflect.ValueOf(&value)) | ||
| }, true | ||
| } | ||
| } |
There was a problem hiding this comment.
The first branch inside the pointer case requires that fieldType implements both scannerInterface and valuerInterface before handling it. Any *T type that implements sql.Scanner (pointer receiver) but does not implement driver.Valuer will fall through this block.
The second branch (fieldType.Elem().Implements(scannerType)) only rescues types where T — not *T — has a value-receiver Scan method, which is uncommon. So the typical custom scanner type:
type Status string
func (s *Status) Scan(src any) error { ... } // pointer receiver, no Value()…used as Status *Status on a struct will reach nullablePrimitiveHandlers(), find no match, and return "rain: unsupported nullable field type *Status" — silently failing at runtime despite the type correctly implementing the standard sql.Scanner interface.
The driver.Valuer requirement is not necessary for scan-path correctness and should be removed from the guard. The same receiver.Interface()/field.Set(receiver) pattern works regardless of whether the type also satisfies Valuer:
if fieldType.Implements(scannerType) {
receiver := reflect.New(fieldType.Elem())
if receiver.Type().Implements(scannerType) {
return receiver.Interface(), func() error {
field.Set(receiver)
return nil
}, true
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/model.go
Line: 264-273
Comment:
**Scanner-only pointer types silently unsupported**
The first branch inside the pointer case requires that `fieldType` implements *both* `scannerInterface` and `valuerInterface` before handling it. Any `*T` type that implements `sql.Scanner` (pointer receiver) but does **not** implement `driver.Valuer` will fall through this block.
The second branch (`fieldType.Elem().Implements(scannerType)`) only rescues types where `T` — not `*T` — has a value-receiver `Scan` method, which is uncommon. So the typical custom scanner type:
```go
type Status string
func (s *Status) Scan(src any) error { ... } // pointer receiver, no Value()
```
…used as `Status *Status` on a struct will reach `nullablePrimitiveHandlers()`, find no match, and return `"rain: unsupported nullable field type *Status"` — silently failing at runtime despite the type correctly implementing the standard `sql.Scanner` interface.
The `driver.Valuer` requirement is not necessary for scan-path correctness and should be removed from the guard. The same `receiver.Interface()`/`field.Set(receiver)` pattern works regardless of whether the type also satisfies `Valuer`:
```go
if fieldType.Implements(scannerType) {
receiver := reflect.New(fieldType.Elem())
if receiver.Type().Implements(scannerType) {
return receiver.Interface(), func() error {
field.Set(receiver)
return nil
}, true
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| if fieldType.Implements(scannerType) && fieldType.Implements(valuerType) { | ||
| receiver := reflect.New(fieldType.Elem()) | ||
| if receiver.Type().Implements(scannerType) { | ||
| return receiver.Interface(), func() error { | ||
| field.Set(receiver) | ||
| return nil | ||
| } | ||
| value := holder.V | ||
| field.Set(reflect.ValueOf(&value)) | ||
| }, true | ||
| } |
There was a problem hiding this comment.
Implements check is always true (dead branch)
When the outer condition on line 265 is satisfied — fieldType.Implements(scannerType) is true — receiver is created as reflect.New(fieldType.Elem()), whose type is exactly fieldType (i.e. *T). Therefore receiver.Type().Implements(scannerType) evaluates the same expression as the outer guard and is always true inside this block.
The inner if guard on line 267 can never be false, so no code path exists where execution would reach the closing } of the outer block without having returned. This should be simplified:
if fieldType.Implements(scannerType) && fieldType.Implements(valuerType) {
receiver := reflect.New(fieldType.Elem())
return receiver.Interface(), func() error {
field.Set(receiver)
return nil
}, true
}Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/model.go
Line: 265-272
Comment:
**Inner `Implements` check is always true (dead branch)**
When the outer condition on line 265 is satisfied — `fieldType.Implements(scannerType)` is `true` — `receiver` is created as `reflect.New(fieldType.Elem())`, whose type is exactly `fieldType` (i.e. `*T`). Therefore `receiver.Type().Implements(scannerType)` evaluates the same expression as the outer guard and is always `true` inside this block.
The inner `if` guard on line 267 can never be `false`, so no code path exists where execution would reach the closing `}` of the outer block without having returned. This should be simplified:
```go
if fieldType.Implements(scannerType) && fieldType.Implements(valuerType) {
receiver := reflect.New(fieldType.Elem())
return receiver.Interface(), func() error {
field.Set(receiver)
return nil
}, true
}
```
How can I resolve this? If you propose a fix, please make it concise.| signed := map[reflect.Kind]bool{ | ||
| reflect.Int: true, | ||
| reflect.Int8: true, | ||
| reflect.Int16: true, | ||
| reflect.Int32: true, | ||
| reflect.Int64: true, | ||
| } | ||
| if !signed[elemType.Kind()] { | ||
| return nil, nil, false |
There was a problem hiding this comment.
nullableSignedIntTarget and nullableUnsignedIntTarget both construct a fresh map[reflect.Kind]bool on every invocation. These functions are called once per column per scanned row, so in high-throughput scenarios this adds heap pressure without any benefit.
Consider promoting them to package-level variables, or replace the map lookup with a switch statement which requires no allocation:
func nullableSignedIntTarget(field reflect.Value) (any, func() error, bool) {
elemType := field.Type().Elem()
switch elemType.Kind() {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
default:
return nil, nil, false
}
// ...
}The same applies to the unsigned map in nullableUnsignedIntTarget (lines 331–337).
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/model.go
Line: 305-313
Comment:
**Map allocated on every call in hot scan path**
`nullableSignedIntTarget` and `nullableUnsignedIntTarget` both construct a fresh `map[reflect.Kind]bool` on every invocation. These functions are called once per column per scanned row, so in high-throughput scenarios this adds heap pressure without any benefit.
Consider promoting them to package-level variables, or replace the map lookup with a `switch` statement which requires no allocation:
```go
func nullableSignedIntTarget(field reflect.Value) (any, func() error, bool) {
elemType := field.Type().Elem()
switch elemType.Kind() {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
default:
return nil, nil, false
}
// ...
}
```
The same applies to the `unsigned` map in `nullableUnsignedIntTarget` (lines 331–337).
How can I resolve this? If you propose a fix, please make it concise.
cungminh2710
deleted the
codex/linear-mention-qc-230-expand-model-scanning-and-custom-typ
branch
1 participant
Motivation
Description
pkg/rain/model.gointo a handler-based system withnullableHandlerfunctions to make adding new nullable types straightforward.sql.Scanner(and valuer-aware types) can be scanned directly into custom value objects when practical.fieldByIndexAllocandembeddedStructTypehelpers.pkg/rain/model_internal_test.goexercising expanded nullable primitives, customsql.Scannertypes, embedded struct allocation/scan, unknown columns, and a clear error path for unsupported nullable types.Testing
go test ./pkg/rain -run TestScanRows -count=1and the targeted scan tests passed.make fmtandmake lintwheremake lintreported0 issues.make testand all packages passed.Codex Task