Repository navigation
Conversation
…when no converter is found EnumTypeConverter asks the registry for a dedicated type converter on every conversion to an enum, and TypeConverterRegistry.lookup resolved it with TypeResolverHelper.doLookup every time: it walks the type hierarchy twice and scans all the type converters, and it caches neither a found converter nor a miss. The registry now caches the results of lookup, misses included, in a map of its own, so listTypeConverters and the statistics do not change. The cache is cleared when a type converter is added (clearMisses), removed, or put for a pair of types while converting (such as a fallback that converted), and a lookup that raced with such a change drops its result. EnumTypeConverter caches the enum constants per enum type with a ClassValue, as getEnumConstants copies them on every call, and its tryConvertTo returns null instead of creating an exception that would be ignored when no enum constant matches. Measured locally (JDK 25 on macOS, single thread, after warm-up): convertTo(LoggingLevel, "INFO") 1180 ns and 1560 B before, 14 ns and 24 B after; tryConvertTo(LoggingLevel, "NOPE") 2604 ns and 4040 B before, 165 ns and 856 B after. Part of CAMEL-25132 (tier 1, item apache#6). _Claude Code on behalf of Adriano Machado (@ammachado)_ _This was generated by an AI agent and may contain inaccuracies. Please verify before relying on it._ Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ammachado
marked this pull request as ready for review
October 29, 2026 04:17
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
reviewed
Oct 29, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Review of CAMEL-25231
One minor documentation suggestion — the code is correct as-is.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…nverter on the try path When trying to convert to an enum, EnumTypeConverter called convertTo on a direct type converter found by lookup, so a failing conversion threw an exception that was only ignored. Call tryConvertTo on the try path instead. Co-authored-by: Claude <noreply@anthropic.com> _Claude Code on behalf of Adriano Machado (@ammachado)_ _This was generated by an AI agent and may contain inaccuracies. Please verify before relying on it._
…rce type chosen for a CachedCxfPayload A CachedCxfPayload is both a CxfPayload and a StreamCache, and both have converters to the XML sources, so which converter was chosen used to depend on the iteration order of the converters map, which can change between JVM runs. CAMEL-24976 (tryMatch walks the type hierarchy in a stable order, interfaces before the super class) and CAMEL-21513 (tryAssignableFrom picks the nearest converter) already fixed that on main; this adds the regression tests for the CXF case. camel-cxf-soap: the converters from StreamCache are chosen for a CachedCxfPayload, a DOMSource holds the document and not its document element (the trigger of CAMEL-25224), and xpath evaluates on a cached payload built from a DOM and from a StAX body. camel-core: the type converter lookup cache of CAMEL-26392 keeps that choice. lookup and convertTo both choose the converter of the interface over the one of the super class, whatever the order the converters were added in. Both new order tests fail when tryMatch tries the super class before the interfaces. _Claude Code on behalf of Adriano Machado (@ammachado)_ _This was generated by an AI agent and may contain inaccuracies. Please verify before relying on it._ Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ammachado
force-pushed
the
CAMEL-25231-cxf-payload-source-determinism
branch
from
October 29, 2026 04:33
ecaefef to
35581d1
Compare
gnodet-bot
reviewed
Oct 29, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
✅ Re-review on 35581d1: solid regression test PR stacked on #28651. The concurrency pattern in the lookup cache is correct, EnumTypeConverter.tryConvertTo follows the established Camel pattern, and the CXF tests properly exercise the determinism fix.
The prior documentation suggestion (why two-arg remove is used in lookup()) remains unaddressed — still non-blocking, already noted in the existing thread.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
|
🧪 CI tested the following changed modules:
🔁 2 tests passed only after a retry on JDK 21 (2 retried attempts) Recovered flaky tests on JDK 21 (2)
🔬 Scalpel shadow comparison — Scalpel: 570 of 708 tested, 27 compile-only — current: 571 all testedMaveniverse Scalpel detected 570 affected modules (current approach: 571). Skip-tests mode would test 570 modules (3 direct + 567 downstream), skip tests for 27 (generated code, meta-modules) Modules only in current approach (1)
Modules Scalpel would test (570)
Modules with tests skipped (27)
Build reactor — dependencies compiled but only changed modules were tested (3 modules, 7.7s total)Total reactor time: 7.7s
Top 20 slowest modules:
|
davsclaus
reviewed
Oct 29, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
The tests are sound (CachedCxfPayload -> StreamCache converters, DOMSource, XPath, and the interface-vs-superclass case in both registration orders), and no production change is consistent with main having fixed this already.
- The body says to keep it a draft until #28651 lands, but it is not a draft and carries both of 28651's commits. Please convert it to draft or rebase after 28651 merges.
- Nit: in
testCachedCxfPayloadUsesTheStreamCacheConverters,assertNotSame(registry.lookup(type, CxfPayload.class), fromStreamCache)passes trivially when the lookup returns null. - Nit: the new test builds a
DefaultCamelContextthat is never closed. - Open thread on the two-argument
lookups.remove(typeConvertible, answer)comment concerns 28651's code.
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.
davsclaus
marked this pull request as draft
October 29, 2026 08:09
oscerd
reviewed
Oct 29, 2026
oscerd
left a comment
oscerd
left a comment
Contributor
There was a problem hiding this comment.
The regression tests here are good and target exactly the property that the type-converter caching has to preserve. A CachedCxfPayload is both a StreamCache and a CxfPayload, and both have converters to the XML sources; resolution tries the interface before the super class, so the StreamCache converters must always win for a CachedCxfPayload no matter what order the converters sit in the registry (which can differ between JVM runs). testCachedCxfPayloadUsesTheStreamCacheConverters pins that precisely — for each of Source/SAXSource/DOMSource/StAXSource/StreamSource/Document it asserts the StreamCache converter differs from the CxfPayload one and that CachedCxfPayload resolves to the StreamCache one. That's the kind of registry-ordering nondeterminism that otherwise shows up as a run-to-run flake, so it's a valuable guard, and it complements the lookup caching directly (the cache must not memoise the wrong one). The DOMSource-holds-the-Document and XPath tests additionally lock down CAMEL-25224 (evaluating from the right context node). CI is green.
One coordination note, not a code issue: this PR and #28651 (CAMEL-26392) both contain the identical CoreTypeConverterRegistry and EnumTypeConverter changes, both based on main — I checked and the added/removed lines match exactly. So the two will conflict if both merge as-is. It'd be cleaner to land the core caching once (e.g. in #28651) and rebase this one down to just the cxf regression tests, or fold them into a single PR — however you prefer to sequence CAMEL-26392 and CAMEL-25231. Flagging so it's a deliberate choice rather than a merge-time surprise.
No blockers on the test content.
Claude Code on behalf of oscerd
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
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.
Description
CAMEL-25231: regression tests for the source type the type converter registry chooses for a CXF
CachedCxfPayload.Stacked on #28651 (CAMEL-26392). This PR carries the commits of #28651 too; only the last commit (
CAMEL-25231: ...) is new here. I'll rebase it once #28651 is merged, and keep it a draft until then.Background
A
CachedCxfPayloadextendsCxfPayloadand implementsStreamCache, and both have converters to the XML sources (camel-xml-jaxp's fromStreamCache,CxfPayloadConverter's fromCxfPayload). The ticket reports that the converter chosen for it changed between JVM runs, because it depended on the iteration order of the converters map. That non-determinism triggered CAMEL-25224 (xslt).As gnodet noted on the ticket, main already fixed this, with no production change needed here:
tryMatchwalks the type hierarchy breadth first in a stable order, interfaces before the super class.tryAssignableFrompicks the nearest converter, ties broken by class name.CoreTypeConverterRegistryTest.testAssignableMatchIsDeterministiccovers the helper with a model of the CXF types. This PR adds tests at the levels the ticket is about: through the registry's publiclookup/convertTo(whichCxfPayloadConverter's fallback calls, and which #28651 now caches), and with the real CXF types.Tests added
camel-cxf-soap,
CachedCxfPayloadTest:testCachedCxfPayloadUsesTheStreamCacheConverters: forSource,SAXSource,DOMSource,StAXSource,StreamSourceandDocument,lookup(type, CachedCxfPayload)returns the converter fromStreamCache, which differs from the one fromCxfPayload.testCachedCxfPayloadToDOMSourceHoldsTheDocument: theDOMSourceholds theDocument, not its document element (the shape that bypassed a template matching/in CAMEL-25224), for a payload built from a DOM body and from a StAX body.testXPathOnCachedCxfPayload: a namespaced xpath evaluates on both payloads.camel-core,
TypeConverterLookupCacheTest:testCachedLookupFromInterfaceAndSuperClassIsDeterministic: with converters from an interface and from the super class added in either order,lookupandconvertToboth choose the interface's converter (only that one is invoked), and the cached lookup keeps returning it.Verification
TypeConverterLookupCacheTest,CoreTypeConverterRegistryTestandEnumConverterTest(25 tests); camel-cxf-soapCachedCxfPayloadTestandCxfPayloadConverterTest(20 tests). I did not run the full module suites.tryMatchchanged to try the super class before the interfaces,testCachedCxfPayloadUsesTheStreamCacheConvertersandtestCachedLookupFromInterfaceAndSuperClassIsDeterministicfail. The DOMSource and xpath tests still pass under that mutation; they check behavior, not the order.CachedCxfPayloadwith camel-base built fromorigin/mainand then from the CAMEL-26392: camel-core - Type converter: cache lookup results, also when no converter is found #28651 branch: the results were identical, so the lookup cache does not change the choice.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 folder: the change is test-only. I ran
mvn formatter:format impsort:sortand the tests incore/camel-coreandcomponents/camel-cxf/camel-cxf-soap, andgit statuswas clean afterwards.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.🤖 Generated with Claude Code on behalf of Adriano Machado (@ammachado)
This was generated by an AI agent and may contain inaccuracies. Please verify before relying on it.