>20MB Strings cannot be written (due to a novel Jackson databind default limit)

[criminosis]

This PR was merged starting with Jackson 2.15.0 that added a limit to stream based parsing of JSON elements:

FasterXML/jackson-core#864

Prior to this PR it was unlimited, then was set to 1MB, then bumped to 5MB and then ultimately to 20MB.

It seems numerous other projects have been surprised by this change at least judging by the number of links to other repos throughout that PR.

A request to increase to 1GB (again the original "limit" was it was unlimited) was denied although the maintainer seems to regret putting the default in the first place:

I agree that perhaps this particular limit would have been safest to leave undefined -- in practice there is the 2 gig limit due to String and Java array limit, fwtw -- I would not think change would warrant 3.0 bump in itself... but as we already have 3.0 (master), change to actual limitation definitely could have (in hindsight) waited for that.

This introduces a novel limitation for anyone writing large strings to JanusGraph. One of my tests involves writing a roughly 80MB string and it has succeeded until JanusGraph 1.0.0.

The new limit was introduced by this PR into JG:
ac67b19

While the change in JanusGraph was merged a while ago, June 13th 2023, it only has been released as part of 1.0.0, and was also pack ported into 0.6.4 by this PR so it may not be fully observed unless individuals have updated and tested this scenario.

Unless the JG team wants to incorporate a new config option to set this new max size limit on the ObjectMapper this is a removal of behavior for the latest released versions as I don't believe there's a means to modify this behavior via an environment variable to Jackson, and the code otherwise seems hardcoded to the defaults here.

To restore the prior unlimited behavior would be to do something like this:

public static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(
            new JsonFactoryBuilder()
                    .streamReadConstraints(
                            StreamReadConstraints
                                    .builder()
                                    .maxStringLength(Integer.MAX_VALUE)
                                    .build()
                    ).build()
    );

Suggested here. I'd be happy to put up such a PR, but figured starting a dialog for if that's acceptable made sense to do first.

[criminosis]

Here's how it presents in JanusGraph's logs:

Caused by: org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException: Could not deserialize the JSON value as required. Nested exception: org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException: Could not deserialize the JSON value as required. Nested exception: org.apache.tinkerpop.shaded.jackson.core.exc.StreamConstraintsException: String length (20054016) exceeds the maximum length (20000000)

...logging of my traversal here omitted...
{"@type":"g:Bytecode",""[truncated 84093872 bytes]; line: 1, column: 334813] (through reference chain: java.util.LinkedHashMap["args"])
at org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException.from(JsonMappingException.java:269) ~[gremlin-shaded-3.7.0.jar:3.7.0]
at org.apache.tinkerpop.shaded.jackson.databind.DeserializationContext.mappingException(DeserializationContext.java:2167) ~[gremlin-shaded-3.7.0.jar:3.7.0]
at org.apache.tinkerpop.gremlin.structure.io.graphson.GraphSONTypeDeserializer.deserialize(GraphSONTypeDeserializer.java:194) ~[gremlin-core-3.7.0.jar:3.7.0]
at org.apache.tinkerpop.gremlin.structure.io.graphson.GraphSONTypeDeserializer.deserializeTypedFromAny(GraphSONTypeDeserializer.java:101) ~[gremlin-core-3.7.0.jar:3.7.0]
at org.apache.tinkerpop.shaded.jackson.databind.deser.std.UntypedObjectDeserializer.deserializeWithType(UntypedObjectDeserializer.java:313) ~[gremlin-shaded-3.7.0.jar:3.7.0]
at org.apache.tinkerpop.shaded.jackson.databind.deser.std.MapDeserializer._readAndBindStringKeyMap(MapDeserializer.java:625) ~[gremlin-shaded-3.7.0.jar:3.7.0]
....further steps in the stack trace omitted....

[li-boxuan]

Thank you for the detailed analysis @criminosis ! A PR, with a test that showcases the regression, would be great!

[criminosis]

@li-boxuan Got a draft PR up that demonstrates the regression that I see in my environment

#4260

java.util.concurrent.CompletionException: io.netty.handler.codec.DecoderException: org.apache.tinkerpop.gremlin.util.ser.SerializationException: org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException: Could not deserialize the JSON value as required. Nested exception: org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException: Could not deserialize the JSON value as required. Nested exception: org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException: Could not deserialize the JSON value as required. Nested exception: org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException: Could not deserialize the JSON value as required. Nested exception: org.apache.tinkerpop.shaded.jackson.core.exc.StreamConstraintsException: String length (20051112) exceeds the maximum length (20000000)
at [Source: (byte[])"{"requestId":"5956ab5a-2c25-4b50-9b17-cb09d2ee843e","status":{"message":"","code":200,"attributes":{"@type":"g:Map","@value":["host","/127.0.0.1:56687"]}},"result":{"data":{"@type":"g:List","@value":[{"@type":"g:Traverser","@value":{"bulk":{"@type":"g:Int64","@value":1},"value":{"@type":"g:Vertex","@value":{"id":{"@type":"g:Int64","@value":4216},"label":"largeStringTestLabel","properties":{"largeString":[{"@type":"g:VertexProperty","@value":{"id":{"@type":"janusgraph:RelationIdentifier","@value""[truncated 21000112 bytes]; line: 1, column: 540]
at [Source: (byte[])"{"requestId":"5956ab5a-2c25-4b50-9b17-cb09d2ee843e","status":{"message":"","code":200,"attributes":{"@type":"g:Map","@value":["host","/127.0.0.1:56687"]}},"result":{"data":{"@type":"g:List","@value":[{"@type":"g:Traverser","@value":{"bulk":{"@type":"g:Int64","@value":1},"value":{"@type":"g:Vertex","@value":{"id":{"@type":"g:Int64","@value":4216},"label":"largeStringTestLabel","properties":{"largeString":[{"@type":"g:VertexProperty","@value":{"id":{"@type":"janusgraph:RelationIdentifier","@value""[truncated 21000112 bytes]; line: 1, column: 540]
at [Source: (byte[])"{"requestId":"5956ab5a-2c25-4b50-9b17-cb09d2ee843e","status":{"message":"","code":200,"attributes":{"@type":"g:Map","@value":["host","/127.0.0.1:56687"]}},"result":{"data":{"@type":"g:List","@value":[{"@type":"g:Traverser","@value":{"bulk":{"@type":"g:Int64","@value":1},"value":{"@type":"g:Vertex","@value":{"id":{"@type":"g:Int64","@value":4216},"label":"largeStringTestLabel","properties":{"largeString":[{"@type":"g:VertexProperty","@value":{"id":{"@type":"janusgraph:RelationIdentifier","@value""[truncated 21000112 bytes]; line: 1, column: 540]
at [Source: (byte[])"{"requestId":"5956ab5a-2c25-4b50-9b17-cb09d2ee843e","status":{"message":"","code":200,"attributes":{"@type":"g:Map","@value":["host","/127.0.0.1:56687"]}},"result":{"data":{"@type":"g:List","@value":[{"@type":"g:Traverser","@value":{"bulk":{"@type":"g:Int64","@value":1},"value":{"@type":"g:Vertex","@value":{"id":{"@type":"g:Int64","@value":4216},"label":"largeStringTestLabel","properties":{"largeString":[{"@type":"g:VertexProperty","@value":{"id":{"@type":"janusgraph:RelationIdentifier","@value""[truncated 21000112 bytes]; line: 1, column: 540] (through reference chain: java.util.LinkedHashMap["result"]->java.util.LinkedHashMap["data"])
...and so forth

From following around in git blames I discovered it seems Tinkerpop intends for for these to be configurable when they updated, I was just searching in JanusGraph code earlier for hits on ObjectMapper, but judging by the multiple nestings in the error message it seems like even with the setting being applied on the serializer something is eventually getting landed on that bounces back with the default that Tinkerpop prescribed. Any thoughts @li-boxuan?

[li-boxuan]

Aha, sorry I missed that notice for TinkerPop release. I read your original stacktrace again and it seems the exception is thrown by TinkerPop. I guess someone on TinkerPop discord channel might be more knowledgable to answer that.

[criminosis]

@li-boxuan I pulled down the tinkerpop code and replicated my test. I was suspicious of these two lines because they weren't propagating the stream read constants behind the scenes of the object mapper they were based off of:

https://github.com/apache/tinkerpop/blob/55a7b7f25677c38ad23a813d19dfb58d69954d35/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONTypeDeserializer.java#L108-L109

However the test ended up passing. It was then I realized my oversight was in JG's test. Namely you can specify a different max string length in the resource file, but that resource file doesn't seem to have any bearing on the traversal being instantiated for the test case: https://github.com/JanusGraph/janusgraph/blob/master/janusgraph-server/src/test/java/org/janusgraph/graphdb/tinkerpop/gremlin/server/io/JanusGraphSONModuleIT.java#L30

So long story short the end result to resolve my issue was to create a copy of the default server config file but with the serializers un-commented out and specifying maxStringLength: 2147483647 for org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV3 and org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV2 in order to restore the pre-Tinkerpop 3.7.0 & pre-Jackson data-bind 2.15.0 behavior with regards to string length.

Got me wondering if it might be worth an entry on the JG docs website? Either how to restore the prior behavior, set these new stream reader limits, or even about these channel serializers in general? Maybe a section in the advanced janusgraph server configurations?

[li-boxuan]

Hey great to see you have figured out the resolution. Documenting that on the advanced janusgraph server configurations section would be awesome. Ideally we also need to include a note in https://docs.janusgraph.org/changelog/.

Btw, regarding your original PR, now that you figured out the problem with the test setup, would you be able to continue on that PR? This is absolutely not required, I just feel that you have already made some efforts, and it would be nice to have a test that validates the behavior.