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

IncompatibleClassChangeError deserializing interface methods with default impl #30

Closed
shawnsmith opened this issue Sep 13, 2017 · 7 comments
Milestone

Comments

@shawnsmith
Copy link

shawnsmith commented Sep 13, 2017

With Afterburner I'm getting the exception java.lang.IncompatibleClassChangeError: Found class <MyClass>, but interface was expected trying to deserialize an object where a setter is implemented in an interface.

I've tested with Jackson+Afterburner 2.8.7, 2.8.10 and 2.9.0 and all fail in the same way. (jdk 8)

java.lang.IncompatibleClassChangeError: Found class com.fasterxml.jackson.module.afterburner.deser.TestInterfaceDeser$Model, but interface was expected

	at com.fasterxml.jackson.module.afterburner.deser.TestInterfaceDeser$Model$Access4JacksonDeserializer5b28f286.stringSetter(com/fasterxml/jackson/module/afterburner/deser/TestInterfaceDeser$Model$Access4JacksonDeserializer.java)
	at com.fasterxml.jackson.module.afterburner.deser.SettableStringMethodProperty.deserializeAndSet(SettableStringMethodProperty.java:53)
	at com.fasterxml.jackson.module.afterburner.deser.SuperSonicBeanDeserializer.deserialize(SuperSonicBeanDeserializer.java:159)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4001)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2992)
	at com.fasterxml.jackson.module.afterburner.deser.testInterfaceDeserialize(TestInterfaceDeser.java:40)

Here's a test case that reproduces the issue:

package com.fasterxml.jackson.module.afterburner.deser;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.module.afterburner.AfterburnerModule;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;

public class TestInterfaceDeser {

    public interface Typed {
        String getType();
        default void setType(String type) {
            if (!getType().equals(type)) {
                throw new IllegalStateException(String.format("Expected '%s': %s", getType(), type));
            }
        }
    }

    public static class Model implements Typed {
        @Override
        public String getType() {
            return "Model";
        }
    }

    @Test
    public void testInterfaceDeserialize() throws Exception {
        Model model = new Model();

        ObjectMapper mapper = new ObjectMapper().registerModule(new AfterburnerModule());

        String json = mapper.writeValueAsString(model);
        assertEquals("{\"type\":\"Model\"}", json);

        // Throws: java.lang.IncompatibleClassChangeError: Found class com.fasterxml.jackson.module.afterburner.deser. TestInterfaceDeser$Model, but interface was expected
        Model roundTrip = mapper.readValue(json, Model.class);

        assertNotNull(roundTrip);
    }
}
@cowtowncoder
Copy link
Member

Try not using default methods? I can not unfortunately use this reproduction as Jackson 2.x does not require Java 8 and must build on Java 7.

@shawnsmith
Copy link
Author

I've worked around it for now by specifying a custom serializer which makes Afterburner skip the property:

@JsonDeserialize(using = PlainStringDeserializer.class)
default void setType(String type) {
   ...
}

and

public static class PlainStringDeserializer extends StdDeserializer<String> {
    public PlainStringDeserializer() {
        super(String.class);
    }

    @Override
    public String deserialize(JsonParser jp, DeserializationContext ctx) throws IOException {
        if (jp.getCurrentToken() != JsonToken.VALUE_STRING) {
            return (String) ctx.handleUnexpectedToken(String.class, jp);
        }
        return jp.getText();
    }
}

Specifying the regular com.fasterxml.jackson.databind.deser.std.StringDeserializer wasn't enough, it looks like it has to be a non-default serializer.

@cowtowncoder
Copy link
Member

I think this needs to be tackled via Jackson 3.x version of project, to verify handling of default implementations. But fix itself can I hope be backported in 2.9.x.
I wonder if version of asm library might matter as well; while we are using a relatively recent version maybe we need newer.

@shawnsmith
Copy link
Author

It looks like a fix along the following lines might work without requiring an asm upgrade:

In PropertyMutatorCollector.java replace:

boolean isInterface = method.getDeclaringClass().isInterface();
mv.visitMethodInsn(isInterface ? INVOKEINTERFACE : INVOKEVIRTUAL, ...);

with

boolean isInterface = isInterfaceMethod(method);
mv.visitMethodInsn(isInterface ? INVOKEINTERFACE : INVOKEVIRTUAL, ...);

and add

public class DynamicPropertyAccessorBase
{
...
    protected static boolean isInterfaceMethod(Method method) {
        // Forward compatible with Java 8 equivalent: method.getDeclaringClass().isInterface() && !method.isDefault()
        return method.getDeclaringClass().isInterface() &&
                (method.getModifiers() & (Modifier.ABSTRACT | Modifier.PUBLIC | Modifier.STATIC)) != Modifier.PUBLIC;
    }
}

PropertyAccessorCollector.java has a similar but slightly different pattern for calls to visitMethodInsn(), I haven't done any tests to see if it requires the same fix.

The implementation for isInterfaceMethod() above was derived from inlining Method.isDefault() from JDK 8 and simplifying the result.

@cowtowncoder
Copy link
Member

Interesting. That could work... Thank you for doing this research; I have been swamped with other things and have limited time to use here, so having a solution speeds fixing up a lot.

But so basically fundamental problem is that although method is defined in an interface, it is "materialized" as regular class method. And certainly seems to be missing ACC_ABSTRACT.
I think that's only thing needed to checked here; static flag should have removed method from being considered already, and I think all methods are public in interfaces.

@cowtowncoder cowtowncoder changed the title IncompatibleClassChangeError deserializing interface methods IncompatibleClassChangeError deserializing interface methods with default impl Sep 15, 2017
@cowtowncoder cowtowncoder changed the title IncompatibleClassChangeError deserializing interface methods with default impl IncompatibleClassChangeError deserializing interface methods with default impl Sep 15, 2017
@cowtowncoder cowtowncoder modified the milestones: 2.8.0, 2.8.11 Sep 15, 2017
@cowtowncoder
Copy link
Member

Will be fixed in 2.8.11 (if ever released); 2.9.2.

Oddly enough only seems to affect deserialization; serialization seemed ok.
Probably something similar may need to be tackled with mr Bean too.

@shawnsmith
Copy link
Author

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants