wallet address query split #249
Conversation
WalkthroughThe codebase introduces a modular query builder for ClickHouse, centralizing SQL construction and adding specialized UNION query logic for wallet address filtering in the "transactions" table. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ClickHouseConnector
participant QueryBuilder
Caller->>ClickHouseConnector: GetAggregations(table, qf)
ClickHouseConnector->>QueryBuilder: buildQuery(table, columns, qf)
alt table == "transactions" and wallet address filter present
QueryBuilder->>QueryBuilder: buildUnionQuery(table, columns, qf)
QueryBuilder->>QueryBuilder: addPostQueryClauses(unionQuery, qf)
else
QueryBuilder->>QueryBuilder: buildStandardQuery(table, columns, qf)
QueryBuilder->>QueryBuilder: addPostQueryClauses(standardQuery, qf)
end
ClickHouseConnector->>ClickHouseConnector: Execute built query
ClickHouseConnector-->>Caller: Return QueryResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)internal/storage/clickhouse.go (1)
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
internal/storage/clickhouse.go (2)
555-569: Consider validating wallet address format.The method checks if
qf.WalletAddress != ""but doesn't validate if it's a valid Ethereum address format. Consider adding validation to prevent potential SQL injection or invalid queries.func (c *ClickHouseConnector) buildQuery(table, columns string, qf QueryFilter) string { var query string // Check if we need to handle wallet address with UNION for transactions - if table == "transactions" && qf.WalletAddress != "" { + if table == "transactions" && qf.WalletAddress != "" && ethereum.IsHexAddress(qf.WalletAddress) { query = c.buildUnionQuery(table, columns, qf) } else { query = c.buildStandardQuery(table, columns, qf) }
2092-2095: Consider alternative testing approach.Exposing internal methods for testing can lead to maintenance issues. Consider testing through the public API or using a test-specific interface.
Instead of exposing internal methods, you could:
- Test through the public
GetAggregationsorGetTransactionsmethods- Create a separate test helper that doesn't pollute the production code
- Use dependency injection to make the query builder testable
If you must keep this approach, consider adding a build tag to exclude it from production builds:
// +build test // TestQueryGeneration is only available in test builds func (c *ClickHouseConnector) TestQueryGeneration(table, columns string, qf QueryFilter) string { return c.buildQuery(table, columns, qf) }internal/storage/clickhouse_connector_test.go (1)
111-249: Comprehensive test coverage for UNION query logic.Good test coverage for the new query building functionality. Consider these improvements:
- The hardcoded connection details might fail in different environments
- String-based assertions are fragile - consider parsing the SQL or using a SQL parser library
- Add test cases for edge cases like empty wallet address or special characters
Consider adding these test cases:
// Test case 5: Empty wallet address should use standard query t.Run("Empty wallet address", func(t *testing.T) { qf := QueryFilter{ ChainId: big.NewInt(8453), WalletAddress: "", Limit: 5, } query := connector.TestQueryGeneration("transactions", "*", qf) if strings.Contains(query, "UNION ALL") { t.Errorf("Empty wallet address should not trigger UNION query: %s", query) } }) // Test case 6: Special characters in wallet address (to verify escaping) t.Run("Wallet address with special characters", func(t *testing.T) { qf := QueryFilter{ ChainId: big.NewInt(8453), WalletAddress: "0x'; DROP TABLE transactions; --", Limit: 5, } query := connector.TestQueryGeneration("transactions", "*", qf) // Verify the address is properly handled (though this would still be vulnerable with current implementation) if strings.Contains(query, "DROP TABLE") { t.Errorf("SQL injection attempt should be escaped: %s", query) } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/storage/clickhouse.go(3 hunks)internal/storage/clickhouse_connector_test.go(5 hunks)
🔇 Additional comments (4)
internal/storage/clickhouse.go (3)
469-526: Clean refactoring of query construction logic.The delegation to
buildQueryimproves code organization and maintainability by centralizing the SQL construction logic.
655-696: Well-structured WHERE clause building.Good separation of concerns by extracting WHERE clause logic and correctly handling the special case for wallet address in transactions table.
621-653: Verify GROUP BY behavior with UNION ALL subqueriesWhen wrapping a UNION ALL query in a subquery, the current code does:
SELECT * FROM (<union-query>) GROUP BY …Using
SELECT *can break if the union’s columns don’t exactly match those listed in the GROUP BY (or aren’t aggregated), leading to SQL errors. Please:
- Manually verify any existing UNION ALL + GROUP BY use cases to ensure the returned columns align.
- Add or update tests covering UNION ALL queries with GROUP BY to catch mismatches.
- Consider replacing
SELECT *with an explicit column list (only grouping keys and aggregated columns) or validating the column set before wrapping.internal/storage/clickhouse_connector_test.go (1)
14-109: Test updates correctly reflect the type mapping changes.The updated test cases properly validate the pointer-based type mapping, including double pointers for nullable types.
Summary by CodeRabbit