DRILL-8415: Upgrade Jackson 2.14.3 → 2.16.1

[jnturton]

DRILL-8415: Upgrade Jackson 2.14.3 → 2.16.1

Description

The following should be investigated before merging.

There are some security focused enhancements including a new class called StreamReadConstraints. The defaults on StreamReadConstraints are pretty high but it is not inconceivable that some Drill users might need to relax them. Parsing large strings as numbers is sub-quadratic, thus the default limit of 1000 chars or bytes (depending on input context).

When the Drill team consider upgrading to Jackson 2.15 or above, you might also want to consider adding some way for users to configure the StreamReadConstraints.

Documentation

N/A

Testing

Unit tests pass.

[Lceeba]

Unsubscribe

…

On Wed, 3 Jan, 2024, 13:41 James Turton, ***@***.***> wrote: DRILL-8415 <https://issues.apache.org/jira/browse/DRILL-8415>: Upgrade Jackson 2.14.3 → 2.16.1 Description The following should be investigated before merging. There are some security focused enhancements including a new class called StreamReadConstraints. The defaults on StreamReadConstraints <https://javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.15.0-rc1/com/fasterxml/jackson/core/StreamReadConstraints.html> are pretty high but it is not inconceivable that some Drill users might need to relax them. Parsing large strings as numbers is sub-quadratic, thus the default limit of 1000 chars or bytes (depending on input context). When the Drill team consider upgrading to Jackson 2.15 or above, you might also want to consider adding some way for users to configure the StreamReadConstraints. Documentation N/A Testing Unit tests pass. ------------------------------ You can view, comment on, or merge this pull request online at: #2866 Commit Summary - 827521f <827521f> Upgrade Jackson 2.14.3 → 2.16.1. File Changes (1 file <https://github.com/apache/drill/pull/2866/files>) - *M* pom.xml <https://github.com/apache/drill/pull/2866/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8> (2) Patch Links: - https://github.com/apache/drill/pull/2866.patch - https://github.com/apache/drill/pull/2866.diff — Reply to this email directly, view it on GitHub <#2866>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZFXPXLDZDST7EC5F5TWEDYMUHDDAVCNFSM6AAAAABBLB6LK2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGA3DGNBWGIZTCOA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[cgivre]

LGTM +1 (Pending CI)

[jnturton]

I starting adding congifuration support for the new StreamReadConstraints, first globally and then just in the JSON reader, but I got stopped by a sense of YAGNI. It's hard to imagine someone who will need something beyond the default values in Jackson and more configuration is more complexity that users must contend with. So my opinion at this point is that we should only add that configurability if someone asks for it...

[cgivre]

@jnturton This looks good however there is a merge conflict. Can you please resolve so that we can run the CI?

[jnturton]

I haven't rebased this yet in case we decide to squash the WIP commits that were merged into master. Once a decision is made either way this can be rebased and a CI run obtained.

[cgivre]

I haven't rebased this yet in case we decide to squash the WIP commits that were merged into master. Once a decision is made either way this can be rebased and a CI run obtained.

I'm fine with leaving the WIP commits as long as we don't make a habit out of it. It's probably more of a hassle to undo the PR, squash the commits and re-merge them.