CAMEL-24132: camel-jpa - Fix medium-severity findings from component review#24794
Conversation
|
🌟 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.
Review: CAMEL-24132 — Fix medium-severity findings from component review
Verdict: Approve ✅
This PR addresses 5 of 6 medium-severity findings from the JPA component review. Each fix is well-targeted:
M1 — receiveNoWait() refactored (JpaPollingConsumer)
Clean refactoring into doReceive(boolean useLocking). receiveNoWait() now correctly skips pessimistic locking so it returns immediately rather than blocking on row locks.
M2 — future.cancel(true) on timeout (JpaPollingConsumer)
Previously, receive(timeout) caught TimeoutException but left the future running — meaning the submitted Callable could continue holding DB locks indefinitely. The future.cancel(true) fix correctly interrupts the abandoned task.
M3 — Skip setLockMode() for native queries (JpaPollingConsumer)
The JPA spec throws IllegalStateException when setLockMode() is called on a native query. The guard nativeQuery == null correctly prevents this, and the nativeQuery field is already defined in the class (line 53). This is consistent with the native-query guards added in PR #24793 (createDeleteHandler) and the existing lockEntity() method.
M4 — Constraint violation race handling (JpaMessageIdRepository)
Two good changes:
- Catching
SQLIntegrityConstraintViolationException/EntityExistsExceptionand returningfalse(duplicate) instead of throwing is correct for concurrent insert races. Walking the cause chain handles JPA provider wrapping. - Removing the premature
entityManager.close()inside thetryblock (afterflush()) is correct — thefinallyblock already handles cleanup.
M6 — isUpdateQuery() whitespace & keyword detection (JpaProducer)
Fixes leading-whitespace queries that were misclassified. The logic change from "not-SELECT → update" to "INSERT/UPDATE/DELETE → update" is more explicit. This does change behavior for CTE-based DML (WITH ... UPDATE ...) and MERGE/CALL statements, which would now default to non-update, but users can set useExecuteUpdate explicitly for those edge cases, and CTE selects (WITH ... SELECT ...) — the more common case — are now correctly handled.
Minor observations (non-blocking):
-
FQCNs in
isConstraintViolation():java.sql.SQLIntegrityConstraintViolationExceptionandjakarta.persistence.EntityExistsExceptionare used as fully qualified names rather than imported. Per project conventions, these should be imported. The build's OpenRewrite rule should auto-fix this, but the formatter/impsort might not add the imports automatically. -
Premature
entityManager.close()inremove()andclear(): The premature close-after-flush was correctly removed fromadd(), but the same pattern still exists inremove()(line 206) andclear()(line 256). These are benign (thefinallyguard prevents double-close) but inconsistent.
Reviewed with Claude Code (claude-code/1.0.3) on behalf of gnodet. 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: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
…review - M1: receiveNoWait() no longer delegates to receive() with pessimistic locking - M2: receive(timeout) cancels the background future on timeout - M3: setLockMode() is skipped for native queries per JPA spec - M4: Concurrent insert race in JpaMessageIdRepository maps constraint violation to duplicate - M6: isUseExecuteUpdate() trims query and detects update keywords instead of select Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
b713b67 to
d31c4c8
Compare
2 participants
Summary
Fixes 5 of 6 medium-severity findings from the July 2026 camel-jpa component review (CAMEL-24132):
receiveNoWait()no longer delegates toreceive()with pessimistic locking — refactored intodoReceive(boolean useLocking)soreceiveNoWait()runs without locksreceive(timeout)now callsfuture.cancel(true)onTimeoutExceptionso abandoned tasks don't accumulate holding DB lockssetLockMode()is skipped for native queries, which throwIllegalStateExceptionper JPA specJpaMessageIdRepository.add()now catches constraint violations and maps them toreturn false(duplicate detected) instead of failing the exchange. Also removed prematureentityManager.close()inside the transaction lambdaisUseExecuteUpdate()now trims query strings and detects update keywords (INSERT/UPDATE/DELETE) instead of select, so queries with leading whitespace or CTEs are correctly classifiedM5 (parallel EntityManager fix missing from Multicast/RecipientList/WireTap) is tracked separately in CAMEL-24174.
Test plan
nativeQuery+ defaultconsumeLockEntity=trueClaude Code on behalf of davsclaus
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com