feat(collections): add Address and Integer codecs #22517
Conversation
cool-develope
requested review from
aaronc,
testinginprod,
a team and
facundomedica
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing schema handling and value decoding processes. Key changes include the addition of error checks in decoding methods, new functions for schema type conversions, and improved validation for specific data types. The modifications aim to ensure more robust handling of various data types and streamline the encoding and decoding processes, while also marking certain existing codecs as deprecated. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
collections/indexing.go (1)
153-155: Consider refactoring decoder handling to reduce duplicationThe key and value decoder logic follows similar patterns. Consider extracting the common decoder handling logic into a helper function to improve maintainability and reduce code duplication.
Example refactor:
func decodeWithSchemaType(decoder codec.SchemaCodec, data []byte, decode func([]byte) (any, error)) (any, error) { x, err := decode(data) if err != nil { return nil, err } if decoder.ToSchemaType == nil { return x, nil } return decoder.ToSchemaType(x) }This helper could then be used for both key and value decoding:
res.keyDecoder = func(i []byte) (any, error) { return decodeWithSchemaType(keyDecoder, i, func(data []byte) (any, error) { _, x, err := c.m.kc.Decode(data) return x, err }) } res.valueDecoder = func(i []byte) (any, error) { return decodeWithSchemaType(valueDecoder, i, c.m.vc.Decode) }types/collections.go (2)
124-139: LGTM with a minor suggestion for error messagesThe implementation correctly handles schema encoding/decoding for address types with proper error handling and type assertions.
Consider enhancing the error message to include the actual value:
- return t, fmt.Errorf("expected string, got %T", s) + return t, fmt.Errorf("expected string, got %T: %v", s, s)
235-253: LGTM with suggestions for error handling and performanceThe implementation correctly handles schema encoding/decoding for math.Int with proper validation.
Consider these improvements:
- Enhance error messages to include the actual value:
- return math.Int{}, fmt.Errorf("expected string, got %T", s) + return math.Int{}, fmt.Errorf("expected string, got %T: %v", s, s)
- Optimize the error path by combining the parsing check and conversion:
- t, ok := math.NewIntFromString(sz) - if !ok { - return math.Int{}, fmt.Errorf("failed to parse Int from string: %s", sz) - } - return t, nil + if t, ok := math.NewIntFromString(sz); ok { + return t, nil + } + return math.Int{}, fmt.Errorf("failed to parse Int from string: %s", sz)collections/pair.go (3)
248-257: Refactor duplicated code for obtaining schema codecsThe code blocks for obtaining
codec1andcodec2fromcodec.KeySchemaCodecand handling errors are nearly identical. Refactoring this duplication into a helper function can improve maintainability and reduce redundancy.Here's how you might refactor:
func (p pairKeyCodec[K1, K2]) SchemaCodec() (codec.SchemaCodec[Pair[K1, K2]], error) { field1, err := getNamedKeyField(p.keyCodec1, p.key1Name) if err != nil { return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key1 field: %w", err) } field2, err := getNamedKeyField(p.keyCodec2, p.key2Name) if err != nil { return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key2 field: %w", err) } - codec1, err := codec.KeySchemaCodec(p.keyCodec1) - if err != nil { - return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key1 schema codec: %w", err) - } - - codec2, err := codec.KeySchemaCodec(p.keyCodec2) - if err != nil { - return codec.SchemaCodec[Pair[K1, K2]]{}, fmt.Errorf("error getting key2 schema codec: %w", err) - } + codec1, err := getKeySchemaCodec(p.keyCodec1, "key1") + if err != nil { + return codec.SchemaCodec[Pair[K1, K2]]{}, err + } + + codec2, err := getKeySchemaCodec(p.keyCodec2, "key2") + if err != nil { + return codec.SchemaCodec[Pair[K1, K2]]{}, err + }And define the helper function
getKeySchemaCodecto encapsulate the error handling.
261-269: Refactor duplicated code when converting to schema typesThe conversion of
k1andk2usingtoKeySchemaTypeis duplicated. Refactoring this into a loop or helper function can reduce redundancy and enhance code readability.Here's an example of how you might refactor:
- k1, err := toKeySchemaType(codec1, pair.K1()) - if err != nil { - return nil, err - } - k2, err := toKeySchemaType(codec2, pair.K2()) - if err != nil { - return nil, err - } - return []interface{}{k1, k2}, nil + keys := make([]interface{}, 2) + codecs := []codec.SchemaCodec{codec1, codec2} + pairValues := []any{pair.K1(), pair.K2()} + for i, cdc := range codecs { + k, err := toKeySchemaType(cdc, pairValues[i]) + if err != nil { + return nil, err + } + keys[i] = k + } + return keys, nil
276-284: Refactor duplicated code when converting from schema typesSimilarly, the code for converting
k1andk2from schema types is duplicated. Consider refactoring to reduce repetition and improve maintainability.Here's how you might adjust the code:
- k1, err := fromKeySchemaType(codec1, aSlice[0]) - if err != nil { - return Pair[K1, K2]{}, err - } - k2, err := fromKeySchemaType(codec2, aSlice[1]) - if err != nil { - return Pair[K1, K2]{}, err - } - return Join(k1, k2), nil + keys := make([]interface{}, 2) + codecs := []codec.SchemaCodec{codec1, codec2} + for i, cdc := range codecs { + k, err := fromKeySchemaType(cdc, aSlice[i]) + if err != nil { + return Pair[K1, K2]{}, err + } + keys[i] = k + } + return Join(keys[0].(K1), keys[1].(K2)), nil
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
collections/indexing.go(1 hunks)collections/pair.go(2 hunks)indexer/postgres/params.go(1 hunks)schema/kind.go(0 hunks)types/collections.go(3 hunks)
💤 Files with no reviewable changes (1)
- schema/kind.go
🧰 Additional context used
📓 Path-based instructions (4)
collections/indexing.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
collections/pair.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
indexer/postgres/params.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/collections.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
indexer/postgres/params.go (1)
111-115: LGTM! The changes look good but require verification.
The modification to handle address values as strings instead of byte slices is well-implemented with proper error handling and clear error messages.
Let's verify that all callers have been updated to pass string values:
✅ Verification successful
Verified: All callers have been updated to pass string values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential callers of bindParam that might be affected by the AddressKind change
# Look for any code that might still be passing []byte for AddressKind fields
# Search for struct field declarations of AddressKind type
rg -l "schema\.AddressKind" | xargs -I {} rg -A 5 "type.*struct" {}
# Search for direct usage of bindParam with byte slices
ast-grep --pattern 'bindParam($_, []byte{$$$})'
Length of output: 1406
collections/indexing.go (1)
153-155: LGTM: Consistent nil check for schema type conversion
The added nil check for valueDecoder.ToSchemaType is a good defensive programming practice that:
- Maintains consistency with the key decoder's behavior
- Prevents potential nil pointer dereferences
- Provides a clean fallback to return the original decoded value
| func fromKeySchemaType[T any](cdc codec.SchemaCodec[T], key any) (T, error) { | ||
| if cdc.FromSchemaType != nil { | ||
| return cdc.FromSchemaType(key) | ||
| } | ||
| return key.(T), nil | ||
| } |
There was a problem hiding this comment.
Handle type assertions safely to prevent potential panics
In the fromKeySchemaType function, the type assertion key.(T) may cause a panic if key is not of type T. Use the comma-ok idiom to safely perform the type assertion and handle the error accordingly.
Here's how you can modify the code:
func fromKeySchemaType[T any](cdc codec.SchemaCodec[T], key any) (T, error) {
if cdc.FromSchemaType != nil {
return cdc.FromSchemaType(key)
}
- return key.(T), nil
+ tKey, ok := key.(T)
+ if !ok {
+ var zero T
+ return zero, fmt.Errorf("expected type %T, got %T", zero, key)
+ }
+ return tKey, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func fromKeySchemaType[T any](cdc codec.SchemaCodec[T], key any) (T, error) { | |
| if cdc.FromSchemaType != nil { | |
| return cdc.FromSchemaType(key) | |
| } | |
| return key.(T), nil | |
| } | |
| func fromKeySchemaType[T any](cdc codec.SchemaCodec[T], key any) (T, error) { | |
| if cdc.FromSchemaType != nil { | |
| return cdc.FromSchemaType(key) | |
| } | |
| tKey, ok := key.(T) | |
| if !ok { | |
| var zero T | |
| return zero, fmt.Errorf("expected type %T, got %T", zero, key) | |
| } | |
| return tKey, nil | |
| } |
|
@cool-develope your pull request is missing a changelog! |
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
genericAddressKeyandintValueCodectypes.Improvements
DecimalKindto ensure values conform to expected formats.bindParammethod.Deprecations