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

Jackson 3 TypeFactory CollectionTypes #3926

Closed
pjfanning opened this issue May 13, 2023 · 10 comments
Closed

Jackson 3 TypeFactory CollectionTypes #3926

pjfanning opened this issue May 13, 2023 · 10 comments

Comments

@pjfanning
Copy link
Member

pjfanning commented May 13, 2023

Could we consider fixing the issue described in

// 01-Nov-2015, tatu: As of 2.7, couple of potential `CollectionLikeType`s (like
// `Iterable`, `Iterator`), and `MapLikeType`s (`Map.Entry`) are not automatically
// detected, related to difficulties in propagating type upwards (Iterable, for
// example, is a weak, tag-on type). They may be detectable in future.
about not treating some collection types as collection types in TypeFactory.

  • Stream
  • Iterator
  • Map.Entry
@pjfanning pjfanning added the to-evaluate Issue that has been received but not yet evaluated label May 13, 2023
@JooHyukKim
Copy link
Member

JooHyukKim commented May 13, 2023

+1. Would help in cases like this PR in FasterXML/jackson-dataformat-xml#597

Only crossing out because this issue is against Jackson 3 not affecting the linked PR. But still +1 👍🏻 😄

@pjfanning pjfanning changed the title Jackson 2 TypeFactory CollectionTypes Jackson 3 TypeFactory CollectionTypes May 13, 2023
@pjfanning
Copy link
Member Author

+1. Would help in cases like this PR in FasterXML/jackson-dataformat-xml#597

I meant to say Jackson 3 in the issue title. I don't think there will be an appetite to change this in Jackson 2.

@cowtowncoder
Copy link
Member

Before considering those possibilities (note: they cannot be CollectionTypes as types do not implement Collection), it'd be good to know what the intended benefit is -- is it just for more convenient detection of type parameters or something else? Not against improvements, just want to know the goal.

@yihtserns
Copy link
Contributor

yihtserns commented May 15, 2023

...they cannot be CollectionTypes as types do not implement Collection...

Looking at the quoted code, I think he meant to say "collection-like types" not "collection types".

@cowtowncoder
Copy link
Member

@yihtserns Right, that makes sense. But I think I'd prefer new "IteratorType" (or "IterableType"), since these are not (depending on one's POV, one can disagree) Collection-like -- difference between value and sort of pointer to value.
Part of the challenge is that I think there's a chance (maybe small) that some existing code might otherwise assume properties for newly discovered types that are not true.

@cowtowncoder
Copy link
Member

One quick note on Map.Entry: that sort of qualifies as MapLikeType. But here I am particularly worried about compatibility: registering Serializers/Deserializers via Serializers / Deserializers has separate callback methods -- so changing JavaType for Map.Entry would likely break some registrations.

Same is actually true for possible new "IterationType" (or whatever name we'd use).
For 2.x we'd need to consider whether to change Serializers/Deserializers by adding new method, or not.

@cowtowncoder
Copy link
Member

Name suggestion: IterationType.

@cowtowncoder
Copy link
Member

... and now that I finally read my own code comment, I realized that Old Me pointed out relevant challenge wrt add-on/tag types. So there may be reasons why there may be challenges in detecting various iterator types. But maybe it's worth trying things out and see what happens.

@cowtowncoder
Copy link
Member

Ok: realized another quick thing:

  1. Separate type makes sense for actual iterators (Iterator, Stream) but
  2. Does NOT make sense for Iterable -- that's not iterator, it's something that can produce iterator. And as such is almost certainly Collection- or Map-Like type

With that, I think progress can be made.

This still leaves the question of Map.Entry -- I think it could be upgraded into MapLikeType, but maybe as separate PR/issue

@cowtowncoder
Copy link
Member

So, support for IterationType (which initially covers Iterator, Stream, DoubleStream/IntStream/LongStream) has been added for 2.16. This may perhaps be closed.

Not sure about Map.Entry, could consider making it MapLikeType; if so, should file separate issue.

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Dec 6, 2023
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

No branches or pull requests

4 participants