Repository navigation
CAMEL-26147: camel-jpa - a consumer query that selects values delivers them - #28630
vignesh-manel wants to merge 6 commits into
Conversation
…s them With the default consumeLockEntity=true, JpaConsumer.lockEntity locks each query result. For a value that is not an entity, such as a selected column, the lock fails, and the catch returned false (it only accepted arrays), so every row was skipped with only a DEBUG log and the route got no message. A value that is not an entity has no lock, so it is now processed. With that alone, the default consumeDelete=true then called em.remove on the value, which failed and rolled back the poll. The delete handler now removes only entities and logs a WARN for any other result, as it already did for the arrays of native queries. The jpa doc said to use entityType=java.util.List for a List body: it is the entity class name of the endpoint, jpa:java.util.List. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔁 3 tests passed only after a retry on JDK 21 (3 retried attempts) Recovered flaky tests on JDK 21 (3)
🔬 Scalpel shadow comparison — Scalpel: 8 of 708 tested, 25 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (3 direct + 7 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (25)
All tested modules (35 modules, 5m 45s total)Total reactor time: 5m 45s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Verdict: Thanks @vignesh-manel, the diagnosis, the doc fix and the test are spot on. One change is needed so the fix also holds on Hibernate.
Blockers
JpaConsumer.java:380-395:lockEntitystill callsentityManager.lockon a value that is not an entity. On Hibernate this dooms the transaction, so the values are delivered and then the poll fails at commit.
Other findings: 2 non-blocking.
Blocking: check isEntity before the lock
The new test passes on OpenJPA, which our CI uses, but Hibernate behaves differently when lock fails. Reading the Hibernate 7.4.13 bytecode (javap; I did not run it): locking an object with no persistence-context entry, such as a String, throws DetachedObjectException in DefaultLockEventListener.onLock. Under JPA bootstrap (Spring/Spring Boot), ExceptionConverterImpl then calls markForRollbackOnly() and rethrows an IllegalArgumentException. lockEntity catches it and the route still gets the value, but at the end of the poll the TransactionTemplate commit finds the transaction rollback-only and throws UnexpectedRollbackException, so poll() fails on every run. The native-query Object[] path (CAMEL-24131) has the same problem.
Since there is nothing to lock, we can skip the call:
if (!getEndpoint().isConsumeLockEntity()) {
return true;
}
if (!isEntity(entity, entityManager)) {
// a value that is not an entity (a selected column, or the Object array of a native query) has no lock;
// a failed lock marks the transaction rollback-only on some providers (Hibernate)
return true;
}mvn test -Dhibernate -Dtest=JpaConsumerScalarResultTest in components/camel-jpa runs the new test with Hibernate. I expect it to fail on the current head and pass with this change, but that is from reading the code, not from running it.
Non-blocking
- [Suggestion] With the default
consumeDelete=true, a scalar query logs the "Cannot auto-delete" WARN for every row on every poll. The upgrade guide covers it; logging once per consumer would keep the log readable. - [Note]
isEntitynow also gates the lock. The existing consumeDelete tests passing on OpenJPA is good evidence it recognises enhanced entities.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…t an entity lockEntity now checks isEntity before calling entityManager.lock, so a selected column or the Object array of a native query is processed without a lock attempt. Before, the lock was attempted and its failure caught, but on Hibernate a failed lock can mark the transaction rollback only. A failed lock of an entity still skips it. The WARN that a query result that is not an entity cannot be deleted is logged once per consumer (then DEBUG), as each poll delivers the same results again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@davsclaus thanks for the review. Both points are done in fa54e0f. Blocker, I also ran it on Hibernate, which my first round missed:
WARN per row: done. The "Cannot auto-delete" WARN is logged once per consumer and then at DEBUG. In the test (two polls of two rows) it appears once in the log, and the upgrade-guide entry now says so. On fa54e0f, the full camel-jpa suite passes after I'll re-request your review once CI is green on fa54e0f. Claude Code on behalf of vignesh-manel |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
TL;DR
Verdict: LGTM — all three findings from the previous review are addressed. The fix is correct and complete.
Blocking resolved: isEntity check before lockEntity
The new isEntity guard is in place (JpaConsumer.java:376-380), and it short-circuits before any entityManager.lock(...) call. The isEntity implementation iterates entityManager.getMetamodel().getEntities() using type.getJavaType().isInstance(result), which correctly handles Hibernate-proxied subclasses. The catch-block now returns false (no longer entity.getClass().isArray()) — arrays are now handled by the early guard, so that path is dead code no more.
Non-blocking resolved: once-per-consumer WARN logging
AtomicBoolean notEntityWarned with compareAndSet(false, true) is the standard thread-safe once-only gate. First non-entity poll logs WARN; subsequent polls log DEBUG. Clean.
Delete logic
isEntity(entityBean, em) replaces !entityBean.getClass().isArray() — consistent with the lock guard.
Test coverage
JpaConsumerScalarResultTest covers both the delivery path (values reach mock:result, rows are not deleted, re-polled) and the lock path (mock EntityManager verifies lock() is never called for non-entities, and is called for a real entity). Solid.
Suggestion: Consider backporting to camel-4.22.x — UnexpectedRollbackException on every poll is a regression-class bug for users on Hibernate with scalar queries, and 4.22.x is the current LTS.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
Verdict: Comment: the blocker of the earlier review is fixed at this head and the new tests pin it well; what remains is documentation precision, plus a rebase for a conflict in the upgrade guide.
Blockers
- None. The blocker of the earlier CHANGES_REQUESTED review is addressed at
JpaConsumer.java:379-383.
Other findings: 7 non-blocking (1 important, 3 suggestions, 1 question, 2 nits), plus a rebase (camel-4x-upgrade-guide-4_24.adoc conflicts with main).
This is a rules-and-conventions review (project CLAUDE.md, docs and upgrade-guide rules, test conventions); it is not a substitute for static analysis tools.
What works well
- Checking
isEntityagainst the metamodel withisInstance(JpaConsumer.java:406-413) replaces the exception-drivenisArray()fallback, whose TODO from 2011 asked for exactly this kind of alternative. Entities keep the old lock path and failure handling (:384-399). testNoLockIsAttemptedForAValueThatIsNotAnEntity(JpaConsumerScalarResultTest.java:71-87) runs a mockEntityManagerover the real metamodel, so our OpenJPA-only CI also guards the Hibernate concern. The red-before/green-after notes in the description make the change easy to follow.- The test follows the module's JUnit assertion style (camel-jpa has no AssertJ tests), with no
Thread.sleepand no FQCNs, and the catalog copy of the doc is identical to the component doc.
Earlier review, status at fa54e0f
davsclaus's CHANGES_REQUESTED was made on a054697. At this head:
- Lock pre-check: addressed.
lockEntityreturns beforeentityManager.lock(:387/:389) for any result that is not an entity, which also covers the native-queryObject[]path he flagged. - WARN once per consumer: addressed (
:512-521, flag at:69), and the upgrade-guide note says so. isEntityon enhanced entities: CI on fa54e0f (Build and test, SUCCESS) ran the camel-jpa suite on OpenJPA (124 tests, 0 failures), both new tests andJpaWithNativeQueryTest(the native-queryObject[]path through the new gate) included.
The CHANGES_REQUESTED state stays until he re-reviews; no review is requested on the PR right now.
On transaction and EntityManager handling: isEntity only reads the metamodel of the batch EntityManager (calls at :201 and :231), so it adds no persistence-context or transaction side effects. The polling consumer behind pollEnrich is not affected: it sets the lock mode on the query (JpaPollingConsumer.java:155-156) and removes nothing.
Rebase
GitHub reports a conflict. The only file that main changed since the merge base and that this PR also touches is the 4.24 upgrade guide: CAMEL-26135 (0122749) appended its note right after the same camel-jbang note that this PR appends after (head :3859 and :3873). When rebasing, placing the note between two existing notes, for example right after the other camel-jpa note (main :1138, before camel-a2a at :1154), keeps it clear of later appends at the end of the file, such as the camel-jpa note of the open #28496, which also appends there. #28496 does not touch JpaConsumer.java, and its jpa-component.adoc hunk (lines 134-153) does not overlap this one (59-67).
Findings
[Important] The component doc does not describe the new consumer behaviour. The generated Message Body table already says a column query gives one row per message (JpaEndpoint.java:56-59), but "Consuming from the endpoint" (jpa-component.adoc:81-109) only describes entities that are deleted or marked with @Consumed. After this change a value query delivers every row on every poll, and neither consumeDelete nor @Consumed can mark a value as consumed (JpaConsumer.java:498 and :510 only act on entities). The upgrade guide is for migration, so new users will not look for it there. A short paragraph would cover it, for example:
A query that selects values instead of entities, such as a column (`select x.address from Customer x`), or a
native query without `resultClass`, gives one message per row. Such a value is not locked, and it cannot be deleted
or marked with `@Consumed`, so each poll delivers the same rows again: set `consumeDelete=false` and make the query
exclude the rows already handled, or select the entity instead.
[Suggestion] Upgrade-guide note and WARN wording. The note ends with "Set consumeDelete=false for such a query" (camel-4x-upgrade-guide-4_24.adoc:3878-3879). That removes the WARN, but the rows keep coming on every poll, which is what an upgrading user is most likely to trip over: a route that received nothing now gets the whole result on each poll. Could we say it explicitly, for example "each poll delivers the values again, whatever consumeDelete is. Set consumeDelete=false to avoid the WARN, and make the query exclude the rows already handled"? The WARN text (JpaConsumer.java:514-516) also suggests "configure a resultClass", which the consumer reads only for nativeQuery (:420-425); for a selected column, "select the entity" is the hint that helps.
[Suggestion] Producer paragraph: arrays. The rewritten paragraph (jpa-component.adoc:62-66) says a List or an array fails with the entity class, but only says what to use for a List. The endpoint's own producer body description (JpaEndpoint.java:64-66) says "a List or an array of entities needs entityType java.util.List (or java.lang.Object)". Aligning the two would cover array bodies too: "If the body is a List or an array of entities, use java.util.List (or java.lang.Object) as the entity class name ...".
[Suggestion] One more test row (optional). A row with a null address in testScalarResultsAreDelivered (JpaConsumerScalarResultTest.java:50-51) would pin what a NULL column gives at this head: a message with a null body (isInstance(null) is false at JpaConsumer.java:408, and the delete handler logs null at :517).
[Question] Backport. Another review suggests porting this to 4.22.x. If we do: camel-4.22.x has the same lockEntity (the isArray() fallback at its JpaConsumer.java:376), so value queries never delivered on released versions; it is a long-standing bug rather than a regression. A port would also change behaviour in a patch release (every row on every poll), so it would need its own note in the 4.22 guide on main. The rollback-only failure on Hibernate comes from reading Hibernate's code; the author could not reproduce it with the module's Hibernate setup (PR description, Verification).
[Nit] lockEntity's Javadoc still says "@return true if the entity was locked" (JpaConsumer.java:373). It now also returns true when there is nothing to lock; as it is a protected extension point, "true if the result can be processed: it was locked, or there is nothing to lock" would keep overriders on the same page.
[Nit] JpaConsumerScalarResultTest and its methods are public (:44, :49, :71). JUnit 5 does not need it, and recent reviews on apache/camel have asked for new test classes to be package-private. Optional: most tests in this module are still public.
Thanks @vignesh-manel for the careful investigation and the clear write-up.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…es, and review follow-ups - jpa doc: a query that selects values gives one message per row, which is not locked, deleted or marked @consumed, so each poll delivers the same rows again. The producer paragraph covers arrays, and java.lang.Object as the entity class name. - The WARN for a result that is not an entity says each poll delivers it again, and suggests selecting the entity (resultClass applies only to a nativeQuery). - The Javadoc of lockEntity says it also returns true when there is nothing to lock. - JpaConsumerScalarResultTest covers a NULL column (a message with a null body), and is package-private. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc
…tever consumeDelete is Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@oscerd thanks for the review. Done in 848416c and 6c13415 (main merged in at d7fa004, a normal merge, since we don't force-push this branch):
Full camel-jpa suite after Claude Code on behalf of vignesh-manel |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
Comment. Thanks @vignesh-manel, every point of my earlier review is addressed on 6c134150, and CI is green.
Blockers: None
Other findings: none.
- Component doc: "Consuming from the endpoint" now explains that a value query gives one message per row, a NULL column included. It also says that such a value is not locked and cannot be deleted or marked with
@Consumed, and that each poll delivers it again. The catalog copy matches. - Upgrade guide and WARN: both now say that the values come again on every poll, whatever
consumeDeleteis. Both point to the real remedies: exclude the handled rows, or select the entity (resultClassonly for anativeQuery). - Producer paragraph: it covers arrays and
java.lang.Object, in line with the endpoint's own metadata. - Test: the NULL row pins a message with a null body, and the poll counts move to 3 and 6 accordingly. The test class and methods are package-private.
- Javadoc:
lockEntity's@returnmatches what it returns. - Rebase: main is merged in, and the 4.24 note now follows the other camel-jpa note, away from the end of the file.
davsclaus's earlier request for changes still needs his re-review to clear.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed the changes since my last review: the points I raised are addressed, and CI is green on the current head.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Description
https://issues.apache.org/jira/browse/CAMEL-26147
Problem
A JPA consumer whose query selects values that are not entities, such as a column (
select x.address from SendEmail x), delivered nothing with the defaultconsumeLockEntity=true.JpaConsumer.lockEntitycallsentityManager.lockon each result; for aStringor aLongthe lock fails, and the catch block returnedentity.getClass().isArray()(written for the arrays of native queries), which is false.processBatchthen skipped every row, and the only trace was a DEBUG log. The endpoint's own body-type metadata says such a query "gives one row per message: an Object array, or the value of a single column".Change
protected boolean isEntity(Object, EntityManager): whether the result is an instance of an entity type of the metamodel (getMetamodel().getEntities()andisInstance, so a provider's subclass or proxy of an entity still counts as an entity).lockEntity: a result that is not an entity (a selected column, or the Object array of a native query) is processed without a lock attempt, as there is nothing to lock. Following the review,isEntityis checked beforeentityManager.lockinstead of catching the failed lock: on Hibernate the lock throwsDetachedObjectException, and a failed lock can mark the transaction rollback only. For an entity the lock is attempted exactly as before, and a failed lock still skips it. The old catch-block fallback for native-query arrays (and its TODO) is gone, since arrays no longer reach the lock.consumeDeletehandler removes only entities. Fixing the lock alone exposed this:em.removeon theStringfailed (Attempt to cast instance "bar@beer.org [java.lang.String]" to PersistenceCapable failed) and rolled back the whole poll. For any other result it logs the WARN it already logged for the arrays of native queries (CAMEL-24131), with the message extended to selected columns. Following the review, the WARN is logged once per consumer and then at DEBUG, as each poll delivers the same results again.jpa:java.util.List(orjpa:java.lang.Object), and that sent tojpa:<entity class>it fails.resultClass) gives one message per row (a NULL column gives a null body), which is not locked, deleted or marked with@Consumed, so each poll delivers the same rows again; setconsumeDelete=falseand make the query exclude the rows already handled, or select the entity. Both copies ofjpa-component.adocare updated.consumeDeleteis; make the query exclude the rows already handled, or select the entity. With the defaultconsumeDelete=truea WARN is logged once. Following the review, the entry sits after the other camel-jpa entry instead of at the end of the file. The WARN text now suggests selecting the entity (resultClassapplies only to anativeQuery), and the Javadoc oflockEntitysays it also returns true when there is nothing to lock.JpaConsumerScalarResultTest(JUnit assertions, like the other tests of the module; package-private):testScalarResultsAreDelivered: two polls of a column query deliver the values (a NULL column as a null body), and the rows stay.testNoLockIsAttemptedForAValueThatIsNotAnEntity: with a mockEntityManageron the real metamodel,lockEntitynever callslockfor aStringor anObject[], and does call it for aSendEmailentity.Not changed: the ticket also says the doc sentence "For sending to the endpoint, the entityClassName is optional" is wrong because the path is
required = truein the metadata. I checked it with a temporary test:jpa:?usePersist=truewith an entity body persisted it. Only a barejpa:fails, and that is URI syntax. So I left that sentence as it is.Verification (local, JDK 21, Maven 3.9.12)
mock://result Received message count. Expected: <2> but was: <0>, although the poll returned 2.lockEntitychange, it failed with thePersistenceCapableerror above. With both changes it passes: the 2 values are delivered, the 2 rows stay, and the next poll delivers them again.isEntitypre-check,testNoLockIsAttemptedForAValueThatIsNotAnEntityfailed with Mockito "Never wanted here", sincelockwas called withbar@beer.organd with theObject[]. It passes after.-Dhibernate, Hibernate 7.4.13): on a054697, with DEBUG logging, each lock of theStringfailed withDetachedObjectException: Given entity is not associated with the persistence context. In that test setup the poll still committed and the test passed, so I could not reproduce the rollback-only failure, but the pre-check removes the call. On fa54e0f, both tests pass on OpenJPA and on Hibernate, and the WARN appears once in the log across the two polls of two rows.Listor aSendEmail[]sent tojpa:org.apache.camel.examples.SendEmailfails withInvalidPayloadRuntimeException, and aListsent tojpa:java.util.Listis persisted.mvn clean: 52 classes, 124 tests, 0 failures, 1 skipped, both on OpenJPA (default) and on Hibernate (-Dhibernate). (An earlier version of this description said 53 classes and 124 tests: that run had nocleanand counted the report of the deleted temporary test.)mvn install -Psourcecheckpasses incomponents/camel-jpa, with no uncommitted changes afterwards.MonitorStartupTestin camel-jbang-plugin-tui, is unrelated. CI passed on fa54e0f as well (same modules; the two retried tests,AiPanelAcpTestandMonitorStartupTestin camel-jbang-plugin-tui, are unrelated).Listand aSendEmail[]are persisted through bothjpa:java.util.Listandjpa:java.lang.Object, and that a NULL column gives one message with a null body. Full camel-jpa suite aftermvn clean, on OpenJPA and on Hibernate: 52 classes, 124 tests, 0 failures, 1 skipped; sourcecheck clean. main did not touch camel-jpa since the previous merge. CI on 6c13415 is pending.Target
mainbranch)Tracking
Apache Camel coding standards and style
I checked that each commit in the pull request has a meaningful subject line and body.
I have run
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.Not from the root: I built
components/camel-jpawith-Psourcecheckand copied the doc to the catalog. CI's uncommitted-changes check covers the rest.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.Claude Code on behalf of vignesh-manel
🤖 Generated with Claude Code