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

Deduction of supertypes #3300

Open
wants to merge 3 commits into
base: 2.13
Choose a base branch
from

Conversation

drekbour
Copy link
Contributor

To discuss: Alternative implementation of #3289 allowing supertypes to be selected.

Animal {a,b} <- Cat {c} , Dog {d}

In this form it is very lenient:

  • Any single valid Animal field (e.g. {a}) will resolve Animal if Animal is included in the @JsonSubTypes
  • But a non-abstract annotated root class already counts as a JsonType (I had not realised Animal was included without being explicitly redeclared in it's own @JsonSubTypes)

As seen from changes to the tests, it's actually fairly hard to get this code to fallback to defaultImpl if Animal is non-abstract. This represents a change in behaviour.

h3. FAIL_ON_UNKNOWN_PROPERTIES

Notably the code remains unaware of FAIL_ON_UNKNOWN_PROPERTIES so {a,x} will resolve subtype as Animal but then fail on property x. This is quite easy to resolve but is a policy that needs clarifying - should {a,x} fail deduction and use defaultImpl or deduce Animal and fail via further-downstream error handling?

// Pre-sorted fingerprints means the _first_ item may be a supertype...
BitSet supertypeFields = candidates.get(0);

// bitset is mutable, must take a copy :( ThreadLocal?
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'm not in love with this method being part of the live processing. Have a sneaking feeling I've missed some logic reduction here

@cowtowncoder
Copy link
Member

I wish I could give good feedback here, but unfortunately I don't really know the area well enough.
I grasp the general challenges but I feel this really needs attention from someone else to focus, in addition to @drekbour who obviously knows what he's doing. :)

Just not sure where to get that feedback; mailing lists are often black holes / void into which I can yell questions and not get many answers.

@drekbour
Copy link
Contributor Author

Ok. I'm still thinking on it myself but am coming around to the idea that honouring FAIL_ON_UNKNOWN_PROPERTIES is more likely to meet user expectations. I'll add a further commit to this PR with that.

@drekbour
Copy link
Contributor Author

FYI This is PR is not abandoned. Additional thought : incorporate required properties

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

Successfully merging this pull request may close these issues.

None yet

2 participants