Non-generic interface method hides type resolution info from generic base class

[tbartley]

I saw this issue via MongoJack and haven't yet been able to construct a pure jackson databind test case but it seems that in some circumstances a class definition like the following:

public interface I {
   String getT();
}

public class A<T> {
    private T t;
    public A(T t) { this.t = t; }
    public T getT() { return t; }
}

public class B extends A<String> implements I {
    public B(String t) { super(t); }
}

can result in the type information required to resolve the type of t to a String is lost.

This happens because in AnnotatedClass._addMemberMethods is this code:

                /* 06-Jan-2010, tatu: [JACKSON-450] Except that if method we saw first is
                 *   from an interface, and we now find a non-interface definition, we should
                 *   use this method, but with combination of annotations.
                 *   This helps (or rather, is essential) with JAXB annotations and
                 *   may also result in faster method calls (interface calls are slightly
                 *   costlier than regular method calls)
                 */
                if (old.getDeclaringClass().isInterface() && !m.getDeclaringClass().isInterface()) {
                    methods.add(old.withMethod(m));
                }

In my test, the interface getter is discovered first and so plays the role of "old", then the base class getter is being added and so the original definintion of the method is simply adjusted with a reference to the method being processed. However the method from the interface has no type resolution information since the interface is not defined with generics so the type resolution information is lost and thus t can end up being serialialised as Object instead of String and so a serialisation failure occurs.

I implemented a fix as follows. To AnnotatedMethod add the method:

    public AnnotatedMethod withContextAndMethod(TypeResolutionContext ctxt, Method m) {
        return new AnnotatedMethod(ctxt, m, _annotations, _paramAnnotations);
    }

and then change the code in AnnotatedClass to:

                if (old.getDeclaringClass().isInterface() && !m.getDeclaringClass().isInterface()) {
                    methods.add(old.withContextAndMethod(typeContext, m));
                }

which resolved my problem. Happy to submit a pull request, but I haven't got the CLA ready yet and I would also like some help constructing a pure Jackson unit test to replicate the issue.

To be specific about my MongoJack reproducer:

public interface I {
   String getT();
}

public class A<T> {
    private T t;
    public A(T t) { this.t = t; }
    public T getT() { return t; }
}

public class B extends A<String> implements I {
    public B(String t) { super(t); }
}

@Test
public void test() {
		ObjectMapper om = new ObjectMapper();
		MongoJackModule.configure(om);
		DBQuery.Query query = DBQuery.is("_id", "t");

		JavaType bType = om.getTypeFactory().constructType(B.class);
		DBObject serialized = SerializationUtils.serializeQuery(om, bType, query);
		System.out.println(serialized);
}

Prints { "_id": "t" } as expected with my fix and throws this exception without it:

org.mongojack.MongoJsonMappingException: Error serializing value t in DBQuery operation _id
	at org.mongojack.internal.util.SerializationUtils.serializeQueryField(SerializationUtils.java:266)
	at org.mongojack.internal.util.SerializationUtils.serializeQueryCondition(SerializationUtils.java:182)
	at org.mongojack.internal.util.SerializationUtils.serializeQuery(SerializationUtils.java:155)
	at org.mongojack.internal.util.SerializationUtils.serializeQuery(SerializationUtils.java:143)
	at bugs.jackson.selfpolymorph.test(selfpolymorph.java:69)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:678)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)
Caused by: com.fasterxml.jackson.databind.JsonMappingException: No serializer found for class java.lang.String and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS)
	at com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:284)
	at com.fasterxml.jackson.databind.SerializerProvider.mappingException(SerializerProvider.java:1110)
	at com.fasterxml.jackson.databind.SerializerProvider.reportMappingProblem(SerializerProvider.java:1135)
	at com.fasterxml.jackson.databind.ser.impl.UnknownSerializer.failForEmpty(UnknownSerializer.java:69)
	at com.fasterxml.jackson.databind.ser.impl.UnknownSerializer.serialize(UnknownSerializer.java:32)
	at org.mongojack.internal.util.SerializationUtils.serializeQueryField(SerializationUtils.java:264)
	... 27 more

My testing has been using the 2.8 branch of jackson databind.

[cowtowncoder]

Ok. First of all, thank you for reporting this.

I wouldn't be surprised if there were cases where refinement could lead to loss of typing, but should not.
Your example declaration looks a good start for trying to come up with a failing test case.
I am not so sure about the fix, although it too could probably help here. One concern I have is that specific logic wrt interface/concrete class may not be required or exact rule, so it'd be good to have sort of counter cases. But if and when existing tests pass, that is at least some verification that change wouldn't be very risky.

But one thing that is needed is to base this off of 2.9(.0.pr4). There have been quite significant changes in handling, and it is likely that fixes may only be safe enough to make for 2.9 at this point (since 2.8 patches extend to quite far, and once 2.9 is out, specifically, I want to minimize risk for 2.8 patches... have had some problems in this area lately :) ).

So would it be possible to quickly verify that patch also works for 2.9.0.pr4? I hope to look into this, too, but I do have quite full a plate unfortunately.

[tbartley]

Sure thing. If there's been significant change, I'll first verify it's still causing me an issue with 2.9. And then if the fix still applies. FWIW The 2.8 test suite passes with my change.

[tbartley]

The same issue is present in 2.9.0.pr4. Looking now to see whether/where I can apply the fix.

[tbartley]

Fix is now in AnnotatedMethodCollector:

                } else if (Modifier.isAbstract(old.getModifiers())
                        && !Modifier.isAbstract(m.getModifiers())) {
                    // 06-Jan-2010, tatu: Except that if method we saw first is
                    // from an interface, and we now find a non-interface definition, we should
                    //   use this method, but with combination of annotations.
                    //   This helps (or rather, is essential) with JAXB annotations and
                    //   may also result in faster method calls (interface calls are slightly
                    //   costlier than regular method calls)
                    b.method = m;
              }

becomes:

                } else if (Modifier.isAbstract(old.getModifiers())
                        && !Modifier.isAbstract(m.getModifiers())) {
                    // 06-Jan-2010, tatu: Except that if method we saw first is
                    // from an interface, and we now find a non-interface definition, we should
                    //   use this method, but with combination of annotations.
                    //   This helps (or rather, is essential) with JAXB annotations and
                    //   may also result in faster method calls (interface calls are slightly
                    //   costlier than regular method calls)
                    b.method = m;
                    b.typeContext = tc;
              }

requiring typeContext to be non-final in MethodBuilder.

The test suite continues to pass with this change.

[cowtowncoder]

Seems reasonable, finally found time to add. Simplified test did not unfortunately verify the fail (probably since String is "natural" type, binds from java.lang.Object), so would be great to have a reproduction. But it seems like a proper fix.
Included in 2.9 for 2.9.1.

[tbartley]

Thanks Tatu, Yeah I was struggling with a test case as you know. Cheers, Tim On Aug 23, 2017, at 1:48 PM, Tatu Saloranta <notifications@github.com<mailto:notifications@github.com>> wrote: Seems reasonable, finally found time to add. Simplified test did not unfortunately verify the fail (probably since String is "natural" type, binds from java.lang.Object), so would be great to have a reproduction. But it seems like a proper fix. Included in 2.9 for 2.9.1. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#1705 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ADMGnHN6JeHTO5KhNcZvFzHwe_ISXJJHks5sbLo7gaJpZM4OaJ-B>.

[cowtowncoder]

@tbartley yes no problem, getting the fix is still most important here.

[sjz]

Thanks for this @cowtowncoder, @tbartley

Came across this issue today.
Not sure if it was intentional - the line that fixes the issue, as I am experiencing it, was commented out. Comment here: 48fc70b#commitcomment-24618605

[daveatclearbox]

Following on from sjz's comment above I have added a failing test to the commit comment referenced above.

[cowtowncoder]

As per comment on commit, a new issue would be most welcome, and so I can fix it.
Failing test is great; not sure how I managed to leave commented out fix in there.

[cowtowncoder]

Actually, since this was in 2.9.1, I can re-open, re-fix it without too much confusion...