Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Disable Jackson default JSON processing limits #17854

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Disable Jackson default JSON processing limits #17854

merged 1 commit into from
Jun 21, 2023

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Jun 12, 2023 •
edited by findepi
Loading

Description

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Fixes #17843

@wendigo wendigo requested a review from kokosing June 12, 2023 15:00
@cla-bot cla-bot bot added the cla-signed label Jun 12, 2023
@wendigo wendigo requested a review from hashhar June 12, 2023 15:00
@wendigo
Copy link
Contributor Author

wendigo commented Jun 12, 2023

I tried to create smallest change possible

@github-actions github-actions bot added iceberg Iceberg connector tests:hive labels Jun 12, 2023
@wendigo wendigo force-pushed the serafin/json-factory-ban branch 3 times, most recently from f8f79a5 to e6ee843 Compare June 13, 2023 04:22
hashhar
hashhar approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good % comments.

re: allowing default factory on serialization paths - we won't know when code is introduced in the serialization places which also does de-serialization. Why not use the no-limits JsonFactory in serialization path as well - doesn't seem to hurt?

testing/trino-benchmark/src/main/java/io/trino/benchmark/JsonBenchmarkResultWriter.java Outdated Show resolved Hide resolved
testing/trino-benchmark/src/main/java/io/trino/benchmark/OdsBenchmarkResultWriter.java Outdated Show resolved Hide resolved
@wendigo wendigo requested review from hashhar and findepi June 13, 2023 07:37
kokosing
kokosing reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change not only ban json direct usage of JsonFactory but it also fixes #17843. Logically these are two different changes it would be nice to keep them as separate commits. Also we should provide test coverage for #17843

client/trino-cli/src/main/java/io/trino/cli/JsonPrinter.java Show resolved Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/util/DefaultJsonFactory.java Outdated Show resolved Hide resolved
lib/trino-hive-formats/src/main/java/io/trino/hive/formats/line/json/JsonSerializer.java Outdated Show resolved Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/util/DefaultJsonFactory.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/operator/scalar/JsonExtract.java Show resolved Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/PartitionData.java Outdated Show resolved Hide resolved
@findepi
Copy link
Member

findepi commented Jun 14, 2023

added "Fixes #17843" in the description

@findepi
Copy link
Member

findepi commented Jun 19, 2023

#17854 (comment) is resolved, please help me understand what the resolution is.
What TrinoJsonFactory subclass is for?

@wendigo
Copy link
Contributor Author

wendigo commented Jun 19, 2023

@findepi disallow mutating already configured JsonFacfory. Instead you should use a builder if you want a different configuration. This makes it easier to reason about factories that are used since they configuration is immutable in the runtime

@findepi findepi mentioned this pull request Jun 19, 2023
Merged
@wendigo wendigo force-pushed the serafin/json-factory-ban branch 2 times, most recently from 82e38e3 to e27855f Compare June 21, 2023 10:26
@wendigo
Copy link
Contributor Author

wendigo commented Jun 21, 2023

@findepi @kokosing dropped the immutability checks. Please review

@wendigo wendigo requested review from findepi and losipiuk June 21, 2023 12:11
findepi
findepi reviewed Jun 21, 2023
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Provide safe way to create JsonFactories"

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/util/JsonUtils.java Show resolved Hide resolved
lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/util/TestJsonUtils.java Outdated Show resolved Hide resolved
lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/util/TestJsonUtils.java Outdated Show resolved Hide resolved
lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/util/TestJsonUtils.java Outdated Show resolved Hide resolved
lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/util/TestJsonUtils.java Outdated Show resolved Hide resolved
@findepi
Copy link
Member

findepi commented Jun 21, 2023

lgtm %

@wendigo wendigo requested a review from findepi June 21, 2023 14:21
@findepi
Copy link
Member

findepi commented Jun 21, 2023

CI checkstyle failure looks related

@wendigo wendigo force-pushed the serafin/json-factory-ban branch 2 times, most recently from b929265 to 48e5c93 Compare June 21, 2023 15:53
@wendigo @findepi
Disable Jackson default JSON processing limits
89c61de 
Jackson 2.15 introduced read constraints that are meant to protect deserialization path
from deeply nested/long string JSON documents. Introduced limits are too small for Trino
to work properly. Airlift is already disabling those limits when ObjectMapperProvider is used.

Additionaly ban direct usage of JsonFactory and JsonFactoryBuilder.
Instead JsonUtils.jsonFactory() or JsonUtils.jsonFactoryBuilder() should be used.
@findepi findepi changed the title Provide safe way to create JsonFactories Disable Jackson default JSON processing limits Jun 21, 2023
@findepi findepi merged commit 0c0f86c into trinodb:master Jun 21, 2023
@wendigo wendigo deleted the serafin/json-factory-ban branch June 21, 2023 20:38
@github-actions github-actions bot added this to the 420 milestone Jun 21, 2023
@colebow colebow mentioned this pull request Jun 22, 2023
Merged
@sgoncharov1 sgoncharov1 mentioned this pull request Sep 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenjian2664 chenjian2664 chenjian2664 approved these changes

@kokosing kokosing kokosing approved these changes

@findepi findepi findepi approved these changes

@electrum electrum Awaiting requested review from electrum

@martint martint Awaiting requested review from martint

@hashhar hashhar Awaiting requested review from hashhar

@losipiuk losipiuk Awaiting requested review from losipiuk

Assignees
No one assigned
Labels
cla-signed iceberg Iceberg connector
Milestone
420
Development

Successfully merging this pull request may close these issues.

JSON functions fail on large input due to Jackson's string length limit
8 participants
@wendigo @findepi @martint @electrum @losipiuk @kokosing @hashhar @chenjian2664