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

Add empty class support for deduction #3137

Closed
wants to merge 2 commits into from

Conversation

xJoeWoo
Copy link

@xJoeWoo xJoeWoo commented Apr 30, 2021

Fix #3139

This PR adds the ability of deserializing an empty class to AsPropertyTypeDeserializer.

@cowtowncoder
Copy link
Member

Quick note: due to size of changes, I think this needs to go in 2.13 branch; this may be safe change but just in case.

@drekbour WDYT?

@@ -35,6 +35,8 @@
// Bitmap of available fields in each subtype (including its parents)
private final Map<BitSet, String> subtypeFingerprints;

private static final String EMPTY_CLASS_MARKER = "";
Copy link
Member

Choose a reason for hiding this comment

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

Needs Javadoc to explain use.

Copy link
Member

Choose a reason for hiding this comment

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

One comment: empty String is actually valid property name in JSON -- is this problematic?

@cowtowncoder
Copy link
Member

Ok so I wish I had time to dig into this deeper to make sure but... it seems to me that since "empty" class is pretty much a special case, it might make sense to not try to shoehorn it with bogus "empty property", but rather have a separate marker to indicate type with no properties, if any (and if multiple, probably fail). And then fully separate handling of "no incoming properties" case (to match to one existing no-properties pojo, if any) from handling of "one or more properties".
It seems like this might lead to even simpler handling.
I realize I may be wrong about feasibility since I haven't tried to do this.

@drekbour
Copy link
Contributor

drekbour commented May 2, 2021

I'll take a look over the next few days

@drekbour
Copy link
Contributor

drekbour commented May 3, 2021

Concept:

@cowtowncoder This is another example of the impossible-to-implement "deduction by absence" - one which could be handled (with compromises) by defaultImpl.

However ... the specific case of an empty subtype matching an explicit {} has merit and Jackson shouldn't proscribe it.

Implementation:

@xJoeWoo
There is a lot of change here to create a sentinel field representing "no fields". I think this is missing a crucial point. In the existing code an empty class is already represented by an empty fingerprint (BitSet(0)).

With respect, I'll submit a competing PR with a simpler implementation of the same concept

@cowtowncoder
Copy link
Member

Thank you @drekbour -- this makes sense. I was thinking along same lines wrt this being true special case (wrt earlier discussions on why generic absence-based selection cannot be done by simple extension).

And @xJoeWoo definitely gets credit for proposing adding this useful feature (will add in release notes once we get this done), either way.

@cowtowncoder
Copy link
Member

Merged the alternate implementation, thank you @xJoeWoo for reporting the issue & submitting the patch!

@xJoeWoo
Copy link
Author

xJoeWoo commented May 4, 2021

@drekbour The concept of your implementation is better and neater than mine. I really like to read such brilliant codes from great projects.

@cowtowncoder It's my pleasure to make this tool better ;). And I have to admit that I didn't realize a empty name property was acceptable in json standard until you mentioned it.

@xJoeWoo
Copy link
Author

xJoeWoo commented May 4, 2021

Concept:

@cowtowncoder This is another example of the impossible-to-implement "deduction by absence" - one which could be handled (with compromises) by defaultImpl.

However ... the specific case of an empty subtype matching an explicit {} has merit and Jackson shouldn't proscribe it.

Implementation:

@xJoeWoo
There is a lot of change here to create a sentinel field representing "no fields". I think this is missing a crucial point. In the existing code an empty class is already represented by an empty fingerprint (BitSet(0)).

With respect, I'll submit a competing PR with a simpler implementation of the same concept

Ah.. I walked through buildFingerprints method again and found the empty BitSet did created if no properties were presented. Totally no need to use an empty property name to mark an empty class. I didn't catch the simpler way.

@cowtowncoder
Copy link
Member

cowtowncoder commented May 4, 2021

@xJoeWoo np at all -- it's much easier to spot various choices if you wrote the feature (in this case, @drekbour ) :)
And use of empty String as key really isn't common, most likely it is used as marker of some kind, if anything. So it's more a trivia thing.
Once again, thank you for suggesting the improvement!

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.

3 participants