chore(deps): Update jackson version to 2.18.6#27293
Conversation
imsayari404
force-pushed
the
jacksoncve
branch
4 times, most recently
from
d031572 to
9dd9fac
Compare
|
Regarding : I have reverted this fix in order to ensure testcases aren't breaking in presto-iceberg. |
| createConfiguredObjectMapper(), | ||
| new TaskManagerConfig().setTaskConcurrency(4)); | ||
| } | ||
| private static ObjectMapper createConfiguredObjectMapper() |
There was a problem hiding this comment.
Missing newline. The method can be a static constructor on it's own. The returned mapper a private static final
There was a problem hiding this comment.
I have made the change and refactored to use the JsonObjectMapperUtils utility class.
| this.standaloneSpillerFactory = requireNonNull(standaloneSpillerFactory, "standaloneSpillerFactory is null"); | ||
| this.useNewNanDefinition = requireNonNull(functionsConfig, "functionsConfig is null").getUseNewNanDefinition(); | ||
| } | ||
| // Configure Jackson MixIns and settings to handle serialization issues with Jackson 2.18.6 |
| throw new UnsupportedOperationException(); | ||
| }); | ||
| } | ||
| private static ObjectMapper configureObjectMapper(ObjectMapper mapper) |
There was a problem hiding this comment.
These two methods seems identical to the ones in LocalQueryRunner ? Can you refactor out the common components to a utility class
| configureObjectMapper(new ObjectMapper()), | ||
| new SpoolingOutputBufferFactory(new FeaturesConfig())); | ||
| } | ||
| private static ObjectMapper configureObjectMapper(ObjectMapper mapper) |
| private abstract static class BlockMixIn | ||
| { | ||
| @JsonIgnore | ||
| abstract Block getLoadedBlock(); | ||
| } | ||
|
|
||
| // MixIn to ignore getOutput() method in Slice which causes circular references with BasicSliceOutput | ||
| private abstract static class SliceMixIn | ||
| { | ||
| @JsonIgnore | ||
| abstract Object getOutput(); | ||
| } | ||
|
|
||
| // MixIn to ignore getDiscreteValues() method in ValueSet which throws UnsupportedOperationException | ||
| private abstract static class ValueSetMixIn | ||
| { | ||
| @JsonIgnore | ||
| abstract Object getDiscreteValues(); | ||
| } | ||
|
|
||
| // MixIn to ignore getSingleValue() method and valuesProcessor field in SortedRangeSet | ||
| private abstract static class SortedRangeSetMixIn | ||
| { | ||
| @JsonIgnore | ||
| abstract Object getSingleValue(); | ||
|
|
||
| @JsonIgnore | ||
| abstract Object getValuesProcessor(); | ||
| } | ||
|
|
||
| // MixIn to ignore getSingleValue() method in Range which throws IllegalStateException | ||
| private abstract static class RangeMixIn | ||
| { | ||
| @JsonIgnore | ||
| abstract Object getSingleValue(); | ||
| } | ||
|
|
||
| // MixIn to ignore getSingleValue() and getNullableSingleValue() methods in Domain which throw IllegalStateException | ||
| private abstract static class DomainMixIn | ||
| { | ||
| @JsonIgnore | ||
| abstract Object getSingleValue(); | ||
|
|
||
| @JsonIgnore | ||
| abstract Object getNullableSingleValue(); | ||
| } |
There was a problem hiding this comment.
Why do we need to create individual classes here ? Could we instead create a single IgnoreFieldsMixIn and define the virtual methods with @JsonIgnore on them ?
There was a problem hiding this comment.
Consolidated into a single IgnoreProblematicMethodsMixIn class 👍🏻
imsayari404
requested review from
a team,
ZacBlanco,
feilong-liu,
hantangwangd and
jaystarshot
as code owners
prestodb-ci
requested review from
a team and
Mariamalmesfer
and removed request for
a team
Reviewer's GuideUpdates the global Jackson dependency to 2.18.6 and introduces a centralized ObjectMapper configuration utility to handle Optional support, self-reference and deep nesting issues, wiring it into key planner/execution/testing code paths and updating Nessie to a compatible version. Sequence diagram for configuring ObjectMapper in HistoryBasedPlanStatisticsManagersequenceDiagram
participant HistoryBasedPlanStatisticsManager
participant ObjectMapper_original
participant ObjectMapper_copy
participant JsonObjectMapperUtils
participant CachingPlanCanonicalInfoProvider
HistoryBasedPlanStatisticsManager->>ObjectMapper_original: copy()
ObjectMapper_original-->>HistoryBasedPlanStatisticsManager: ObjectMapper_copy
HistoryBasedPlanStatisticsManager->>ObjectMapper_copy: configure(ORDER_MAP_ENTRIES_BY_KEYS, true)
HistoryBasedPlanStatisticsManager->>ObjectMapper_copy: configure(SORT_PROPERTIES_ALPHABETICALLY, true)
HistoryBasedPlanStatisticsManager->>JsonObjectMapperUtils: configureObjectMapperForJackson2186(ObjectMapper_copy)
JsonObjectMapperUtils-->>HistoryBasedPlanStatisticsManager: configured ObjectMapper_copy
HistoryBasedPlanStatisticsManager->>CachingPlanCanonicalInfoProvider: new(historyBasedStatisticsCacheManager, ObjectMapper_copy, metadata)
CachingPlanCanonicalInfoProvider-->>HistoryBasedPlanStatisticsManager: instance ready
Class diagram for centralized Jackson 2_18_6 ObjectMapper configurationclassDiagram
class JsonObjectMapperUtils {
<<final>>
+static ObjectMapper configureObjectMapperForJackson2186(ObjectMapper objectMapper)
+static ObjectMapper createConfiguredObjectMapper()
}
class IgnoreProblematicMethodsMixIn {
<<abstract>>
+Object getLoadedBlock()
+Object getOutput()
+Object getDiscreteValues()
+Object getSingleValue()
+Object getValuesProcessor()
+Object getNullableSingleValue()
}
class LocalExecutionPlanner {
-ObjectMapper sortedMapObjectMapper
}
class HistoryBasedPlanStatisticsManager {
-ObjectMapper newObjectMapper
}
class LocalQueryRunner {
-ObjectMapper objectMapper
}
class ObjectMapper
class Block
class Slice
class ValueSet
class SortedRangeSet
class Range
class Domain
JsonObjectMapperUtils ..> ObjectMapper : configures
JsonObjectMapperUtils ..> Block : addsMixIn
JsonObjectMapperUtils ..> Slice : addsMixIn
JsonObjectMapperUtils ..> ValueSet : addsMixIn
JsonObjectMapperUtils ..> SortedRangeSet : addsMixIn
JsonObjectMapperUtils ..> Range : addsMixIn
JsonObjectMapperUtils ..> Domain : addsMixIn
JsonObjectMapperUtils *-- IgnoreProblematicMethodsMixIn : innerMixIn
IgnoreProblematicMethodsMixIn <|.. Block : appliedAsMixIn
IgnoreProblematicMethodsMixIn <|.. Slice : appliedAsMixIn
IgnoreProblematicMethodsMixIn <|.. ValueSet : appliedAsMixIn
IgnoreProblematicMethodsMixIn <|.. SortedRangeSet : appliedAsMixIn
IgnoreProblematicMethodsMixIn <|.. Range : appliedAsMixIn
IgnoreProblematicMethodsMixIn <|.. Domain : appliedAsMixIn
LocalExecutionPlanner ..> JsonObjectMapperUtils : configureObjectMapperForJackson2186
HistoryBasedPlanStatisticsManager ..> JsonObjectMapperUtils : configureObjectMapperForJackson2186
LocalQueryRunner ..> JsonObjectMapperUtils : createConfiguredObjectMapper
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
JsonObjectMapperUtils.configureObjectMapperForJackson2186name bakes in a specific Jackson version and will age poorly; consider a version-agnostic name (e.g.,configurePlanObjectMapperor similar) so future upgrades don't require a rename or misleading method name. configureObjectMapperForJackson2186both mutates the passedObjectMapperand returns it, but callers sometimes ignore the return value; it would be clearer to either make this method purely functional (return a new mapper) or clearly side-effecting (void, with all callers using the same pattern).- In
TestIcebergSystemTablesNessieWithBearerAuth, the relaxed regex that also accepts theNessieErrorNPE will mask regressions now that Nessie is bumped to 0.105.0; consider keeping a stricter assertion and, if needed, gating transitional behavior on the dependency version instead of permanently broadening the expected failure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `JsonObjectMapperUtils.configureObjectMapperForJackson2186` name bakes in a specific Jackson version and will age poorly; consider a version-agnostic name (e.g., `configurePlanObjectMapper` or similar) so future upgrades don't require a rename or misleading method name.
- `configureObjectMapperForJackson2186` both mutates the passed `ObjectMapper` and returns it, but callers sometimes ignore the return value; it would be clearer to either make this method purely functional (return a new mapper) or clearly side-effecting (void, with all callers using the same pattern).
- In `TestIcebergSystemTablesNessieWithBearerAuth`, the relaxed regex that also accepts the `NessieError` NPE will mask regressions now that Nessie is bumped to 0.105.0; consider keeping a stricter assertion and, if needed, gating transitional behavior on the dependency version instead of permanently broadening the expected failure.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
imsayari404
force-pushed
the
jacksoncve
branch
2 times, most recently
from
a55efa9 to
d5c94f8
Compare
aaneja
left a comment
aaneja
left a comment
There was a problem hiding this comment.
We need to fix the underlying via jackson annotations or create use-case based Json serde classes. Let's create TODOs for these and follow up.
Approving since tests are passing with the current Ignore mix-ins
| private abstract static class IgnoreProblematicMethodsMixIn | ||
| { | ||
| // Ignore Block.getLoadedBlock() - causes circular reference | ||
| @JsonIgnore |
There was a problem hiding this comment.
For this circular reference why not add the @JsonIgnore to the Block interface directly ?
There was a problem hiding this comment.
See also - BlockJsonSerde class
There was a problem hiding this comment.
I agree that adding @JsonIgnore directly to the Block interface would be cleaner. I used the MixIn approach to fix the Jackson 2.18.6 compatibility issue without modifying multiple interface files.
We need to fix the underlying via jackson annotations or create use-case based Json serde classes. Let's >create TODOs for these and follow up.
sure, will do that
| // Ignore Range.getSingleValue() - throws IllegalStateException | ||
| // Ignore Domain.getSingleValue() - throws IllegalStateException | ||
| @JsonIgnore | ||
| abstract Object getSingleValue(); |
There was a problem hiding this comment.
For these - can you reply with the actual IllegalStateException text ?
There was a problem hiding this comment.
Domain.getSingleValue() (Domain.java):
throw new IllegalStateException("Domain is not a single value");
Range.getSingleValue() (Range.java):
throw new IllegalStateException("Range does not have just a single value");
SortedRangeSet.getSingleValue() (SortedRangeSet.java):
throw new IllegalStateException("SortedRangeSet does not have just a single value");
There was a problem hiding this comment.
These are just the default method implementations that Jackson is now calling. Makes sense to JsonIgnore them 👍
|
@imsayari404 imported this issue as lakehouse/presto #27293 |
prestodb-ci
added
follow-up-1
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
prestodb-ci
added
follow-up-2
Update: Airbase Jackson UpgradeI created an airbase PR to upgrade Jackson there: prestodb/airbase#43 Key findings: Airbase manages Jackson version (currently 2.10.0): https://github.com/prestodb/airbase/blob/master/airbase/pom.xml#L147 Airlift already supports Jackson 2.18.6 where ObjectMapperProvider registers Jdk8Module(): This Presto PR adds Presto-specific ObjectMapper configurations for Block, Domain, Range classes that don't exist in airbase/airlift. Both PRs are independent and can proceed in parallel. Hi @tdcmeehan , Could you please review when you get a chance? |
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
prestodb-ci
added
follow-up-3
|
Update: Following to fix root causes with @JsonIgnore annotations, I've added @JsonIgnore directly to all Presto-owned classes (Block, Domain, Range, ValueSet, SortedRangeSet). For the external Slice library circular reference issue (Slice.getOutput() → BasicSliceOutput → infinite loop), I've added a temporary MixIn in HistoryBasedPlanStatisticsManager.java and LocalExecutionPlanner.java while the permanent fix is in progress via upstream PR: prestodb/slice#4. Once the Slice PR is approved, I'll update the Slice dependency version in this pr and remove the temporary MixIns. |
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
prestodb-ci
added
follow-up-4
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
|
@imjalpreet / @aaneja / @nishithakbhaskaran, could you please review when you get a chance? |
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
| .maxNestingDepth(2000) | ||
| .build()); | ||
|
|
||
| // Added MixIn for Slice circular reference, it's an external library |
There was a problem hiding this comment.
With prestodb/slice#4 this become redundant right ?
There was a problem hiding this comment.
Yes, exactly once prestodb/slice#4 is merged and released, this MixIn becomes redundant
nishithakbhaskaran
left a comment
nishithakbhaskaran
left a comment
There was a problem hiding this comment.
Appreciate the effort @imsayari404 !!
Thanks for the upgrade
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
4 participants
Description
Upgrades
dep.jackson.versionfrom2.15.4to2.18.6to remediate GHSA-72hv-8253-57qq.Test Failures Fixed
The upgrade exposed pre-existing issues in Presto's ObjectMapper usage that 2.15.4 happened to tolerate. Each fix below is a direct response to an actual CI failure with a concrete stack trace.
Failure 1 — TestPrestoSparkStatsCalculator
and
failed testcase :
presto-spark-base/src/test/java/com/facebook/presto/spark/planner/TestPrestoSparkStatsCalculator.javaRoot cause:
The ObjectMapper used in HistoryBasedPlanStatisticsManager (created via objectMapper.copy()) lacked Jdk8Module registration, leaving CachingPlanCanonicalInfoProvider unable to serialize Optional fields.
HistoryBasedPlanStatisticsManagerhas a single commit since creation, meaningJdk8Modulewas never explicitly registered.Fix:
.registerModule(new Jdk8Module())afterobjectMapper.copy()Failure 2 — TestDriver
Root cause:
TestDriverpasses a barenew ObjectMapper()directly tocreateFragmentResultCacheContext(), bypassingHistoryBasedPlanStatisticsManagerentirely. WithoutJdk8Module, serialization fails silently,createFragmentResultCacheContext()returnsOptional.empty(), and.get()in the static initializer throwsNoSuchElementExceptionpreventing theentire test class from loading.
Fix:
registerModule(new Jdk8Module())+FAIL_ON_SELF_REFERENCES=falsemaxNestingDepth(2000)+BlockMixInon the bareObjectMapperFailure 3 — LocalQueryRunner / LocalExecutionPlanner
Root cause:
Block.getLoadedBlock()returnsBlockitself a circularreference. Confirmed in source:presto-common/.../block/Block.java:378
default Block getLoadedBlock()← returns thisAdditionally
ValueSet.getDiscreteValues()throwsUnsupportedOperationExceptionand
Range.getSingleValue()throwsIllegalStateExceptionwhen serialized:presto-common/.../predicate/ValueSet.java:114throw new UnsupportedOperationException();and
presto-common/.../predicate/Range.java:166throw new IllegalStateException("Range does not have just a single value");Jackson 2.18.6's stricter error handling exposes these pre-existing issues when attempting to serialize getter methods that throw exceptions or create circular references.
Fix:
FAIL_ON_SELF_REFERENCES=false+@JsonIgnoreMixIns onBlock,Slice,ValueSet,SortedRangeSet,Range,Domain+maxNestingDepth(2000)for complex Presto plan structuresFailure 4 — TaskTestUtils / TestSqlTaskManager
Root cause: Same
Block.getLoadedBlock()circular reference. Both files pass barenew ObjectMapper()toSqlTaskManager.Fix:
configureObjectMapper()helper withBlockMixIn+FAIL_ON_SELF_REFERENCES=false+maxNestingDepth(2000)Failure 5 — TestIcebergSystemTablesNessieWithBearerAuth
Root cause: Nessie client 0.103.0 is incompatible with Jackson 2.18.6 , HTTP error responses fail to deserialize, returning
nullinstead of a properNessieErrorobject.Fix: Bump Nessie
0.103.0 → 0.105.0which adds Jackson 2.18.6compatibility. Test updated to accept both error patterns during the transition window.Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Upgrade Jackson to 2.18.6 and adjust Presto’s JSON serialization configuration and related dependencies to remain compatible with the stricter behavior.
Bug Fixes:
Enhancements:
Build:
Tests: