Increase default max allowed String value length from 5 megs to 20 megs

[cowtowncoder]

(note: possible follow-up to #959)

Based on user/dev feedback wrt 2.15.0 release, there seem to be use cases where JSON String values (note: NOT Document size) exceed 5 million characters. While this is configurable limit it seems best to try to change default to be yet more lenient, given that this is not the likeliest way to try to cause DoS. It is also the case that those who do wish to use more modest sizes will anyway need to lower limits, regardless of defaults we choose.

My initial thinking is to increase this further to 20 million (giving max memory usage of about 40 megs in JVM -- although temporarily up to twice that due to segmented storage), for Jackson 2.15.1.

[gdimitrov7]

We hit the same issue in our product. 20M should be much better and less intrusive and I fully support it.

Q1: Why there is a limit for the max String value? I mean I understand the other limits - maxDepth, maxNumLen - there are performance penalties for them. In the same I do not see exponential increase for the String values length. And yes, there can be OOM but most apps limit the input to be processed - for example on HTTP request body length. Example: Our server has 2GiB heap with max request body 20M - it does not matter if we have JSON with:

• 2000 fields by 10k chars each (~20M)
• or 1 field by 20M chars (~20M)

in both cases it should take 2x40M bytes to process the request and parse the body, meaning that we can process ~25 request in the same (if there is no other mem consumed).

I do not say we have to remove the limit, but can it be much bigger by default?

Q2: Is it possible to read the default max from system property?

   public static final int DEFAULT_MAX_STRING_LEN =  
              Integer.getInteger("com.fasterxml.jackson.core.StreamReadConstraints.maxStringLen", 20_000_000);

It will be much easy to be overwritten by apps that need bigger limit by some valid reasons.

[cowtowncoder]

Yeah, String value length limit is sort of inspired by number value limit; but practical effects are quite different. In theory I think that if limits were included from beginning they wouldn't have been problematic; going from unlimited to limit is the issue.
On multiple vs individual fields; this is where maximum doc length limit (not yet implemented) would be needed anyway.

As to system properties: for libraries like Jackson, which may be used by multiple other things, frameworks, concurrently, system-wide settings do not make sense in my opinion (system properties essentially being mutable singletons).

Be that as it may, Jackson does not use system properties for anything and my view is that while I do not object to their use, it should occur via extension modules (although in this particular case that'd be difficult to apply).

[gdimitrov7]

I understand that system properties are not used in Jackson. In the same time they have place in other libraries like logback or even in the JDK.

I agree they are mutable singletons but in cases like this - enforcing new limits - they provide easy way to change to custom bigger value at runtime with just restarting the application and with no code changes and no new version. And provides much easy support when we have issue at the customer environment.

[pjfanning]

I suggested before the idea of having static methods on StreamReadConstraints that can be used to change the settings for the default StreamReadConstraints. I think this is a viable solution with one downside - some rogue lib maintainer could set values using this approach and that affects any other code running using that ClassLoader. The static methods would ideally only be used by users providing the actual services.

Most users should strongly consider creating their own factory methods for creating ObjectMapper instances - so that they can centralise their preferred config settings.

[cowtowncoder]

Ok. So, although I have been resistant to ideas like what @pjfanning proposed, I am starting to think about this bit more. And I prefer static setter methods over system properties, as mechanism jackson-core could provide; and then allow something else to offer system properties mechanism if that is deemed useful.

As long as static setter method was properly documented with all the caveat and warnings -- basically stating that it is not recommended to be used except by end-user application; and that there are problems with stateful global singletons in general -- I think I'd be +1 for adding such methods in 2.16.
It seems like one of those things that while not generally recommended, could solve some actual real-world issues.

[pjfanning]

@cowtowncoder the static methods are most useful for v2.15.1 - to make it easier for people who are migrating now. They won't be very useful if left till v2.16.0.

I've raised issues in a number of projects (libs and apps) that use Jackson and there hasn't been much interest in them supporting allowing their users to configure StreamReadConstraints.

I think we are stuck with having to support a mechanism that allows users to modify the default values that StreamReadConstraints applies when users don't set their own via creating their own JsonFactory with their own StreamReadConstraints instance attached.