Skip to content

Conversation

robbytx
Copy link
Contributor

@robbytx robbytx commented Sep 22, 2020

This PR is equivalent to #106 but targets 2.11 instead. The fix (if you wish to cherry-pick separately) is in d3a84d7.

Also: To simplify the test case syntax, this PR upgrades the test sources for the MrBean module to Java 8, by cherry-picking the same commit 286b5d9 from PR 109.

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.

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.module.mrbean.AbstractTypeMaterializer.MyClassLoader;
import org.objectweb.asm.AnnotationVisitor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In master, the package name has changed to net.bytebuddy.jar.asm

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we switched from Asm to ByteBuddy for 3.0... for tests we could still also include Asm unless you wanted to write alt implementation for BB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#106 has the ByteBuddy-based implementation for master. Migrating it back to 2.11 mostly just required changing the package name for the imports because I wasn't using any of the higher-level ByteBuddy constructs.

Copy link
Member

Choose a reason for hiding this comment

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

Asm was only replaced with ByteBuddy for 3.0 (see #28) so no need for that wrt tests.
Although since 2.x has lived longer than I thought back then, maybe backporting change for 2.13 would be good.
So maybe replacing master test from #106 would make sense.

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 22, 2020

Looks like changes I made prevent automatic merge; will merge manually due to it being one small change plus new test file.

@cowtowncoder cowtowncoder added this to the 2.11.3 milestone Sep 22, 2020
@cowtowncoder cowtowncoder changed the title MrBean: Avoid generating properties based on synthetic bridge methods Avoid generating implementations of synthetic bridge methods Sep 22, 2020
cowtowncoder added a commit that referenced this pull request Sep 22, 2020
@robbytx robbytx deleted the patch-bridge-methods branch September 23, 2020 13:59
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