Skip to content

Conversation

robbytx
Copy link
Contributor

@robbytx robbytx commented Sep 17, 2020

Problem

Java 5 supports covariant return types, which if overridden cause the compiler to generate "bridge" methods: https://stackoverflow.com/questions/14694852/can-overridden-methods-differ-in-return-type

These methods appear in Class.getDeclaredMethods() and if they happen to appear before the true non-bridge method, then they can cause BeanBuilder either to:

  1. Generate a getter method with the wrong return type, or
  2. Skip generation of a necessary getter method due to seeing the "concrete" bridge method.

On my machine (Java 1.8.0_181-b13 on Mac), the failing test cases produce two distinct errors:

  1. Jackson data-binding failure because MrBean generated a setter with the parent type.
com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized property "flavor" (class com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$Drink), not marked as ignorable (one known property: "type"])
 at [Source: (String)"{"drink":{"type":"coffee","flavor":"pumpkin spice"}}"; line: 1, column: 37] (through reference chain: com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$CoffeeHolder["drink"]->com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$Drink["flavor"])

	at com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException.from(UnrecognizedPropertyException.java:52)
	at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownProperty(DeserializationContext.java:1061)
	at com.fasterxml.jackson.databind.deser.std.StdDeserializer.handleUnknownProperty(StdDeserializer.java:1606)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownProperty(BeanDeserializerBase.java:1606)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1584)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._vanillaDeserializeWithUnknown(BeanDeserializer.java:443)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._vanillaDeserialize(BeanDeserializer.java:382)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:205)
	at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._vanillaDeserialize(BeanDeserializer.java:338)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:205)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:269)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:2420)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:1431)
	at com.fasterxml.jackson.module.mrbean.TestBridgeMethods.testSimpleCovariantProperty(TestBridgeMethods.java:59)
  1. AbstractMethodError because MrBean skipped generating a getter due to the bridge method:
java.lang.AbstractMethodError: Method com/fasterxml/jackson/module/mrbean/generated/com/fasterxml/jackson/module/mrbean/TestBridgeMethods$SpecificCoffeeHolder.getObject()Lcom/fasterxml/jackson/module/mrbean/TestBridgeMethods$Coffee; is abstract

	at com.fasterxml.jackson.module.mrbean.generated.com.fasterxml.jackson.module.mrbean.TestBridgeMethods$SpecificCoffeeHolder.getObject(Unknown Source)
	at com.fasterxml.jackson.module.mrbean.TestBridgeMethods.testGenericCovariantProperty(TestBridgeMethods.java:73)

Solution

Skip over all synthetic and bridge methods as is done in databind's AnnotatedMethodCollector.

@robbytx
Copy link
Contributor Author

robbytx commented Sep 17, 2020

My company's corporate CLA should be coming shortly -- they've agreed to sign it.

* </ul>
*/
@SuppressWarnings("unchecked")
private static <T> Class<T> reorderBridgeMethodFirst(Class<T> clazz, final String methodName) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love feedback if there is an easier way of doing this. I'm not very familiar with ASM, but I couldn't find any simpler APIs that let me reorder definitions within the class file.

@robbytx
Copy link
Contributor Author

robbytx commented Sep 18, 2020

This PR should be update to merge from https://github.com/bazaarvoice/jackson-modules-base/tree/bridge-methods

@cowtowncoder
Copy link
Member

Looks good wrt fix; test looks impressive but unfortunately I can not really say whether there might be an easier way.
Would be happy to merge this since I got CCLA.

But one quick question first: wouldn't this make sense to merge against 2.12, or is there specific reason it's for master?
I can cherry-pick the fix without problem I think but just want to make sure this wasn't intentional first.

@robbytx
Copy link
Contributor Author

robbytx commented Sep 20, 2020

Similar to #102, I would like like this bug fix to go into 2.12, and into a patch version of 2.11 if appropriate.

Please advise if you would like me to recreate this PR from the BV repo against master, 2.12, or 2.11.

@cowtowncoder
Copy link
Member

I think that as per my comments on #102, this too can go in 2.11.

@robbytx
Copy link
Contributor Author

robbytx commented Sep 22, 2020

Closing this in favor of #110 which targets 2.11

@robbytx robbytx closed this Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants