feat(indexer/base): schema and value validation #20665
Conversation
WalkthroughThe updates enhance validation mechanisms throughout the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IndexerBase
participant Field
participant EnumDefinition
participant ModuleSchema
Client->>IndexerBase: Create/Update EnumDefinition
IndexerBase->>EnumDefinition: Validate()
EnumDefinition-->>IndexerBase: Validation Result
Client->>IndexerBase: Create/Update Field
IndexerBase->>Field: Validate()
Field-->>IndexerBase: Validation Result
Client->>IndexerBase: Create/Update ModuleSchema
IndexerBase->>ModuleSchema: Validate()
ModuleSchema-->>IndexerBase: Validation Result
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
|
|
|
aaronc
changed the title
|
|
|
|
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
indexer/base/kind.go
Outdated
| return "float32" | ||
| case Float64Kind: | ||
| return "float64" | ||
| case Bech32AddressKind: |
Check warning
Code scanning / CodeQL
Directly using the bech32 constants Warning
| } | ||
|
|
||
| // address prefix only valid with Bech32AddressKind | ||
| if c.Kind == Bech32AddressKind && c.AddressPrefix == "" { |
Check warning
Code scanning / CodeQL
Directly using the bech32 constants Warning
| // address prefix only valid with Bech32AddressKind | ||
| if c.Kind == Bech32AddressKind && c.AddressPrefix == "" { | ||
| return fmt.Errorf("missing address prefix for field %q", c.Name) | ||
| } else if c.Kind != Bech32AddressKind && c.AddressPrefix != "" { |
Check warning
Code scanning / CodeQL
Directly using the bech32 constants Warning
…into aaronc/indexer-base-validation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- indexer/base/fields.go (1 hunks)
- indexer/base/fields_test.go (1 hunks)
- indexer/base/object_type.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/base/fields.go
- indexer/base/object_type.go
Additional context used
Path-based instructions (1)
indexer/base/fields_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
| func TestValidateForKeyFields(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| keyFields []Field | ||
| key interface{} | ||
| errContains string | ||
| }{ | ||
| { | ||
| name: "no key fields", | ||
| keyFields: nil, | ||
| key: nil, | ||
| }, | ||
| { | ||
| name: "single key field, valid", | ||
| keyFields: object1Type.KeyFields, | ||
| key: "hello", | ||
| errContains: "", | ||
| }, | ||
| { | ||
| name: "single key field, invalid", | ||
| keyFields: object1Type.KeyFields, | ||
| key: []interface{}{"value"}, | ||
| errContains: "invalid value", | ||
| }, | ||
| { | ||
| name: "multiple key fields, valid", | ||
| keyFields: object2Type.KeyFields, | ||
| key: []interface{}{"hello", int32(42)}, | ||
| }, | ||
| { | ||
| name: "multiple key fields, not a slice", | ||
| keyFields: object2Type.KeyFields, | ||
| key: map[string]interface{}{"field1": "hello", "field2": "42"}, | ||
| errContains: "expected slice of values", | ||
| }, | ||
| { | ||
| name: "multiple key fields, wrong number of values", | ||
| keyFields: object2Type.KeyFields, | ||
| key: []interface{}{"hello"}, | ||
| errContains: "expected 2 key fields", | ||
| }, | ||
| { | ||
| name: "multiple key fields, invalid value", | ||
| keyFields: object2Type.KeyFields, | ||
| key: []interface{}{"hello", "abc"}, | ||
| errContains: "invalid value", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := ValidateForKeyFields(tt.keyFields, tt.key) | ||
| if tt.errContains == "" { | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| } else { | ||
| if err == nil || !strings.Contains(err.Error(), tt.errContains) { | ||
| t.Fatalf("expected error to contain %q, got: %v", tt.errContains, err) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider enhancing the error checks in TestValidateForKeyFields to verify the type of errors, not just the error messages.
This ensures that the function not only returns the expected message but also the correct error type, which is crucial for debugging and maintenance.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := ValidateForKeyFields(tt.keyFields, tt.key) | ||
| if tt.errContains == "" { | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| } else { | ||
| if err == nil || !strings.Contains(err.Error(), tt.errContains) { | ||
| t.Fatalf("expected error to contain %q, got: %v", tt.errContains, err) | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Refactor error handling in TestValidateForKeyFields to use error types or constants instead of string contains checks.
This change would make the tests less fragile to changes in the error message wording and improve the robustness of the test suite.
| func TestValidateForValueFields(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| valueFields []Field | ||
| value interface{} | ||
| errContains string | ||
| }{ | ||
| { | ||
| name: "no value fields", | ||
| valueFields: nil, | ||
| value: nil, | ||
| }, | ||
| { | ||
| name: "single value field, valid", | ||
| valueFields: []Field{ | ||
| { | ||
| Name: "field1", | ||
| Kind: StringKind, | ||
| }, | ||
| }, | ||
| value: "hello", | ||
| errContains: "", | ||
| }, | ||
| { | ||
| name: "value updates, empty", | ||
| valueFields: object3Type.ValueFields, | ||
| value: MapValueUpdates(map[string]interface{}{}), | ||
| }, | ||
| { | ||
| name: "value updates, 1 field valid", | ||
| valueFields: object3Type.ValueFields, | ||
| value: MapValueUpdates(map[string]interface{}{ | ||
| "field1": "hello", | ||
| }), | ||
| }, | ||
| { | ||
| name: "value updates, 2 fields, 1 invalid", | ||
| valueFields: object3Type.ValueFields, | ||
| value: MapValueUpdates(map[string]interface{}{ | ||
| "field1": "hello", | ||
| "field2": "abc", | ||
| }), | ||
| errContains: "expected int32", | ||
| }, | ||
| { | ||
| name: "value updates, extra value", | ||
| valueFields: object3Type.ValueFields, | ||
| value: MapValueUpdates(map[string]interface{}{ | ||
| "field1": "hello", | ||
| "field2": int32(42), | ||
| "field3": "extra", | ||
| }), | ||
| errContains: "unexpected values", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := ValidateForValueFields(tt.valueFields, tt.value) | ||
| if tt.errContains == "" { | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| } else { | ||
| if err == nil || !strings.Contains(err.Error(), tt.errContains) { | ||
| t.Fatalf("expected error to contain %q, got: %v", tt.errContains, err) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding checks for specific error types in TestValidateForValueFields to ensure that the correct exceptions are being thrown.
This would enhance the test's ability to verify not only the occurrence of an error but its specific type, aiding in precise troubleshooting and validation.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := ValidateForValueFields(tt.valueFields, tt.value) | ||
| if tt.errContains == "" { | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| } else { | ||
| if err == nil || !strings.Contains(err.Error(), tt.errContains) { | ||
| t.Fatalf("expected error to contain %q, got: %v", tt.errContains, err) | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Refactor error handling in TestValidateForValueFields to use error types or constants instead of string contains checks.
Using error types or constants would make the tests more reliable and less dependent on specific error message content, which might change over time.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- indexer/base/module_schema.go (1 hunks)
- indexer/base/module_schema_test.go (1 hunks)
- indexer/base/object_type.go (2 hunks)
- indexer/base/object_type_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/base/module_schema.go
- indexer/base/module_schema_test.go
- indexer/base/object_type.go
- indexer/base/object_type_test.go
| // Bech32AddressKind is a bech32 address type and values of this type must be of the go type string or []byte | ||
| // or a type which implements fmt.Stringer. Fields of this type are expected to set the AddressPrefix field | ||
| // in the field definition to the bech32 address prefix. | ||
| // Bech32AddressKind is a bech32 address type and values of this type must be of the go type []byte. |
There was a problem hiding this comment.
I switched the requirement to be just []bytes so the validation logic doesn't need to do any bech32 parsing here (although it is a pretty lightweight dependency). Also wondering if we need a length limit on addresses or not.
There was a problem hiding this comment.
How about a length limit of 64 or 128 bytes for addresses?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (12)
- indexer/base/enum.go (1 hunks)
- indexer/base/enum_test.go (1 hunks)
- indexer/base/field.go (2 hunks)
- indexer/base/field_test.go (1 hunks)
- indexer/base/kind.go (4 hunks)
- indexer/base/kind_test.go (1 hunks)
- indexer/base/listener.go (2 hunks)
- indexer/base/module_schema_test.go (1 hunks)
- indexer/base/name.go (1 hunks)
- indexer/base/name_test.go (1 hunks)
- indexer/base/object_type.go (2 hunks)
- indexer/base/object_type_test.go (1 hunks)
Files not reviewed due to errors (3)
- indexer/base/name.go (no review received)
- indexer/base/name_test.go (no review received)
- indexer/base/listener.go (no review received)
Files skipped from review as they are similar to previous changes (9)
- indexer/base/enum.go
- indexer/base/enum_test.go
- indexer/base/field.go
- indexer/base/field_test.go
- indexer/base/kind.go
- indexer/base/kind_test.go
- indexer/base/module_schema_test.go
- indexer/base/object_type.go
- indexer/base/object_type_test.go
Additional context used
Path-based instructions (3)
indexer/base/name.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/base/name_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"indexer/base/listener.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
|
Going to go ahead and merge this and move forward with splitting out to |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- indexer/base/kind.go (4 hunks)
- indexer/base/kind_test.go (1 hunks)
- indexer/base/object_type.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- indexer/base/kind.go
- indexer/base/kind_test.go
- indexer/base/object_type.go
Description
Ref: #20352
This PR adds schema and value validation logic to
Kind,Field,ObjectType, andModuleSchematypes incosmossdk.io/indexer/base. It also reduces the valid set of go types that conform toKinds for simplifying future implementation and testing details.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