-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to jackson 2.13 #13541
Update to jackson 2.13 #13541
Conversation
28ed2e5
to
e2e518a
Compare
I don't think that |
graylog2-server/pom.xml
Outdated
<dependency> | ||
<!-- Workaround for https://github.com/FasterXML/jackson-bom/pull/38 --> | ||
<groupId>com.fasterxml.jackson.module</groupId> | ||
<artifactId>jackson-module-scala_2.13</artifactId> | ||
<version>2.9.10</version> | ||
<artifactId>jackson-module-scala_3</artifactId> | ||
<version>2.13.4</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure, that we still need the workaround? I think we can remove this dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't. I was just updating versions without looking any deeper.
Removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting a dependency error running the integration tests, so I am reverting the reversion of scala 3.
com.fasterxml.jackson.databind.JsonMappingException: Scala module 2.10.5 requires Jackson Databind version >= 2.10.0 and < 2.11.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickmann Is this a runtime dependency, or can we put it into the test scope?
The stack trace in the test output seems to be rooted in the rest-assured dependency, a testing library.
The jackson-module-scala_3
dependency should also be present in the jackson-bom now. That means we don't need the hardcoded <version>
tag for it.
...rg/graylog/plugins/views/migrations/V20200204122000_MigrateUntypedViewsToDashboardsTest.java
Outdated
Show resolved
Hide resolved
graylog2-server/src/test/java/org/graylog2/jackson/MongoJodaDateTimeSerializerTest.java
Outdated
Show resolved
Hide resolved
graylog2-server/src/test/java/org/graylog2/jackson/MongoZonedDateTimeSerializerTest.java
Outdated
Show resolved
Hide resolved
05d45bf
to
85e3128
Compare
graylog2-server/src/test/java/org/graylog2/plugin/MessageSummaryTest.java
Outdated
Show resolved
Hide resolved
Hah, so they changed it from throwing an exception to returning null and then to returning an empty node object? |
f1bd0d7
to
4ad38c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: #13541 (comment)
f710f4c
to
9bf6690
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
9bf6690
to
2689334
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jackson-bom got bumped to 2.13.4.20221013
to fix a jackson-databind security issue: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.13#micro-patches. Let's update to that version. 🙂
Jackson, a gift that keeps on giving :-/ |
2689334
to
33c523b
Compare
4fac5dd
to
09c2ac3
Compare
Upgrade to the current Jackson version 2.13.4, while keeping everything else unchanged (as much as possible).
https://github.com/Graylog2/graylog-plugin-enterprise/issues/3674
Details
If configuration of
ObjectMapper
is modified after first usage, changes may or may not take effect, and configuration calls themselves may fail [link]. This behavior appears to have changed since version 2.9. It only affects a few tests which attempt to modify the mapper:graylog2-server/src/test/java/org/graylog/events/contentpack/facade/EventDefinitionFacadeTest.java
graylog2-server/src/test/java/org/graylog/events/contentpack/facade/NotificationFacadeTest.java
DateTimeFormatter
now throws an exception for empty string, resulting in a JSON missing node that needs to be handled:graylog2-server/src/main/java/org/graylog/plugins/beats/Beats2Codec.java
Jackson 2.11 changed the default
DateTime
formatting for timezone offset tohh:mm
instead ofhhmm
[link]. We need to explicitly set the legacy format in theObjectMapper
.graylog2-server/src/main/java/org/graylog2/shared/bindings/providers/ObjectMapperProvider.java
graylog2-server/src/test/java/org/graylog/plugins/views/migrations/V20200204122000_MigrateUntypedViewsToDashboardsTest.java
graylog2-server/src/test/java/org/graylog2/jackson/MongoJodaDateTimeSerializerTest.java
graylog2-server/src/test/java/org/graylog2/jackson/MongoZonedDateTimeSerializerTest.java
Jackson no longer includes Joda date/time by default. The Joda module needs to be explicitly registered:
graylog2-server/src/test/java/org/graylog2/plugin/MessageSummaryTest.java
/jenkins-pr-deps Graylog2/graylog-plugin-enterprise#4099,Graylog2/graylog-plugin-enterprise-integrations#902