gnodet commented on PR #21800:
URL: https://github.com/apache/camel/pull/21800#issuecomment-4037001940
## In-depth Review of Jackson 3 Migration
Impressive effort migrating 507 files across the entire Camel codebase. The
dual-BOM approach (Jackson 2 first, Jackson 3 second) in the parent POM is
well-structured, and the 5 excluded components are properly isolated. Here are
the findings organized by severity.
---
### Critical / High
**1. Salesforce `JsonUtils.createObjectMapper()` silently drops date
features**
The old code configured:
```java
objectMapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS,
false);
objectMapper.configure(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE,
false);
```
The new code drops both. `WRITE_DATES_AS_TIMESTAMPS` defaults to `true`, so
dates will serialize as numeric timestamps instead of ISO strings — a silent
behavioral regression affecting all Salesforce API interactions involving date
fields.
**2. ServiceNow `getOrCreateMapper()` drops all custom date/time
serialization**
The method was drastically simplified, removing `JavaTimeModule`
registration with custom date/time serializers using `dateFormat`,
`timeFormat`, and `dateTimeFormat` patterns. These configuration properties
still exist on `ServiceNowConfiguration` but are **now dead code**. Users with
custom date/time format settings will find them silently ignored.
**3. CBOR registry lookup changed from `ObjectMapper` to
`TokenStreamFactory`**
In `CBORDataFormat`, old code looked up `ObjectMapper` instances in the
registry:
```java
Set<ObjectMapper> set =
camelContext.getRegistry().findByType(ObjectMapper.class);
```
New code looks for `TokenStreamFactory` instead. Users who registered a
`CBORMapper` in the Camel registry will no longer be auto-discovered — a
breaking change in the component's contract.
---
### Medium
**4. `SObjectCompositeResponse`/`SObjectCompositeResult` — behavioral fix
hidden in migration**
`@JsonProperty` changes from `"results"` → `"compositeResponse"` and
`"referenceID"` → `"referenceId"` are bug fixes to match the actual Salesforce
API field names, not pure migration changes. Should be called out in the PR
description.
**5. `jackson-jq` upgraded to `2.0.0-alpha1`**
Using an alpha release in production carries stability risk. This is
necessary for Jackson 3 compatibility, but worth flagging.
**6. Exception handling regressions from checked → unchecked migration**
Several places lost meaningful error handling when
`IOException`/`JsonProcessingException` (checked) became `JacksonException`
(unchecked):
- `LoginConfigHelper.loadFromSfCliCredentials()` — try-catch removed
entirely; malformed credentials now crash instead of being gracefully logged
and skipped
- `AbstractDTOBase.toString()` — can now throw unchecked from `toString()`
instead of returning fallback string
- `DefaultBulkApiV2Client.marshalRequest()`/`unmarshalResponse()` —
`SalesforceException` wrapping lost; raw `JacksonException` propagates,
changing the error contract
**7. `MicrometerModule` — half-completed IOException migration**
`serializeStatistics()` still declares `throws IOException` and the caller
wraps it in a try-catch, but Jackson 3's `JsonGenerator` no longer throws
checked `IOException`. The `throws IOException` is dead code.
**8. Deleted test `testUndertowPojoTypePutUserFail`**
In `RestUndertowHttpPojoTypeTest`, the test validating that sending a wrong
POJO type returns HTTP 400 was removed entirely. This could mask a behavioral
change in Jackson 3's type handling.
**9. `LocalDate` serializer/deserializer dropped from Salesforce
`TimeModule`**
Registration was removed entirely. If any Salesforce DTO uses `LocalDate`,
it falls back to Jackson 3's default handling, which may differ from Jackson
2's `JavaTimeModule`.
---
### Low / Minor
**10. Hardcoded version in `camel-salesforce-maven-plugin`**
`jsonschema-generator` is pinned to `4.38.0` while the parent property is
`5.0.0`. Should use the property or document why.
**11. Inconsistent `asText()` → `asString()` vs `textValue()` migration**
Some files use `asString()` (correct Jackson 3 rename), others use
`textValue()` (returns `null` for non-text nodes instead of converting).
`GistHelper` and `GitHubHelper` use `textValue()` which changes behavior.
**12. Redundant `${jackson3-version}` on some deps**
Several components specify explicit versions on `tools.jackson` deps that
are already managed by the parent BOM (e.g., `camel-docling`,
`camel-langchain4j-chat`, `camel-flowable`). Not harmful, just inconsistent.
**13. Inefficient repeated `rebuild().build()` in `CBORDataFormat`**
Each feature toggle creates a new ObjectMapper. Should `rebuild()` once,
configure all features, then `build()` once.
**14. `new ObjectMapper()` vs `JsonMapper.builder().build()`**
Several files (`MongoDbEndpoint`, `LoginConfigHelper`, `NodeState`) use `new
ObjectMapper()` directly. In Jackson 3, the idiomatic pattern is
`JsonMapper.builder().build()`. Not a bug but inconsistent with the rest of the
PR.
**15. Dead property `jackson-databind-nullable-version`**
Defined in parent POM but appears unused across the entire codebase.
---
### What looks good
- Dual-BOM strategy with correct import order
- Jackson 2 annotations (`com.fasterxml.jackson.annotation`) correctly kept
unchanged
- `TextNode` → `StringNode` consistently applied
- `fieldNames()` → `propertyNames().iterator()` correctly applied
- ObjectMapper immutability handled correctly in most places (Salesforce
`JsonUtils`, `SObjectCompositeTest`, etc.)
- `AsNestedPropertyDeserializer` correctly adds `strictTypeIdHandling=false`
for Jackson 3
- networknt json-schema-validator API migration appears correct
- `JSONAssert` properly introduced for order-insensitive JSON comparison in
tests
- 5 excluded components properly isolated with no classpath conflicts
--
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]
