-
Notifications
You must be signed in to change notification settings - Fork 156
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
jackson 2.16 support #564
jackson 2.16 support #564
Conversation
c3d3add
to
96aacd5
Compare
# these defaults are the same as the defaults in `StreamReadConstraints` | ||
max-nesting-depth = 1000 | ||
max-number-length = 1000 | ||
max-string-length = 20000000 |
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.
read.constraints {
max-nesting-depth = 1000
max-number-length = 1000
max-string-length = 20000000
}
Seems more clear
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.
I honestly don't think adding the word constraints
to the config names here makes them any clearer. Config names need to balance descriptiveness with conciseness.
jackson 2.16.0 should be out soon and it has some more constraints - I will repurpose this PR to further update to jackson 2.16.0 and support the extra constraints |
96aacd5
to
7e2d1a4
Compare
b3732ba
to
c3f0b7e
Compare
.maxNumberLength(config.getInt("read.max-number-length")) | ||
.maxStringLength(config.getInt("read.max-string-length")) | ||
.maxNameLength(config.getInt("read.max-name-length")) | ||
.maxDocumentLength(config.getLong("read.max-document-length")) |
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.
Do these configurations have default values, or users must specify the configuration?
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 ship reference conf with the defaults. Users can override the values by setting them in their own application.conf or application.json.
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.
This is how Akka/Pekko has always worked.
Update JacksonObjectMapperProvider.scala Update JacksonObjectMapperProvider.scala Update JacksonObjectMapperProvider.scala
c3f0b7e
to
cf2a742
Compare
@He-Pin @mdedetrich is this something we can merge into main branch? Jackson 2.16 has quite a bit of security hardening. |
@mdedetrich if we are getting close to creating an RC for the M1 of pekko 1.1.0, I would like to get this merged. |
Okay ill look at it in more detail tomorrow |
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
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
max-name-length = 50000 | ||
# max-document-length of -1 means unlimited | ||
max-document-length = -1 | ||
} |
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.
Add a blank line here
see https://www.javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.16.0/com/fasterxml/jackson/core/StreamReadConstraints.html
Users who want this feature now can use these libs instead:
For Pekko 1.1, I still want to get this merged centrally.