CAMEL-26106: camel-jpa: don't hard-wire the Spring based DefaultTransactionStrategy as the default - #28496

Clean refactor — centralising the "which TransactionStrategy?" decision in JpaHelper.resolveTransactionStrategy(...) and routing JpaComponent, JpaEndpoint, JpaMessageIdRepository and JpaKeyValueRepository through it reads well, and keeping the Spring DefaultTransactionStrategy construction in a single private method so non-Spring runtimes can swap it is a nice touch. Tests cover the default and the from-registry paths for both component and endpoint, the docs are updated, and the upgrade-guide entry is present.

A few non-blocking questions:

1. findSingleByType returns null when the registry holds more than one TransactionStrategy, so the code silently falls back to DefaultTransactionStrategy. The previous behaviour logged a WARN when multiple were found (and the old docs mention it). Worth a LOG.warn/debug when >1 is present and ignored, so a misconfigured registry isn't silent?
2. Could you confirm the previous "look up a TransactionTemplate and extract the strategy from it" behaviour is fully preserved inside DefaultTransactionStrategy (it now runs only when no single TransactionStrategy bean exists), so users relying on a registered TransactionTemplate see no change?
3. The protected JpaEndpoint.createTransactionStrategy() return-type widening (DefaultTransactionStrategy → TransactionStrategy) — correctly called out in the upgrade guide; just flagging it affects any subclass overriding it.

Claude Code on behalf of oscerd

Thanks Jiri, routing the call sites through JpaHelper.resolveTransactionStrategy is a nice clean-up. Keeping the Spring DefaultTransactionStrategy creation in one method makes the Camel Quarkus side much simpler, and Spring and Spring Boot users see no change unless they already have a single TransactionStrategy bean. The docs now describe what the code actually does, and the new WARN plus its test for several beans (8070767) cover the ambiguous case.

A few points:

1. Autowiring. The component lookup goes around Camel's autowiring. entityManagerFactory on JpaComponent is @Metadata(autowired = true), but transactionStrategy (JpaComponent.java:46) is not, so the new findSingleByType in JpaHelper.resolveTransactionStrategy is used even when autowiredEnabled=false is set on the component or the context. Could we mark transactionStrategy as autowired instead? DefaultAutowiredLifecycleStrategy would then set it when the component is added, honouring autowiredEnabled; JpaEndpoint.doInit already copies it from the component, and the catalog would show it as autowired. The endpoint lookup can stay for endpoints on a component without a factory. This needs the catalog, configurer and component DSL regenerated.

2. Logging. The WARN is evaluated on every resolution, so with several beans it is logged once for the component and again for each endpoint without a strategy. Not a big deal, but logging it once (or at DEBUG for the endpoints) would be quieter. An INFO line naming the bean that was picked (findByTypeWithName), like DefaultTransactionStrategy does for its TransactionManager, would also help.

3. Upgrade guide. Two additions would help:

   • A single registry bean now applies to every JPA component and endpoint without a configured strategy, including ones using a different EntityManagerFactory. If the bean is tied to one factory (for example a DefaultTransactionStrategy built for it), set transactionStrategy explicitly.
   • Subclasses that override JpaEndpoint.createTransactionStrategy() must be recompiled: a class compiled against the old DefaultTransactionStrategy return type has a different method descriptor and is otherwise silently not called.

4. Comment on JpaHelper.createDefaultTransactionStrategy. The repositories still pass null as the context, so this private method is what Camel Quarkus will need to substitute. Could the comment say that it is replaced by name, so a later refactoring does not break it? The JIRA also mentioned FactoryFinder, which would avoid patching altogether. Was that considered?

5. Tests. A test for the JpaMessageIdRepository(emf, processorName) default would be good. testJpaEndpointTransactionStrategyFromRegistry also leaves the factory the endpoint creates unclosed.

6. Backport. The JIRA asks for 4.22.x. That changes behaviour (a registry bean is now used) and a protected method signature on an LTS line, so we should agree on it explicitly.

None of these are blockers on their own, but I'd like to settle point 1 before merging.

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.

Solid refactoring. Centralising the strategy-selection logic in JpaHelper.resolveTransactionStrategy() is the right call — the old pattern of each class creating its own DefaultTransactionStrategy was fragile and hard to extend. The new tests cover the key scenarios clearly.

One design observation worth discussing:

JpaMessageIdRepository and JpaKeyValueRepository both call resolveTransactionStrategy(null, entityManagerFactory). With null for the CamelContext argument, the registry lookup is bypassed entirely — these classes always fall back to DefaultTransactionStrategy, regardless of what's registered. This is consistent with the old behaviour (they previously called new DefaultTransactionStrategy(null, emf) directly), but it means users who register a custom TransactionStrategy in the Camel registry and use JpaMessageIdRepository or JpaKeyValueRepository directly won't benefit from the auto-discovery.

Was this intentional? These classes don't hold a CamelContext reference, so null is the only option at construction time. If the intent is to eventually support registry-based resolution here too, that would require adding a CamelContext field (or an alternative constructor). If the intent is to keep them Spring-independent at this layer, null is correct and the Javadoc on resolveTransactionStrategy's camelContext parameter ("or null if not available (then the registry is not used)") makes the design explicit.

No action required — raising it for awareness and to confirm the intent.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Following up on my earlier review — both previous findings (design notes on JpaMessageIdRepository and JpaKeyValueRepository bypassing registry lookup with null camelContext) are addressed: the author confirmed the intent in the thread replies. ✅

One new observation on the updated code:

LOG.info for registry discovery — JpaHelper.resolveTransactionStrategy logs at INFO when a TransactionStrategy is found in the registry. This fires at component/endpoint initialization time; Camel's convention for internal autowiring resolution is DEBUG. INFO is appropriate for user-configured values, but a registry auto-discovery is internal wiring — see inline comment.

The double registry lookup when zero beans are present (findSingleByType → null, then findByType(...).size() for the >1 check) is technically two scans, but it only matters at startup and the cost is negligible. Noting for awareness, not a blocker.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Thanks Jiri. I re-checked the branch at 8753588 against my review of 8070767. Everything I raised is addressed, including point 1, which I wanted settled before merging:

1. Autowiring. transactionStrategy is now @Metadata(autowired = true) (JpaComponent.java:46). The configurer's getAutowiredNames(), both jpa.json files and the catalog doc are regenerated, and CI's uncommitted-changes check passed. The component keeps its own lookup in doInit (JpaComponent.java:186-194), and I first wondered whether this duplicates the autowiring logic. It doesn't. AbstractCamelContext.addComponent initializes the component before postInitComponent runs the autowiring (AbstractCamelContext.java:517-528). Without the lookup, a component added that way with an explicit EntityManagerFactory would create the default first, and autowiring would then skip the non-null option. For components resolved by the context, autowiring sets the option before doInit, so the lookup never runs. The lookup uses the same flags as DefaultAutowiredLifecycleStrategy. The endpoint uses the rule DefaultEndpoint.doInit applies to autowired endpoint options, and the endpoint's autowiredEnabled already includes the context-level flag (DefaultComponent.java:157). Both lookups use findSingleByType, so they always pick the same bean as autowiring. The new tests cover the auto-created component, the addComponent-style init, and autowiredEnabled=false on both the context and the component.
2. Logging. Agreed that endpoints log the WARN only when the component has no EntityManagerFactory, and then each endpoint does create its own default, so one WARN per endpoint is fair.
3. Upgrade guide. Both additions are in: the bean applies whatever the EntityManagerFactory is, and subclasses overriding createTransactionStrategy() must be recompiled.
4. Comment. JpaHelper.java:134-135 now says Camel Quarkus replaces the method by name. Thanks also for explaining why FactoryFinder doesn't fit (no context in the repositories, and only no-arg constructors).
5. Tests. JpaMessageIdRepositoryTest covers the 2-arg constructor, and testJpaEndpointTransactionStrategyFromRegistry closes its factory.
6. Backport. Agreed, main only. CAMEL-26106 still lists 4.22.3 as a fix version, so please remove it.

On gnodet-bot's open thread about JpaHelper.java:106 (INFO vs DEBUG): I asked for an INFO line in my last review, and at component level it matches the INFO that autowiring itself logs. An endpoint that resolves its own strategy logs it once per endpoint, though, and DefaultEndpoint logs endpoint autowiring at DEBUG. So DEBUG for the endpoint path (or for both) is fine with me too. Either way, a reply and resolving that thread is enough. It doesn't block.

Before this can be merged:

• The branch conflicts with main in camel-4x-upgrade-guide-4_24.adoc (new entries were appended at the end on main), so it needs another rebase.
• The red "Build and test" job is not caused by this PR. It is CatalogDocExamplesTest failing on the camel-hibernate doc examples (selectionQuery not applicable in producer/consumer only mode), which main fixed in #28530 after this run started. The rebase picks that fix up.

Approving, conditional on the rebase and a green CI.

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.

Following up on my earlier review — the null-camelContext design findings are addressed. One nit from my previous review remains open.

LOG.info for registry discovery — still INFO in the new commits. See inline comment.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Following up on my earlier review — the remaining open nit (LOG.info → LOG.debug for registry discovery) is now addressed. Line 106 of JpaHelper.resolveTransactionStrategy uses LOG.debug, aligned with Camel’s convention for internal autowiring resolution. All previous gnodet-bot findings are resolved.

The overall refactoring is solid: centralising DefaultTransactionStrategy creation in JpaHelper.createDefaultTransactionStrategy gives Camel Quarkus a clean substitution point (documented in the comment), the @Metadata(autowired = true) annotation on transactionStrategy honours autowiredEnabled correctly, the tests cover the main paths (default, single registry bean, multiple beans → WARN, autowiring disabled at context and component levels, endpoint-only EMF), the EMF resource-leak in testJpaEndpointTransactionStrategyFromRegistry is fixed, and the upgrade guide accurately documents both the behavioural change and the binary-compat break on subclasses of JpaEndpoint.

LGTM.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-checked a6750d4 against my approval of 8753588. The rebase resolved the upgrade-guide conflict: the camel-jpa entry now comes after main's entries, and nothing else in the guide changed. The only code change is the new commit that logs the TransactionStrategy picked from the registry at DEBUG instead of INFO, which settles gnodet-bot's thread. My approval stands, pending a green CI on this head (queued).

One leftover from my last review: CAMEL-26106 still lists 4.22.3 as a fix version. Since this is main only, please remove it.

Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.

Re-checked after the rebase: the PR diff is the same as the one I reviewed on a6750d4 (only upgrade-guide context moved), all earlier points are addressed, threads resolved, and CAMEL-26106 fixVersions is now 4.24.0 only. LGTM.

Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.