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.

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

Allow override of StreamReadContraints default with overrideDefaultStreamReadConstraints() #1019

Merged

Conversation

pjfanning
Copy link
Member

if this approach is acceptable, I can extend this PR with some tests

@pjfanning pjfanning marked this pull request as draft May 13, 2023 20:30
@cowtowncoder
Copy link
Member

As per my other note, I don't want to make API changes in 2.15. Will release 2.15.1 soon; this PR may be left as fallback in case there really is enough push back to require addition (i.e. I can still consider change, 2.15.2 or 2.15.1 makes little difference semantically).

@darrenoc3
Copy link

darrenoc3 commented May 16, 2023

@cowtowncoder I am just one person, but consider this my pushback: it's extremely inconvenient for legacy projects that are already handling strings* >5MB or greater in production to have no mechanism to configure the default globally. You can see another example in Github where Palantir have had to jump through hoops and use Reflection to override the default in their codebase.

Originally I wrote "documents" but I meant "strings". It just so happens that in our largest documents, string size is equivalent to document size

@cowtowncoder
Copy link
Member

@darrenoc3 Documents can be as big as necessary, 5 MB is for String values, not documents (currently no limit although we'll likely introduce mechanism for enabling one).
I am also bit surprised about P as a company since I thought we had feedback from them during pre-release phase suggesting 5 meg limit was actually fine for them in particular (I may have mistaken company with another one tho).

But fwtw as per #1014 limit will be bit higher (20 MB) in 2.15.1, to be released today.

@pjfanning
Copy link
Member Author

@cowtowncoder I think this is the Palantir change - palantir/conjure-java-runtime#2601 - they are using the 2.15.0 API but setting a 50Mb per string limit, just using Reflection because they want to support jackson 2.14 too.

They seem to have control over the ObjectMappers that get created. Many users do not - they get them created via 3rd party libs - and there is no rush by 3rd party lib maintainers to allow users to configure the ObjectMappers that the 3rd party libs create.

@cowtowncoder
Copy link
Member

I am coming around to thinking this would make sense for 2.15.2. I'll think about it bit more but will probably merge it.

@cowtowncoder
Copy link
Member

@pjfanning Is this ready to be merged? Only asking as it's still in Draft stage.

@pjfanning pjfanning changed the title [DRAFT] allow override of StreamReadContraints default allow override of StreamReadContraints default May 21, 2023
@pjfanning pjfanning marked this pull request as ready for review May 21, 2023 04:55
@pjfanning
Copy link
Member Author

@cowtowncoder this is ready to merge

@cowtowncoder cowtowncoder changed the title allow override of StreamReadContraints default Allow override of StreamReadContraints default May 22, 2023
@cowtowncoder cowtowncoder changed the title Allow override of StreamReadContraints default Allow override of StreamReadContraints default with overrideDefaultStreamReadConstraints() May 22, 2023
@cowtowncoder cowtowncoder merged commit 12824dd into FasterXML:2.15 May 22, 2023
cowtowncoder added a commit that referenced this pull request May 22, 2023
@pjfanning pjfanning deleted the default-streamreadconstraints-override branch July 8, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants