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 in Dynamic StdKeySerializer #1679

Closed
kpronovost opened this issue Jun 26, 2017 · 8 comments
Closed

StackOverflowError in Dynamic StdKeySerializer #1679

kpronovost opened this issue Jun 26, 2017 · 8 comments
Milestone

Comments

@kpronovost
Copy link

kpronovost commented Jun 26, 2017

There seem to be a problem (checked and doesn't seem to be fixed in latest version) with the serialize method of the Dynamic static class of the StdKeySerializer.

        @Override
        public void serialize(Object value, JsonGenerator g, SerializerProvider provider)
                throws IOException
        {
            Class<?> cls = value.getClass();
            PropertySerializerMap m = _dynamicSerializers;
            JsonSerializer<Object> ser = m.serializerFor(cls);
            if (ser == null) {
                ser = _findAndAddDynamic(m, cls, provider);
            }
            ser.serialize(value, g, provider);
        }

The problem comes from the fact that when ser is null, the new ser returned by _findAndAddDynamic is incorrectly filled.

        protected JsonSerializer<Object> _findAndAddDynamic(PropertySerializerMap map,
                Class<?> type, SerializerProvider provider) throws JsonMappingException
        {
            PropertySerializerMap.SerializerAndMapResult result =
                    // null -> for now we won't keep ref or pass BeanProperty; could change
                    map.findAndAddKeySerializer(type, provider, null);
            // did we get a new map of serializers? If so, start using it
            if (map != result.map) {
                _dynamicSerializers = result.map;
            }
            return result.serializer;
        }

So say we are in ser#1, ser#1._dynamicSerializers now has the correct PropertySerializerMap$Single. However, result.serializer._dynamicSerializers has PropertySerializerMap$Empty.
Therefore, a new call with that result ser#2 is made which ends up creating an infinite loop.

Possible fix:

  • replace _dynamicSerializers = result.map; by result.serializer._dynamicSerializers = result.map

If I'm mistaken please let me know, but It seems obvious when debugging that something's is not working as intended

@cowtowncoder
Copy link
Member

As usual, what would really help beyond description (which is nicely detailed, thank you) would be a simple reproduction (test). This would help a lot against regression, as well as fix verification.
I can then verify suggested solution, against usage of PropertySerializerMap elsewhere; I forget what the intended pattern was (mostly is work-around wrt Java's lack of tuple/pair class, if my memory serves me).

@kpronovost
Copy link
Author

kpronovost commented Jun 27, 2017

Thanks, sorry, for lack of sample, was in a rush end of day, here's the simplest code that creates the stack:

import java.util.HashMap;
import java.util.Map;
import org.junit.Test;
import com.fasterxml.jackson.databind.ObjectMapper;

public class ApplicationTest {  
  @Test
  public void StdKeySerializerSampleTest() {
    ObjectMapper objectMapper = new ObjectMapper();
    
    Map<Object, Object> objectMap = new HashMap<Object, Object>();
    objectMap.put(new Object(), new Object());
    
    objectMapper.convertValue(objectMap, String.class);
  }
}

Note that while this code does not make that much sense, it was in the idea of simplifying as much as possible. It should still not create a stackOverflow

@cowtowncoder
Copy link
Member

@kpronovost Thank you!

@cowtowncoder
Copy link
Member

Interesting. Yes, that is gnarly one indeed.

@cowtowncoder
Copy link
Member

This may be due to the problem that recursive definitions are not really handled for key serializers, unlike regular value serializers -- there is special handling during serializer construction to catch this case.
For POJOs this is done via ResolvableSerializer (to a degree), and by context keeping track of (de)serializers that are being constructed, as linkage needs to be established before full contextualization. Effectively what needs to happen here is that serializer has itself as the delegate to call for contained values. Question is just that of how....
I don't think this should (ideally) be done by trying to change PropertySerializerMap, but I don't yet have any alternative good solution either.

@kpronovost
Copy link
Author

Thanks for the input.
Although this is indeed a bug, it only happens with test data generated through PodamFactory, that fills in the map with new Objects. In my application, key is always typed, and therefore resolve without any issue (well I think that's why) to my Pojos. I will work around it in tests for the moment.

cowtowncoder added a commit that referenced this issue Jun 27, 2017
@cowtowncoder
Copy link
Member

@kpronovost I think I can figure out this case, but it probably does make sense to use a work-around in the meantime. But I think it's good to fix this as the reproduction looks like something that would occur quite easily.

@kpronovost
Copy link
Author

kpronovost commented Jun 27, 2017

Thanks for the quick reply.

I already patched with realistic data in all test cases, I linked this issue to our Merge Request and will remove unneeded patch once this is resolved in a later version.

TLDR: Thanks for the fix; not a blocker.

@cowtowncoder cowtowncoder changed the title StackOverflowError in Dynamic StdKeySerializer 2.8.8 StackOverflowError in Dynamic StdKeySerializer Jun 27, 2017
@cowtowncoder cowtowncoder added this to the 2.8.10 milestone Jun 27, 2017
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

2 participants