Skip to content

Relax limit on max string size #96031

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

Merged
merged 2 commits into from
May 11, 2023
Merged

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented May 11, 2023

Jackson 2.15 introduced a (rough) maximum limit on string length. This commit relaxes that limit to its maximum size, leaving document size constraints to other existing limits in the system. We can revisit whether string length within a document should be independently constrained later.

Fixes #96036

Jackson 2.15 introduced a (rough) maximum limit on string length. This
commit relaxes that limit to its maximum size, leaving document size
constraints to other existing limits in the system. We can revisit
whether string length within a document should be independently
constrainted later.
@rjernst rjernst added >non-issue :Core/Infra/Core Core issues without another label v8.8.0 v8.9.0 labels May 11, 2023
@rjernst rjernst marked this pull request as ready for review May 11, 2023 13:57
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label May 11, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst
Copy link
Member Author

rjernst commented May 11, 2023

I've marked this as a non-issue because it will target 8.8.0 where this regression was introduced.

@rjernst rjernst requested a review from DaveCTurner May 11, 2023 14:04
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@rjernst rjernst merged commit 1208c02 into elastic:main May 11, 2023
@rjernst rjernst deleted the xcontent/max_string branch May 11, 2023 15:54
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.8

rjernst added a commit to rjernst/elasticsearch that referenced this pull request May 11, 2023
Jackson 2.15 introduced a (rough) maximum limit on string length. This
commit relaxes that limit to its maximum size, leaving document size
constraints to other existing limits in the system. We can revisit
whether string length within a document should be independently
constrainted later.
elasticsearchmachine pushed a commit that referenced this pull request May 11, 2023
Jackson 2.15 introduced a (rough) maximum limit on string length. This
commit relaxes that limit to its maximum size, leaving document size
constraints to other existing limits in the system. We can revisit
whether string length within a document should be independently
constrainted later.
rshen91 added a commit to elastic/kibana that referenced this pull request May 16, 2023
## Summary

Closes #156901,
#156902, and
#156106

As Tim discovered and mentioned in 156901 - these suite of tests were
skipped because of a new limit that was reverted in es
elastic/elasticsearch#96031

Tests pass without any problems now.
jasonrhodes pushed a commit to elastic/kibana that referenced this pull request May 17, 2023
## Summary

Closes #156901,
#156902, and
#156106

As Tim discovered and mentioned in 156901 - these suite of tests were
skipped because of a new limit that was reverted in es
elastic/elasticsearch#96031

Tests pass without any problems now.
@dadoonet
Copy link
Member

dadoonet commented Nov 7, 2023

I'm wondering if this limit is taken into account for the ingest attachment plugin.

According to the discussion here: https://discuss.elastic.co/t/345687 it seems that Jackson is still using its default limit of 5mb (with jackson 2.15.0):

String length (5046272) exceeds the maximum length (5000000)" appears

Do we need to also modify some code in the ingest pipeline plugin?

BTW, Jackson has modified this limit to 20mb in 2.15.1. May be we should upgrade Jackson? 2.15.3 seems to be the latest stable version...

@stu-elastic
Copy link
Contributor

Hi, @dadoonet please cut an issue if this remains a problem.

It's hard for us to track issues if they exist as comments on a PR several months after it's been merged.

stu-elastic added a commit to stu-elastic/elasticsearch that referenced this pull request Jan 4, 2024
Remove the rough limit on string length from Jackson 2.15.
The limit was already relaxed for JSON in elastic#96031, this extends
that change to other XContent types.

Refs: elastic#96031
stu-elastic added a commit that referenced this pull request Jan 8, 2024
Remove the rough limit on string length from Jackson 2.15. The limit was already relaxed for JSON in #96031, this extends that change to other XContent types.

Refs: #96031
Fixes: #104009
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this pull request Jan 8, 2024
Remove the rough limit on string length from Jackson 2.15. The limit was already relaxed for JSON in elastic#96031, this extends that change to other XContent types.

Refs: elastic#96031
Fixes: elastic#104009
(cherry picked from commit a359b1f)
elasticsearchmachine pushed a commit that referenced this pull request Jan 8, 2024
Remove the rough limit on string length from Jackson 2.15. The limit was already relaxed for JSON in #96031, this extends that change to other XContent types.

Refs: #96031
Fixes: #104009
(cherry picked from commit a359b1f)
@Korzi
Copy link

Korzi commented Dec 2, 2024

Was it backported to Elasticsearch 7 as well? Could you please backport it, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.8.0 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-release 8.8.0 cannot work with string fields longer than 5000000 characters
6 participants