CAMEL-24044: camel-sql - Fix Postgres aggregation repositories binding version without a placeholder#24655
Conversation
…g version without a placeholder
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Correct fix for a long-standing bug where the Postgres aggregation repositories' INSERT statements were missing the version column (and the clustered variant also missed instance_id), causing parameter-count mismatches on every add() call.
Scanner coverage: No static analysis tools (PMD, Checkstyle, semgrep) available. Rely on CI.
Key observations:
- The fix correctly aligns the Postgres subclasses' INSERT column lists with the base
insertHelper()binding order (exchange blob, key, version, and optionally instance_id). - Passing
versioninstead of the hardcoded1Lis correct: the caller provides1Lfor new inserts and the actual version fromget()for completed-table inserts. - The
ClusteredPostgresAggregationRepositoryfix forinstance_idis also correct:insertHelper()conditionally bindsinstanceIdwhencompleted && recoveryByInstance, so the SQL must include it under the same condition. - No impact on other database dialects — fix is confined to Postgres subclasses.
- No SQL injection risks — all values are parameterized via
PreparedStatement. - Integration tests are thorough, covering full lifecycle (add/get/update/remove/scan/recover/confirm),
storeBodyAsTextmode, and multi-node instance-scoped recovery against real PostgreSQL. - Documentation and upgrade guide updates are appropriate.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code review on behalf of @gnodet
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Reviewed with focus on the parameter/placeholder arithmetic, both repository flavors, and the recent deserialization-hardening lineage in these classes — the fix is correct and I have no change requests. Summary of the verification:
Root cause confirmed against main. JdbcAggregationRepository.insertHelper (JdbcAggregationRepository.java:336-340) always binds three base parameters (exchange blob → 1, id → 2, version → 3) before the optional body/header ones, while the Postgres subclasses' SQL builders emit only two base columns (EXCHANGE, ID) — one more bound parameter than ? placeholders in every configuration, so add() could never succeed on Postgres. The new column order (EXCHANGE, ID, VERSION, [BODY], [headers...], [instance_id]) exactly matches the binder's positional order in both the plain and clustered insertHelper, and passing the real version (instead of a hardcoded value) matches base-class semantics for the completed-table insert path. The clustered variant's added instance_id column also makes scan()'s WHERE INSTANCE_ID = ? able to match rows for the first time. The regression traces to the Camel 3.4-era CAMEL-7810 rework, which introduced the version binding without adding the placeholder — nothing intentional is being reverted.
Since the broken path could never succeed, there is no behavioral-compat risk — and the documented default DDL already includes version BIGINT NOT NULL, so users following the docs need no schema change. The stale SQL example in sql-component.adoc is corrected too (the old line had the wrong column order and count).
Tests: the two new real-PostgreSQL ITs (plain + clustered, covering add/get/update/remove/scan/recover/confirm, storeBodyAsText, and two-node instance-scoped recovery) are exactly what this fix needed — the H2-based unit tests structurally couldn't catch a Postgres-only SQL defect. No Thread.sleep; new dependencies are test-scoped only (org.postgresql:postgresql via the existing parent property + project-internal camel-test-infra-postgres), matching the established camel-pg-replication-slot pattern.
Security cross-check: the ObjectInputFilter/deserialization-filter work in these classes is untouched (write path only), and values remain ?-bound — only constant identifiers are concatenated into the SQL.
Minor notes, none blocking:
ClusteredPostgresAggregationRepositoryappends the"instance_id"string literal because the base-classINSTANCE_IDconstant isprivate(ClusteredJdbcAggregationRepository.java:48) — fine as-is; widening the constant toprotectedwould be a reasonable tiny follow-up.- The PR body's
mvn verify -Pitreferences a profile that doesn't exist in this build — failsafe ITs are bound via the always-activefullprofile, so plainmvn verifyruns them; description-only inaccuracy. - The PR body declares "Generated with Claude Code" but the commit lacks the AI co-authorship trailer — per the repo's AI guidelines, worth adding to the squash-merge message.
The two matrix build jobs were still pending at review time — worth confirming green before merge.
Reviewed with Claude Code (Fable 5) on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 12 tested, 28 compile-only — current: 11 all testedMaveniverse Scalpel detected 40 affected modules (current approach: 11).
|
4 participants
JIRA: https://issues.apache.org/jira/browse/CAMEL-24044
Since the CAMEL-7810 optimistic-locking refactor (Camel 3.4),
PostgresAggregationRepository.insert()builtINSERT INTO t(exchange, id, ...) ON CONFLICT DO NOTHINGwithout theversioncolumn while the baseinsertHelperbinds blob, key and version — one more bound parameter than placeholders, so everyadd()failed withThe column index is out of range(verified against real PostgreSQL). WithstoreBodyAsText=truethe version was silently bound into thebodyplaceholder before failing.ClusteredPostgresAggregationRepositoryhad the same defect in its completed-table insert (everyremove()failed at completion) and never wrote theinstance_idcolumn, so instance-scoped recovery could never match any row.Changes:
versioncolumn in both PostgresINSERTstatements and pass the actual version instead of the hardcoded1Linstance_idwhencompleted && recoveryByInstancecamel-test-infra-postgres) covering the full add/get/update/remove/recover/confirm lifecycle,storeBodyAsText, and instance-scoped recovery with two nodes — these fail on currentmainand pass with the fixINSERTexample in the component docs and added an upgrade-guide entry (the aggregation tables need theversion BIGINTcolumn, asJdbcAggregationRepositoryalready required for reads/updates)Run the new ITs with:
mvn verify -Pit -Dit.test='PostgresAggregationRepositoryIT,ClusteredPostgresAggregationRepositoryIT' -Dtest=NoSuchTest -Dsurefire.failIfNoSpecifiedTests=falseClaude Code on behalf of Croway
🤖 Generated with Claude Code