fix(schema)!: fixes and key restrictions based on indexer testing #21362
Conversation
WalkthroughWalkthroughThe recent changes across the schema files enhance the handling of key fields and value comparisons, improve validation logic, and streamline object update mechanisms. A new method for validating key kinds was introduced, along with refined criteria to ensure data integrity. Additionally, updates include improved comments and expanded test cases, contributing to better documentation and increased test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Schema
participant Validator
participant ObjectGenerator
User->>Schema: Request to create object
Schema->>Validator: Validate key kind
Validator-->>Schema: Key kind valid/invalid
Schema->>ObjectGenerator: Generate object with valid keys
ObjectGenerator-->>Schema: Return generated object
Schema-->>User: Provide created object
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 Configuration File (
|
aaronc
changed the title
![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
Outside diff range, codebase verification and nitpick comments (1)
schema/testing/appdatasim/testdata/app_sim_example_schema.txt (1)
100-100: Correct grammatical errors.Use "an" instead of "a" before words starting with a vowel sound.
- "ThreeKeys","Key":["@(\u0001\u0001\tᛰᾚ𐺭a'ᵆᾭaa",16,817744173394],"Value":{"Value1":145},"Delete":false} + "ThreeKeys","Key":["@(\u0001\u0001\tᛰᾚ𐺭an'ᵆᾭaa",16,817744173394],"Value":{"Value1":145},"Delete":false} - "ThreeKeys","Key":["@(\u0001\u0001\tᛰᾚ𐺭a'ᵆᾭaa",16,817744173394],"Value":-1,"Delete":false} + "ThreeKeys","Key":["@(\u0001\u0001\tᛰᾚ𐺭an'ᵆᾭaa",16,817744173394],"Value":-1,"Delete":false}Also applies to: 157-157
Tools
LanguageTool
[misspelling] ~100-~100: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..."ThreeKeys","Key":["@(\u0001\u0001\tᛰᾚ𐺭a'ᵆᾭaa",16,817744173394],"Value":{"Value1...(EN_A_VS_AN)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (12)
- schema/kind.go (3 hunks)
- schema/object_type.go (2 hunks)
- schema/object_type_test.go (1 hunks)
- schema/testing/appdatasim/app_data.go (3 hunks)
- schema/testing/appdatasim/testdata/app_sim_example_schema.txt (1 hunks)
- schema/testing/appdatasim/testdata/diff_example.txt (1 hunks)
- schema/testing/example_schema.go (1 hunks)
- schema/testing/field.go (1 hunks)
- schema/testing/module_schema.go (2 hunks)
- schema/testing/object.go (3 hunks)
- schema/testing/statesim/object_coll.go (5 hunks)
- schema/testing/statesim/object_coll_diff.go (1 hunks)
Additional context used
Path-based instructions (10)
schema/testing/module_schema.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/statesim/object_coll_diff.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/example_schema.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/object_type.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/object.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/field.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/statesim/object_coll.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/appdatasim/app_data.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/object_type_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"schema/kind.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
LanguageTool
schema/testing/appdatasim/testdata/app_sim_example_schema.txt
[misspelling] ~100-~100: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..."ThreeKeys","Key":["@(\u0001\u0001\tᛰᾚ𐺭a'ᵆᾭaa",16,817744173394],"Value":{"Value1...(EN_A_VS_AN)
[misspelling] ~157-~157: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ..."ThreeKeys","Key":["@(\u0001\u0001\tᛰᾚ𐺭a'ᵆᾭaa",16,817744173394],"Value":-1,"Dele...(EN_A_VS_AN)
Additional comments not posted (25)
schema/testing/module_schema.go (2)
46-50: Variable renaming fromschematoschis acceptable.The change in variable name from
schematoschinMustNewModuleSchemaenhances readability without affecting functionality.
32-32: Ensure functionhasDuplicateTypeNamesis correctly implemented and used.The renaming from
hasDuplicateNamestohasDuplicateTypeNamessuggests a focus on type names. Verify that this function accurately checks for duplicate type names in bothKeyFieldsandValueFields.Verification successful
Function
hasDuplicateTypeNamesis correctly implemented and used.The function accurately checks for duplicate type names by iterating over fields and using a map to track seen type names, focusing on fields of
EnumKind. The logic aligns with the intended functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `hasDuplicateTypeNames` function. # Test: Search for the function implementation and usage. Expect: Correct logic for checking duplicate type names. ast-grep --lang go --pattern 'func hasDuplicateTypeNames($_, $_) bool { $$$ }'Length of output: 772
schema/testing/statesim/object_coll_diff.go (1)
38-38: EnsureFormatObjectKeyis correctly implemented and used.The replacement of
fmtObjectKeywithFormatObjectKeysuggests a move to a standardized method. Verify thatFormatObjectKeyis correctly implemented and used for formatting object keys.Verification successful
FormatObjectKey is correctly implemented and used.
The
FormatObjectKeyfunction is implemented with a robust logic to handle different cases of key formatting and is consistently used throughout the codebase for its intended purpose of formatting object keys. No issues found.
- Implementation in
schema/testing/statesim/object_coll.go- Usage in
schema/testing/statesim/object_coll.go,schema/testing/statesim/object_coll_diff.go, andschema/testing/appdatasim/app_data.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `FormatObjectKey` function. # Test: Search for the function implementation and usage. Expect: Correct logic for formatting object keys. ast-grep --lang go --pattern 'func FormatObjectKey($_, $_) string { $$$ }'Length of output: 1532
Script:
#!/bin/bash # Description: Search for all usages of `FormatObjectKey` in the codebase to verify correct usage. rg 'FormatObjectKey' -A 3Length of output: 1919
schema/testing/appdatasim/testdata/diff_example.txt (1)
5-67: Verify intentional changes in expected vs. actual discrepancies.The discrepancies in object collections suggest changes in underlying data handling. Ensure these changes are intentional and documented.
schema/testing/example_schema.go (1)
155-157: New check for valid key kinds enhances robustness.The addition of a check to default invalid key kinds to
Int32KindinmkTestObjectTypeis a positive change. Ensure the logic is correctly implemented.schema/object_type.go (2)
14-16: Clarification on key field constraints is beneficial.The updated comments clarify constraints on key fields, enhancing documentation and understanding.
58-60: New validation logic for key kinds improves robustness.The added validation logic ensures key fields have valid kinds, enhancing error reporting and robustness. Verify the implementation.
schema/testing/object.go (5)
10-12: Good separation of key and value field generation.The introduction of
keyFieldsGenandvalueFieldsGenimproves clarity by distinctly handling key and value fields. This change enhances the code's readability and maintainability.
30-37: Improved duplicate field name checks.The use of
hasDuplicateFieldNamesfor both key and value fields ensures robust validation against duplicate field names, enhancing schema integrity.
50-58: Clarification on duplicate field name detection.The
hasDuplicateFieldNamesfunction effectively checks for duplicate field names, ensuring that schemas are correctly validated for uniqueness.
Line range hint
60-74: Refined duplicate type name checks.The
hasDuplicateTypeNamesfunction now specifically checks for duplicate enum type names, which is crucial for maintaining schema consistency.
104-111: Enhanced key filtering in ObjectUpdateGen.The filtering mechanism prevents updates from using keys already present in the state, which improves the integrity of the object updates.
schema/testing/field.go (1)
38-40: Introduction of KeyFieldGen enhances key field generation.The new
KeyFieldGenensures that only non-nullable fields of valid key kinds are generated, which is essential for schema integrity.schema/testing/statesim/object_coll.go (3)
52-52: Consistent key formatting with FormatObjectKey.The use of
FormatObjectKeyensures consistent key formatting across the codebase, which is crucial for maintaining data integrity.
123-123: Consistent key retrieval with FormatObjectKey.The update to use
FormatObjectKeyinGetObjectmaintains consistency in key handling, which is important for reliable data retrieval.
Line range hint
137-175: Improved key formatting and value normalization.The
FormatObjectKeyfunction provides deterministic key formatting, and thefmtValuefunction enhances value normalization by handling decimal and integer strings more accurately.schema/testing/appdatasim/app_data.go (2)
Line range hint
95-110: Centralized key formatting with formatUpdateKey.The introduction of
formatUpdateKeycentralizes the logic for key generation, improving clarity and reducing potential errors in update handling.
121-134: Robust update key formatting with formatUpdateKey.The
formatUpdateKeyfunction enhances robustness by encapsulating key formatting logic, ensuring consistency and reducing duplication.schema/object_type_test.go (1)
192-230: Enhanced test coverage for invalid key field types.The additional test cases for
float32,float64, andjsonkey fields ensure that these invalid types are correctly flagged, enhancing validation robustness.schema/kind.go (2)
82-83: Clarification on equality comparison is beneficial.The added comments emphasize the importance of using numerical equality over string equality for
IntegerStringKindandDecimalStringKind, which improves clarity.Also applies to: 92-93
376-386: Addition ofValidKeyKindenhances key validation.The new function
ValidKeyKindcorrectly identifies valid key kinds, aligning with the PR's objective to restrict certain types as keys.schema/testing/appdatasim/testdata/app_sim_example_schema.txt (4)
10-16: Schema updates align with PR objectives.The changes to
OnObjectUpdateentries, including new update types and modifications to existing values, align with the PR's objectives to enhance data representation and introduce new types.Also applies to: 22-29, 32-37, 46-50, 57-60, 65-70, 79-82, 88-92, 96-101, 109-113, 115-120, 127-131, 134-140, 146-150
33-33: Removal of outdated updates is appropriate.The removal of certain updates reflects a refinement in the schema, eliminating outdated or irrelevant data types.
Also applies to: 48-48, 53-53, 65-65, 69-69, 81-81, 98-98, 112-112, 122-122, 123-123, 131-131, 137-137
12-12: Structural changes enhance data handling.The structural changes in
OnObjectUpdateentries improve the schema's ability to handle more complex data scenarios.Also applies to: 59-59, 73-73, 89-89, 96-96, 104-104, 113-113, 126-126, 128-128, 138-138, 149-149, 158-158
10-10: Verify changes in module names.Ensure that the changes in module names do not affect processing or referencing in the application.
Also applies to: 57-57, 79-79, 96-96, 115-115, 138-138
Verification successful
Module Names Are Consistently Used
The module names "all_kinds" and "test_cases" are consistently referenced throughout the
app_sim_example_schema.txtfile. There are no discrepancies in their usage that would suggest any issues with processing or referencing within the application.
- All instances of
ModuleNamewith "all_kinds" and "test_cases" appear to be used correctly in context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify changes in module names do not affect processing. # Test: Search for module name usage. Expect: Consistent references. rg --type text 'ModuleName":"all_kinds"' -A 2 rg --type text 'ModuleName":"test_cases"' -A 2Length of output: 154
Script:
#!/bin/bash # Description: Verify changes in module names do not affect processing. # Test: Search for module name usage. Expect: Consistent references. rg 'ModuleName":"all_kinds"' -A 2 rg 'ModuleName":"test_cases"' -A 2Length of output: 61097
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- schema/testing/object.go (4 hunks)
Additional context used
Path-based instructions (1)
schema/testing/object.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (6)
schema/testing/object.go (6)
11-13: LGTM!The
keyFieldsGenvariable correctly generates distinct key fields, ensuring uniqueness by name.
15-16: LGTM!The
valueFieldsGenvariable correctly generates distinct value fields, ensuring uniqueness by name.
24-50: LGTM!The
ObjectTypeGenfunction correctly generates object types and ensures no duplicate field or type names exist.
51-59: LGTM!The
hasDuplicateFieldNamesfunction efficiently checks for duplicate field names using a map.
Line range hint
61-74: LGTM!The
hasDuplicateTypeNamesfunction correctly checks for duplicate type names among fields of kindEnumKind.
85-92: LGTM!The
ObjectUpdateGenfunction correctly filters out keys that already exist in the state, enhancing update integrity.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- schema/testing/appdatasim/app_data.go (4 hunks)
- schema/testing/field.go (2 hunks)
- schema/testing/fmt.go (1 hunks)
- schema/testing/fmt_test.go (1 hunks)
- schema/testing/object.go (3 hunks)
- schema/testing/statesim/object_coll.go (3 hunks)
- schema/testing/statesim/object_coll_diff.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- schema/testing/appdatasim/app_data.go
- schema/testing/field.go
- schema/testing/object.go
- schema/testing/statesim/object_coll_diff.go
Additional context used
Path-based instructions (3)
schema/testing/fmt.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/fmt_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"schema/testing/statesim/object_coll.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (3)
schema/testing/fmt_test.go (1)
9-60: Test cases are comprehensive and well-structured.The test cases provide sufficient coverage for the
ObjectKeyStringfunction and adhere to best practices.schema/testing/statesim/object_coll.go (2)
51-51: Integration ofObjectKeyStringinApplyUpdateis correct.The use of
ObjectKeyStringfor key formatting inApplyUpdateis appropriate and enhances consistency.
122-122: Integration ofObjectKeyStringinGetObjectis correct.The use of
ObjectKeyStringfor key retrieval inGetObjectis appropriate and enhances consistency.
| func ObjectKeyString(objectType schema.ObjectType, key any) string { | ||
| keyFields := objectType.KeyFields | ||
| n := len(keyFields) | ||
| switch n { | ||
| case 0: | ||
| return "" | ||
| case 1: | ||
| valStr := fmtValue(keyFields[0].Kind, key) | ||
| return fmt.Sprintf("%s=%v", keyFields[0].Name, valStr) | ||
| default: | ||
| ks := key.([]interface{}) | ||
| res := "" | ||
| for i := 0; i < n; i++ { | ||
| if i != 0 { | ||
| res += ", " | ||
| } | ||
| valStr := fmtValue(keyFields[i].Kind, ks[i]) | ||
| res += fmt.Sprintf("%s=%v", keyFields[i].Name, valStr) | ||
| } | ||
| return res | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding type checks for key assertions.
In the default case, the function assumes key is a []interface{} without checking. This could lead to a panic if the assumption is incorrect. Consider adding a type check or error handling.
ks, ok := key.([]interface{})
if !ok {
panic("expected key to be of type []interface{}")
}| func fmtValue(kind schema.Kind, value any) string { | ||
| switch kind { | ||
| case schema.BytesKind, schema.AddressKind: | ||
| return fmt.Sprintf("0x%x", value) | ||
| case schema.DecimalStringKind, schema.IntegerStringKind: | ||
| // we need to normalize decimal & integer strings to remove leading & trailing zeros | ||
| d, _, err := apd.NewFromString(value.(string)) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| r := &apd.Decimal{} | ||
| r, _ = r.Reduce(d) | ||
| return r.String() | ||
| default: | ||
| return fmt.Sprintf("%v", value) | ||
| } |
There was a problem hiding this comment.
Replace panic with error handling.
Using panic for error handling is not recommended in production code. Consider returning an error instead.
d, _, err := apd.NewFromString(value.(string))
if err != nil {
return "", err
}Update the function signature to return (string, error) and handle the error in the calling code.
Description
This code was pulled from #21186 after successful testing. I've split it into a separate PR to make that PR smaller.
The most notable changes are:
float32,float64andJSONtypes cannot be used as object key fields because equality is not well definedschematesting.ObjectKeyStringfunctionAuthor 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
Summary by CodeRabbit
New Features
ObjectType, ensuring only valid types are accepted.Improvements
Tests
Documentation