refactor: Quote SQL identifiers in tests via pq.QuoteIdentifier#2130
Merged
refactor: Quote SQL identifiers in tests via pq.QuoteIdentifier#2130
Conversation
The codebase audit for SQL string concatenation turned up 10 files (all tests and one CLI helper) that built SQL by concatenating unquoted identifiers (schema names, tenant loop variables, table names, constraint names). None were exploitable in practice - the values came from hardcoded test constants or known string slices - but the pattern is wrong, inconsistent with production code (which uses pq.QuoteIdentifier uniformly), and seeds bad hygiene if copied into non-test code. Wrap every dynamic identifier in pq.QuoteIdentifier and use fmt.Sprintf to build the statement. Where the same schemaName is used multiple times in one test, cache the quoted form as a local variable to keep call sites compact. Files touched: - services/reference-data/saga_migrations_test.go - services/reference-data/migrations_test.go - services/reference-data/saga/seeder_test.go - services/reference-data/saga/grpc_handler_test.go - services/reference-data/saga/override_api_test.go - services/reference-data/saga/e2e_seeder_test.go - services/market-information/adapters/persistence/testhelpers/testcontainer.go - services/operational-gateway/e2e/e2e_test.go - shared/platform/testdb/integration_test.go - cmd/position-tool/internal/exporter/exporter.go (hardcoded cursor name quoted uniformly with the rest of the codebase)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSQL identifier values (cursor, schema, table, constraint names) are now passed through Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Claude Code ReviewCommit: SummaryClean, well-scoped refactoring that replaces all instances of raw SQL string concatenation with Second commit addresses CodeRabbit feedback by quoting a few additional concatenation sites missed in the initial grep — same mechanical pattern throughout. All changes are correct and consistent. The quoted identifier is computed once and reused within each scope. The PR description's rationale (copy-paste risk, future fuzzing, reserved-word safety) is sound. The explicit skip of Risk Assessment
FindingsNo issues found. The changes are correct and consistent across both commits. Bot Review NotesNo unresolved bot threads at time of review. Previously FlaggedNone — first review found no issues. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/reference-data/migrations_test.go (1)
549-561:⚠️ Potential issue | 🟡 MinorIdentifier quoting here is correct, but later statements still use concatenation.
Line 600 and Line 611 still do
SET search_path TO+tenant. Please switch those to quoted identifier formatting too so the test is fully consistent with this PR’s safety pattern.Suggested follow-up
- _, err := tc.pool.Exec(ctx, `SET search_path TO `+tenant) + _, err := tc.pool.Exec(ctx, fmt.Sprintf("SET search_path TO %s", pq.QuoteIdentifier(tenant))) ... - _, err := tc.pool.Exec(ctx, `SET search_path TO `+tenant) + _, err := tc.pool.Exec(ctx, fmt.Sprintf("SET search_path TO %s", pq.QuoteIdentifier(tenant)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/reference-data/migrations_test.go` around lines 549 - 561, The test currently constructs SET search_path using string concatenation with the raw tenant variable; replace those concatenations with the already-constructed quoted identifier (quoted) and use the same pattern as the CREATE SCHEMA and earlier SET: call tc.pool.Exec with fmt.Sprintf("SET search_path TO %s", quoted) wherever SET search_path is currently built via concatenation so all executions (the ones that use tc.pool.Exec and reference tenant) use pq.QuoteIdentifier-consistent quoting (refer to variables quoted and tc.pool.Exec in this diff).
🧹 Nitpick comments (2)
cmd/position-tool/internal/exporter/exporter.go (1)
569-570: Consider aligning identifier quoting approach.This line uses
pgx.Identifier{}.Sanitize()while the new cursor code usespq.QuoteIdentifier(). Both produce equivalent double-quoted identifiers, but using the same function throughout the file would be more consistent.♻️ Optional: use pq.QuoteIdentifier here too
- schemaName := pgx.Identifier{tenantID.SchemaName()}.Sanitize() + schemaName := pq.QuoteIdentifier(tenantID.SchemaName())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/position-tool/internal/exporter/exporter.go` around lines 569 - 570, The code currently builds schemaName using pgx.Identifier{tenantID.SchemaName()}.Sanitize() and then injects it into query; align this with the rest of the file by using pq.QuoteIdentifier instead (the cursor code already uses pq.QuoteIdentifier). Replace the construction of schemaName and its use in the query with a quoted identifier produced by pq.QuoteIdentifier(tenantID.SchemaName()) so the file consistently uses the same quoting routine when setting search_path.services/reference-data/saga/seeder_test.go (1)
231-257:setupTenantSchemaForSeedercan drift from real migrations.Line 229 says this mirrors the full migrated schema, but this DDL omits several migration-level constraints (e.g., status/script/preconditions checks). That can let tests pass against states production schema would reject.
Refactor direction
- // Create saga_definition table matching the full migrated schema + // Create saga_definition table for tests. + // Prefer applying the same migration SQL files used by migration tests + // (or include the same CHECK constraints here) to avoid schema drift.If you keep inline DDL, add the key checks to stay aligned:
CONSTRAINT uq_saga_definition_name_version UNIQUE (name, version) + ,CONSTRAINT chk_saga_definition_status + CHECK (status IN ('DRAFT', 'ACTIVE', 'DEPRECATED')) + ,CONSTRAINT chk_saga_definition_script_length + CHECK (script IS NULL OR length(script) <= 65536) + ,CONSTRAINT chk_saga_definition_preconditions_length + CHECK (preconditions_expression IS NULL OR length(preconditions_expression) <= 4096)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/reference-data/saga/seeder_test.go` around lines 231 - 257, The inline DDL in setupTenantSchemaForSeeder (createTableSQL for saga_definition) omits important production migration-level constraints, causing test schema drift; either invoke the real migrations in the test setup (run the same migration runner used in production) or augment createTableSQL to mirror those checks: add the same CHECK constraint on status (allowed enum values), the CHECK/NOT NULL rules applied to script and preconditions_expression (and any dependent checks tying script presence to status/version), and any other migration-added constraints/indices present on saga_definition so tests validate the same schema invariants as production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/reference-data/saga_migrations_test.go`:
- Around line 559-560: The test still builds "SET search_path TO " via unsafe
string concatenation with tenant in later Exec calls; update those calls in
saga_migrations_test.go to use the already-constructed quoted identifier (or
fmt.Sprintf with quoted) instead of "+ tenant". Specifically, replace
occurrences where tc.pool.Exec(ctx, "SET search_path TO "+tenant) (or similar)
with tc.pool.Exec(ctx, fmt.Sprintf("SET search_path TO %s", quoted)) so the
schema name is properly quoted and not concatenated unsafely.
In `@services/reference-data/saga/grpc_handler_test.go`:
- Around line 49-50: Several SQL statements in this test still concatenate
schemaName directly; replace those concatenations by quoting the identifier (as
done earlier) using quoted := pq.QuoteIdentifier(schemaName) and inject it with
fmt.Sprintf instead of string concatenation. Locate the SQL usages that build
queries with "... "+schemaName+" ..." (the Exec/Query calls constructing
schema-qualified names) and change them to use the quoted variable (e.g.,
fmt.Sprintf("... %s ...", quoted)) so all schema identifier insertions use
pq.QuoteIdentifier rather than raw concatenation.
---
Outside diff comments:
In `@services/reference-data/migrations_test.go`:
- Around line 549-561: The test currently constructs SET search_path using
string concatenation with the raw tenant variable; replace those concatenations
with the already-constructed quoted identifier (quoted) and use the same pattern
as the CREATE SCHEMA and earlier SET: call tc.pool.Exec with fmt.Sprintf("SET
search_path TO %s", quoted) wherever SET search_path is currently built via
concatenation so all executions (the ones that use tc.pool.Exec and reference
tenant) use pq.QuoteIdentifier-consistent quoting (refer to variables quoted and
tc.pool.Exec in this diff).
---
Nitpick comments:
In `@cmd/position-tool/internal/exporter/exporter.go`:
- Around line 569-570: The code currently builds schemaName using
pgx.Identifier{tenantID.SchemaName()}.Sanitize() and then injects it into query;
align this with the rest of the file by using pq.QuoteIdentifier instead (the
cursor code already uses pq.QuoteIdentifier). Replace the construction of
schemaName and its use in the query with a quoted identifier produced by
pq.QuoteIdentifier(tenantID.SchemaName()) so the file consistently uses the same
quoting routine when setting search_path.
In `@services/reference-data/saga/seeder_test.go`:
- Around line 231-257: The inline DDL in setupTenantSchemaForSeeder
(createTableSQL for saga_definition) omits important production migration-level
constraints, causing test schema drift; either invoke the real migrations in the
test setup (run the same migration runner used in production) or augment
createTableSQL to mirror those checks: add the same CHECK constraint on status
(allowed enum values), the CHECK/NOT NULL rules applied to script and
preconditions_expression (and any dependent checks tying script presence to
status/version), and any other migration-added constraints/indices present on
saga_definition so tests validate the same schema invariants as production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a86962b9-cf2c-4ba8-bc3c-8331a254d51a
📒 Files selected for processing (10)
cmd/position-tool/internal/exporter/exporter.goservices/market-information/adapters/persistence/testhelpers/testcontainer.goservices/operational-gateway/e2e/e2e_test.goservices/reference-data/migrations_test.goservices/reference-data/saga/e2e_seeder_test.goservices/reference-data/saga/grpc_handler_test.goservices/reference-data/saga/override_api_test.goservices/reference-data/saga/seeder_test.goservices/reference-data/saga_migrations_test.goshared/platform/testdb/integration_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The initial grep missed concat sites nested deeper in the same test files and one in a sibling test file that was not in the original audit list. CodeRabbit flagged: - saga_migrations_test.go:600,611 - SET search_path inside nested t.Run for the "same name+version" subtest - migrations_test.go:600,611 - same pattern for instrument_definition - saga/grpc_handler_test.go:582,618,1081 - INSERT/SELECT INTO "+schemaName+".saga_definition across three subtests - operational-gateway/adapters/persistence/setup_test.go:195 - DELETE FROM table loop (same shape as the e2e_test.go version already fixed; missed because it lives in a sibling package) Apply the same pq.QuoteIdentifier + fmt.Sprintf pattern as the rest of the PR so no raw concat remains in any touched file. Verified via `grep -rnE '\`\s*\+\s*(tenant|schemaName|tbl|masterSchema|constraintName|cursorName)' **/*.go` which now returns only migration_runner.go:394 (the processMigrationSQL rewriter doing legitimate string manipulation, not SQL execution).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
"CREATE SCHEMA " + schemaName,"SELECT ... " + schemaName + ".table", etc.) withfmt.Sprintf+pq.QuoteIdentifier.Why this matters
Production repositories already use
pq.QuoteIdentifieruniformly for dynamic identifiers (schema names from validated tenant IDs, etc). The tests drifted from that convention and were using raw+concatenation. Three risks:Using
pq.QuoteIdentifieris the same mechanism the production code uses, so there is one convention across the whole repo.Files touched
Tests (9):
Non-test (1):
Skipped from audit
shared/platform/db/pool.go:76SET statement_timeout = '%dms'with an integer. Not a security risk (numeric value, not identifier) and PostgreSQL's SET does not accept parameterized values cleanly. Left as-is.Test plan
go build ./...cleango vet ./...cleango test -short ./services/reference-data/saga/... ./shared/platform/testdb/... ./cmd/position-tool/internal/exporter/...all pass