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

StackOverflowError with contentConverter that returns array type #357

Closed
fschopp opened this issue Dec 2, 2013 · 14 comments
Closed

StackOverflowError with contentConverter that returns array type #357

fschopp opened this issue Dec 2, 2013 · 14 comments
Milestone

Comments

@fschopp
Copy link

fschopp commented Dec 2, 2013

Jackson version: 2.3.0

The following code snippet

static class A { }

static class B {
    @JsonSerialize(contentConverter = AToStringConverter.class)
    public List<A> list = Arrays.asList(new A());
}

static class AToStringConverter extends StdConverter<A, List<String>> {
    @Override
    public List<String> convert(A value) {
        return Arrays.asList("Hello world.");
    }
}

@Test
void testBSerialization() throws Exception {
    new ObjectMapper().writeValueAsString(new B());
}

produces the following endless recursion:

[...]
at com.fasterxml.jackson.databind.ser.std.StdSerializer.findConvertingContentSerializer(StdSerializer.java:252)
at com.fasterxml.jackson.databind.ser.impl.StringCollectionSerializer.createContextual(StringCollectionSerializer.java:91)
at com.fasterxml.jackson.databind.SerializerProvider.handleSecondaryContextualization(SerializerProvider.java:835)
at com.fasterxml.jackson.databind.SerializerProvider.findValueSerializer(SerializerProvider.java:503)
at com.fasterxml.jackson.databind.ser.std.StdSerializer.findConvertingContentSerializer(StdSerializer.java:252)
at com.fasterxml.jackson.databind.ser.impl.StringCollectionSerializer.createContextual(StringCollectionSerializer.java:91)
at com.fasterxml.jackson.databind.SerializerProvider.handleSecondaryContextualization(SerializerProvider.java:835)
at com.fasterxml.jackson.databind.SerializerProvider.findValueSerializer(SerializerProvider.java:503)
[...]

It seems that the @JsonSerialize annotation is applied both before and after the conversion.

@fschopp
Copy link
Author

fschopp commented Dec 3, 2013

Found two issues that are related in the sense that they also pertain to serializing statically typed arrays: #358 and #359. Looks like a minefield to me... :)

@cowtowncoder
Copy link
Member

Your guess on cause is probably correct: annotation handling is difficult because it is difficult to figure out exact applicability when a chain of conversions (of handlers) is executed.
I hope to find out how this can be resolved.

In the meantime I think the safest work-around actually involves use of JsonNode (or specifically ArrayNode) since those are not generic types, nor use contextual variations.

@cowtowncoder
Copy link
Member

I can reproduce this successfully with sample code.

@fschopp
Copy link
Author

fschopp commented Dec 4, 2013

As part of the fix, I suggest to add a comment to the JavaDoc about the precedence of the various @JsonSerialize properties. For instance, if both a serializer and a converter are given, what is the specified behavior? Possible options include:

  • first the converter is applied, and then the using property overrides the serializer of the conversion result type
  • converter is used, and as is ignored
  • as is used, and converter is ignored

The JavaDoc already has some comments in that direction (e.g., it mentions that "some properties block others"), but it would be great to be more specific.

@cowtowncoder
Copy link
Member

Right, this is a case where options overlap. My thinking is that if en explicit serializer (using etc) is specified, that is to be used as-is, and converter to be ignored. Type override is still used, although it does not necessarily matter.
Only if no explicit serializer is specific is converter considered.

Question of as is interesting one; my gut feeling is that since it specifies input type (for converter), it should not have much if any effect -- converter's output type is what should be used for locating serializer to use.
But things get even more complicated with structured types; so perhaps it would be best to state that none of type modifiers are to be used with converters.

cowtowncoder added a commit that referenced this issue Oct 19, 2014
@cowtowncoder cowtowncoder modified the milestones: 2.2, 2.7.0 Nov 5, 2015
@cowtowncoder cowtowncoder reopened this Apr 14, 2016
@cowtowncoder
Copy link
Member

Alas: it looks like my fix did not fully work; it just happened to add a different flaw that did fix converter problem, but caused other issues. Essentially removal of lock failed, and so lock lasted until end of current processing (writeValue()). I am not fixing the problem with mechanism locking uses, and once lock lifecycle works as intended, the original problem reappears.

So it's back to square one, with 2.7.4. :-(

@nazymko
Copy link

nazymko commented Apr 26, 2016

Hello,
Is this bug fixed in 2.7 ?

@cowtowncoder
Copy link
Member

@nazymko No, unfortunately there is no known working fix. Earlier attempt did hide the problem, but in a way that broke any nested resolution (or one that follows in document order, even if not nested).

@cowtowncoder cowtowncoder removed this from the 2.7.0 milestone Jul 5, 2016
@cowtowncoder cowtowncoder added this to the 2.9.0 milestone Dec 10, 2016
@cowtowncoder cowtowncoder reopened this Dec 10, 2016
@TuomasKiviaho
Copy link

Caused by: java.lang.StackOverflowError
	at com.fasterxml.jackson.databind.SerializerProvider.findValueSerializer(SerializerProvider.java:601)
	at com.fasterxml.jackson.databind.ser.std.StdDelegatingSerializer.createContextual(StdDelegatingSerializer.java:121)
	at com.fasterxml.jackson.databind.SerializerProvider.handleSecondaryContextualization(SerializerProvider.java:1001)
	at com.fasterxml.jackson.databind.ser.std.StdDelegatingSerializer.createContextual(StdDelegatingSerializer.java:125)
	at com.fasterxml.jackson.databind.SerializerProvider.handleSecondaryContextualization(SerializerProvider.java:1001)
	at com.fasterxml.jackson.databind.ser.std.StdDelegatingSerializer.createContextual(StdDelegatingSerializer.java:125)

I think that StdDelegatingSerializer was forgotten from 9434f51

    @Override
    public JsonSerializer<?> createContextual(SerializerProvider provider, BeanProperty property)
        throws JsonMappingException
    {
        JsonSerializer<?> delSer = _delegateSerializer;
        JavaType delegateType = _delegateType;

        if (delSer == null) {
            // Otherwise, need to locate serializer to delegate to. For that we need type information...
            if (delegateType == null) {
                delegateType = _converter.getOutputType(provider.getTypeFactory());
            }
            // 02-Apr-2015, tatu: For "dynamic case", where type is only specified as
            //    java.lang.Object (or missing generic), [databind#731]
            if (!delegateType.isJavaLangObject()) {
                delSer = provider.findValueSerializer(delegateType);
            }
        }
        if (delSer instanceof ContextualSerializer) {
            delSer = provider.handleSecondaryContextualization(delSer, property);
        }
        if (delSer == _delegateSerializer && delegateType == _delegateType) {
            return this;
        }
        return withDelegate(_converter, delegateType, delSer);
    }

The handleSecondaryContextualization kicks in every time in this method and the commit referenced above tries to avoid just kind of situation. I got this with my introspector tweak #1584 in but I think that this could potentially pop up without it.

@cowtowncoder
Copy link
Member

@TuomasKiviaho is that with the original code here, or something else? Test case I had was slightly simplified but it passes. I don't doubt there's a remaining problem, just want to reproduce it -- the whole contextualization is tricky to get right, esp. for content (de)serializers.

@cowtowncoder
Copy link
Member

Original test as-is seems to pass, and test does use StdDelegatingSerializer, for what it is worth.

@TuomasKiviaho
Copy link

I don't doubt there's a remaining problem, just want to reproduce it

This should be reproducible by having StdDelegatingSerializer internal state _delegateSerializer as null and then calling the createContextual method shown above. This state can be achieved with the currently available constructors.

delSer = provider.findValueSerializer(delegateType); seems to return in my case the same StdDelegatingSerializerinstance that I'm already in thus looping delSer = provider.handleSecondaryContextualization(delSer, property); forever.

@cowtowncoder
Copy link
Member

What I mean is a unit test, both as demonstration and to guard against regression, using actual API or declarations to get there.

@cowtowncoder cowtowncoder reopened this Mar 31, 2017
@cowtowncoder
Copy link
Member

At this point, can not reproduce, so closing. I am not arguing that this does not occur, simply that I have no way of triggering this with actual usage and as such do not trust that tests that manipulate internal state would faithfully reproduce problem users might encountered (or, more importantly, verify that fix is appropriate).

A new issue should be filed with reproduction if this is encountered (with ref back to this as background) in wild.

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

No branches or pull requests

4 participants