feat(indexer/postgres): add insert/update/delete functionality #21186
Conversation
|
@aaronc your pull request is missing a changelog! |
aaronc
assigned kocubinski, facundomedica, tac0turtle, julienrbrt, cool-develope and testinginprod
![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 (4)
indexer/postgres/listener.go (1)
28-55: Ensure comprehensive error handling and logging.The
OnObjectUpdatemethod effectively handles object updates, but consider enhancing error handling and logging for better traceability. Ensure that errors are logged with sufficient context to aid in debugging.Additionally, consider wrapping the entire update process in a transaction to ensure atomicity, especially if partial updates could lead to inconsistent states.
if err != nil { - return err + if i.logger != nil { + i.logger.Error("Failed to update object", "module", module, "type", update.TypeName, "key", update.Key, "error", err) + } + return fmt.Errorf("failed to update object in module %s: %w", module, err) }indexer/postgres/delete.go (2)
11-28: Verify SQL injection protection and enhance logging.The
deletemethod constructs SQL statements dynamically. Ensure that all inputs are properly sanitized to prevent SQL injection. The use of parameterized queries helps mitigate this risk, but double-check the implementation.Consider enhancing logging to include more context about the operation being performed, especially in error scenarios.
if err != nil { - return err + tm.options.logger.Error("Failed to execute delete", "sql", sqlStr, "params", params, "error", err) + return fmt.Errorf("failed to execute delete: %w", err) }
46-61: Review retain deletion logic and parameter handling.The
retainDeleteSqlAndParamsmethod generates an UPDATE statement for retaining deletions. Ensure that the logic aligns with the intended functionality and that parameters are safely handled to prevent SQL injection.if err != nil { - return nil, err + tm.options.logger.Error("Failed to generate retain delete SQL", "error", err) + return nil, fmt.Errorf("failed to generate retain delete SQL: %w", err) }indexer/postgres/indexer.go (1)
Line range hint
75-89: Verify integration ofaddressCodecand ensure proper initialization.The addition of the
addressCodecfield in theoptsstruct enhances configurability. Ensure that this field is correctly utilized in the indexer and that its integration does not introduce any unintended side effects.Consider adding comments to document the purpose and usage of the
addressCodecfield for future maintainability.opts := options{ disableRetainDeletions: config.DisableRetainDeletions, logger: params.Logger, + // addressCodec is used for encoding addresses in the indexer. addressCodec: params.AddressCodec, }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
indexer/postgres/tests/go.sumis excluded by!**/*.sum
Files selected for processing (11)
- indexer/postgres/delete.go (1 hunks)
- indexer/postgres/indexer.go (2 hunks)
- indexer/postgres/insert_update.go (1 hunks)
- indexer/postgres/listener.go (1 hunks)
- indexer/postgres/options.go (2 hunks)
- indexer/postgres/params.go (1 hunks)
- indexer/postgres/select.go (1 hunks)
- indexer/postgres/tests/go.mod (3 hunks)
- indexer/postgres/tests/postgres_test.go (1 hunks)
- indexer/postgres/view.go (1 hunks)
- indexer/postgres/where.go (1 hunks)
Additional context used
Path-based instructions (10)
indexer/postgres/options.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/where.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/listener.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/delete.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/indexer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/tests/postgres_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/postgres/insert_update.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/view.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.indexer/postgres/select.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (32)
indexer/postgres/options.go (2)
3-5: Imports are well-organized.The import statements are correctly updated to include the new
addressutilpackage.
16-17: EnsureaddressCodecis initialized properly.The
addressCodecfield is expected to be non-nil. Ensure that it is properly initialized in all instances of theoptionsstruct.indexer/postgres/tests/go.mod (3)
8-14: New dependencies added for testing.The dependencies
cosmossdk.io/schema/testingandpgregory.net/rapidare added, which enhance testing capabilities.
Line range hint
18-28: Indirect dependencies are correctly listed.The indirect dependencies
github.com/cockroachdb/apd/v3andgithub.com/tidwall/btreeare correctly listed, indicating they are used by other dependencies.
40-41: Replace directives point to local paths.The replace directives are correctly set to local paths, which is typical for local development and testing.
indexer/postgres/where.go (2)
8-18: FunctionwhereSqlAndParamsis well-structured.The function efficiently generates a WHERE clause and handles errors consistently. Ensure that
bindKeyParamsis correctly implemented to support this function.
20-59: FunctionwhereSqlefficiently constructs SQL clauses.The function uses
io.Writerto construct SQL strings, which is efficient. Error handling is consistent throughout the function.indexer/postgres/delete.go (1)
30-44: Ensure SQL statement correctness and parameter safety.The
deleteSqlAndParamsmethod generates a DELETE statement. Ensure that the table name and parameters are correctly handled to prevent SQL injection. The use offmt.Fprintfshould be carefully reviewed to avoid any formatting issues.indexer/postgres/tests/postgres_test.go (2)
22-28: Good test structure with sub-tests.The use of
t.Runfor organizing sub-tests is appropriate and enhances the readability and maintainability of the tests.
31-98: Comprehensive test function with setup and cleanup.The test function is well-structured, covering setup, execution, and cleanup phases. The use of
require.NoErrorensures that errors are caught immediately.indexer/postgres/insert_update.go (3)
11-39: Efficient insert/update logic.The function efficiently handles insert/update logic and logs SQL statements if a logger is present.
However, address the existing issue with unused variables as highlighted in previous comments.
41-71: Correct SQL INSERT statement generation.The function correctly constructs the SQL INSERT statement and handles parameter binding. Error handling is appropriate.
73-113: Correct SQL UPDATE statement generation.The function correctly constructs the SQL UPDATE statement and handles parameter binding. Error handling is appropriate.
indexer/postgres/view.go (5)
14-16: Correct application state return.The function correctly returns the application state.
18-25: Correct block number retrieval.The function correctly queries the database for the maximum block number and handles errors appropriately.
33-43: Correct module retrieval.The function correctly retrieves a module by name and returns its view.
45-54: Correct module iteration with callback.The function correctly iterates over modules and applies the callback function.
However, address the existing issue with potential non-determinism due to map iteration as highlighted in previous comments.
81-90: Correct object collection iteration with callback.The function correctly iterates over object collections and applies the callback function.
However, address the existing issue with potential non-determinism due to map iteration as highlighted in previous comments.
indexer/postgres/params.go (4)
10-26: LGTM!The
bindKeyParamsfunction correctly handles key parameter binding with appropriate error handling for type mismatches.
28-63: LGTM!The
bindValueParamsfunction effectively manages value parameter binding with comprehensive error handling for unknown columns and type mismatches.
65-87: LGTM!The
bindParamsfunction efficiently binds parameters to fields with robust error handling for missing values.
89-116: LGTM!The
bindParamfunction handles individual parameter binding with comprehensive error handling for nullability and type mismatches.indexer/postgres/select.go (10)
16-26: LGTM!The
countfunction correctly constructs and executes a SQL count query with optional logging.
28-37: LGTM!The
existsfunction effectively checks for row existence using helper methods for SQL generation.
39-54: LGTM!The
checkExistsfunction includes robust error handling for different query outcomes.
56-70: LGTM!The
existsSqlAndParamsfunction correctly generates a SQL SELECT statement with appropriate error handling.
72-86: LGTM!The
getfunction effectively retrieves a row using helper methods for SQL generation and result reading.
88-96: LGTM!The
selectAllSqlfunction correctly generates a SQL SELECT statement, considering configuration options for deletion flags.
98-116: LGTM!The
getSqlAndParamsfunction correctly generates a SQL SELECT statement and binds parameters with appropriate error handling.
118-147: LGTM!The
selectAllClausefunction correctly constructs a SQL SELECT clause, ensuring all relevant fields are included.
149-214: LGTM!The
readRowfunction includes comprehensive handling for different field types and optional deletion flags.
217-298: LGTM!The
colBindValueandreadColfunctions correctly handle different field types, ensuring proper data conversion and error handling.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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)
- indexer/postgres/insert_update.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/postgres/insert_update.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- indexer/postgres/select.go (1 hunks)
- indexer/postgres/where.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/postgres/select.go
- indexer/postgres/where.go
…aronc/indexer-postgres2
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)
- indexer/postgres/tests/go.mod (3 hunks)
Files skipped from review due to trivial changes (1)
- indexer/postgres/tests/go.mod
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
indexer/postgres/tests/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- indexer/postgres/tests/go.mod (3 hunks)
Files skipped from review due to trivial changes (1)
- indexer/postgres/tests/go.mod
Description
This PR adds the postgres insert/update/delete functionality to the postgres indexer. It is tested using generated data from
cosmossdk.io/schema/testing. This is essentially the "core" postgres state indexer implementation.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
OnObjectUpdatecallback for handling updates to objects.Bug Fixes
Tests