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

Flatten Jackson inheritance hierarchies where needed #277

Merged

Conversation

Projects
None yet
3 participants
@kelnos
Copy link
Member

commented May 2, 2019

This is to prevent us from generating uncompileable multiple-inheritance hierarchies. It works like this:

  1. If there's more than one parent class that has a discriminator, we throw an error.
  2. If there's a single parent that has a discriminator, we extend from that parent class. If there are other parents, we flatten them so the generated class has the properties of all those other parents.
  3. If there's just a single parent, we extend from that parent class.
  4. Otherwise there's no composition or polymorphism going on, so we just emit a class that extends from nothing.

Unfortunately this necessitated removing the static builder() method from the POJO class, because you can't have a static method in a subclass with the same arg list that has a different return type. (Which is an odd restriction since Java doesn't support static method overriding, but perhaps they did it for future-proofing.) To compensate, the builder constructors are now public.

I'm not including a test spec with this PR as it causes the Scala build to fail, which should hopefully be fixed when #234 lands.

Note that the vast majority of this PR is only whitespace change as the indentation level changed for a bunch of code.

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.
Flatten Jackson inheritance hierarchies where needed
This is to prevent us from generating uncompileable multiple-inheritance
hierarchies.  It works like this:

1) If there's more than one parent class that has a discriminator, we
throw an error.

2) If there's a single parent that has a discriminator, we extend from
that parent class.  If there are other parents, we flatten them so the
generated class has the properties of all those other parents.

3) If there's just a single parent, we extend from that parent class.

4) Otherwise there's no composition or polymorphism going on, so we just
emit a class that extends from nothing.

Unfortunately this necessitated removing the static `builder()` method
from the POJO class, because you can't have a static method in a
subclass with the same arg list that has a different return type.
(Which is an odd restriction since Java doesn't support static method
overriding, but perhaps they did it for future-proofing.)  To
compensate, the builder constructors are now public.

@kelnos kelnos requested a review from blast-hardcheese May 2, 2019

@blast-hardcheese
Copy link
Collaborator

left a comment

Conceptually looks good to me. Iterating in on something feature-stable in Java will likely shake out any oddities that emerge from this, but for now it's limited to the Java generators, so I'm onboard.

@tomasherman

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

FYI your build failed

@kelnos

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@tomasherman yeah, the scala 2.11 compiler doesn't like it. we're considering dropping 2.11 support; it's becoming a headache fighting against compiler bugs that will never get fixed.

@blast-hardcheese
Copy link
Collaborator

left a comment

very unfortunate that we're still dealing with that 2.11 bug.

Let's drop support for 2.11.

@blast-hardcheese blast-hardcheese merged commit 70b4a47 into twilio:master May 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

@kelnos kelnos deleted the kelnos:jackson-flatten-multiple-inheritance branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.