allthingssecurity opened a new pull request, #27526: URL: https://github.com/apache/camel/pull/27526
# Description [CAMEL-21513](https://issues.apache.org/jira/browse/CAMEL-21513), [CAMEL-25231](https://issues.apache.org/jira/browse/CAMEL-25231) Reported by Nathan (CAMEL-21513) and Salvatore Mongiardo (CAMEL-25231). When the registry has no type converter for the exact pair of types, it looks for a converter for related types. The converters are kept in a `ConcurrentHashMap` keyed by `TypeConvertible`, and the key's hash code comes from the identity hash codes of the two classes. The iteration order of the map can therefore change between JVM runs, and so could the chosen converter when the search took the first match: - Nathan's case: a `DeferredElementNSImpl` converted to `CxfPayload` matched both `elementToCxfPayload` and `nodeListToCxfPayload`. - In Salvatore's CXF case, a stream-cached CXF payload (`CachedCxfPayload`, a `CxfPayload` that implements `StreamCache`) looked up as a `Source` matched two converters. camel-xml-jaxp's `toSource(StreamCache)` gives a `BytesSource` of the serialized payload, and camel-cxf's `cxfPayLoadToSource` gives the payload's first body source as it is (a stream source, or a `DOMSource` of the element). CAMEL-24976 (#26810) already fixed the main search (`TypeResolverHelper.tryMatch`) on `main`, and its description says it fixes the root cause of CAMEL-21513. It walks the value's type hierarchy breadth-first, so the two cases above now give `Element` and `StreamCache` in every run. 4.22.x and older still take the first match, and Salvatore's observations come from a 4.22 based build, so CAMEL-25231 is a symptom of CAMEL-21513 there. #26810 left one search order-dependent, as it says under "Not changed": `tryAssignableFrom`. It is the last resort: `convertTo` uses it after the fallback converters, and `TypeConverterRegistry.lookup` (which does not use fallback converters) uses it after `tryMatch`. It scans all entries and returns the first one whose types are assignable to (or from) the requested types, and more than one entry often matches. I loaded the converters of camel-core, camel-cxf and camel-xml-jaxp and tried every pair of the types that appear in the registry, plus some CXF and DOM classes. 17 pairs reach `tryAssignableFrom` with candidates from different converters. Among them: - `CachedCxfPayload` to `SAXSource`, `StAXSource` or `StreamSource`: the same two converters as above; - `Node` to `CxfPayload`: `documentToCxfPayload` or `elementToCxfPayload`; - `Object[]` to `ArrayList` or `Collection`: camel-core's or CXF's `MessageContentsList` converter. I ran the same JVM with the five `-XX:hashCode` modes. On `main`, 9 to 15 of these 17 requests pick a different converter than in the default mode. This change makes `tryAssignableFrom` choose the nearest candidate: it scans all entries and keeps the one with the fewest levels between its types and the requested types. The levels are counted breadth-first over `getInterfaces()` and `getSuperclass()`, as `tryHierarchy` walks them. Equally near candidates are ordered by the class names of their from and to types. With the change, the same 17 requests pick the same converter in all five hash modes. When only one entry matches, the result is the same as before. What "deterministic" means here: the choice no longer depends on the iteration order of the map, but it still depends on which entries are in it, and those include the entries `doConvertTo` caches for the pairs it has converted (`converters.put(typeConvertible, assignableConverter)`, and likewise after `tryMatch`, a fallback converter and the `Object` converter). A cached entry is a candidate in later `tryAssignableFrom` scans like a registered converter, so a related pair can still get a different converter depending on the conversions done before in the same JVM (history, not hash order), as on main. Example, checked in Java on this branch with converters `I1 -> T` and `I2 -> T`, `A implements I1, I2`, `B extends A implements I2`, `T2 extends T1 extends T`: `convertTo(T2, b)` uses `I2 -> T` (the nearer one) in a fresh registry, but `I1 -> T` after `convertTo(T1, a)`, whose two equally near candidates were decided by name and cached as `A -> T1`, now the nearest key for `B -> T2` . The Lean model has the same case (`history_dependence`). Leaving the cached entries out of the scan would need the registry to tell registered converters from cached ones (and `tryMatch` uses the cached entries too), so I left it as it is. The scan already visited every entry when nothing matched, and the distances are only computed for the entries that match, so the cost stays small. The 4.23 upgrade guide list for the type converter gets one more item. The defect and the fix were checked with a Lean 4 model of the three searches (main's `tryMatch`, the 4.22 `tryMatch` and `tryAssignableFrom`). Types and their direct super types come from `getInterfaces()`/`getSuperclass()`, and the converter map is a list in iteration order: - The 4.22 `tryMatch` picks Element or NodeList for Nathan's `DeferredElementNSImpl`, and `StreamCache` or `CxfPayload` for `lookup(Source, CachedCxfPayload)`, depending on the order. Main's `tryMatch` picks Element and `StreamCache` in both orders, and the model proves that main's `tryMatch` depends only on which converters are registered. - Main's `tryAssignableFrom` still picks either converter for `CachedCxfPayload` to `SAXSource`, and `tryMatch` finds nothing for that pair, so the search does get there. - The model proves, for every map and every requested pair, that the fixed selection depends only on the set of entries in the map, not on their iteration order. It also proves that the result has the smallest distance among the candidates, and that it equals main's result whenever main had at most one candidate. - Taking the first of the equally near candidates, without the name order, would still depend on the order: the two CXF candidates are both one level away. Tests: `CoreTypeConverterRegistryTest.testAssignableMatchIsDeterministic` registers the converters in two orders, using test types shaped like the CXF case and a case where one candidate is nearer, and asserts that the same converter wins. It fails on `main` (two runs): `expected: <implOfSecond> but was: <second>`. With the change: - the camel-core suite passes (8071 tests, 0 failures, 45 skipped; rerun after the last change with 1 error in `ValidatorExternalResourceTest`, which loads an XSD from raw.githubusercontent.com and could not connect from this machine), and so do the suites of the core modules it is built with (9435 tests in total, including camel-util, camel-support, camel-xml-jaxp, camel-xml-io and camel-yaml-io). camel-xslt, camel-xpath and camel-validator keep their tests in camel-core; - the payload and converter tests of camel-cxf-soap (23 classes, 56 tests: `CxfPayloadConverterTest`, `CachedCxfPayloadTest`, `ConverterTest`, the `*PayLoad*`/`*Payload*` route tests, including stream caching and XPath); - camel-xslt-saxon (65 tests, including `XsltSaxonDomSourceTest` from CAMEL-25224) and camel-saxon (xquery, 89 tests, 4 skipped). # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested the core modules and the modules listed above, including the formatter and import-sort plugins. No generated files change. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
