Support serialization of Iterator and Stream

[JooHyukKim]

resolves below issues

• Unable to serialize top-level Java8 Stream #302
• @JacksonXmlElementWrapper ignored on Stream #329
• @JacksonXmlElementWrapper not respected when serializing Iterators / Iterables #148
  • modified related test

[pjfanning]

is this needed? I'd have expected Iterator and Stream to have JavaTypes that are container types.

[JooHyukKim]

Yes, neither of them are container types :/

[pjfanning]

@cowtowncoder would isCollectionLikeType() something that could be considered here? isContainerType() does not include Streams and Iterators. Since we have a JavaType instance here, it would be nice not to have to hardcode the classes.

[JooHyukKim]

Last time I checked Iterator and Stream return false with isCollectionLikeType(). I will double check and return shortly.

it would be nice not to have to hardcode the classes.

I do agree that place where we hardcod-classes will probably end up with more hardcoded-classes. But I can also think as "how many times will Java creators will add interfaces such as Iterator and Stream?"

[JooHyukKim]

would isCollectionLikeType() something that could be considered here?

Or did you mean making Iterator and Stream officially CollectionLikeType, @pjfanning ?

[cowtowncoder]

I am trying to figure out how I feel about this. I feel Iterators etc are not "Collection"-like; perhaps we could/should consider new IteratorType for 2.16? It would have element type; not sure what other commonality there should be.

Trick here (well one of them) is to figure out how Scala, Kotlin (and maybe later JDK modules) would augment information -- otherwise we could just add something in SimpleType.

For inspiration we could look into ReferenceType (or CollectionLikeType).

There could then be something like isIterableType() in JavaType returning true for Map(Like), Collection(Like), Array and IteratorTypes.

Would this be a way forward?

[JooHyukKim]

I say +1 for IteratorType.

There could then be something like isIterableType() in JavaType returning true for Map(Like), Collection(Like), Array and IteratorTypes.

Sounds like this new IteratorType will be direct superset of CollectionLikeType and MapLikeType, anything that can "return" an Iterator.
EDIT(added) : If so, my opinion is desigining this new JavaType should take longer time to design with caution.

I am worried about the name IteratorType as it might create confusion. In reality Map, Stream do not extend, or implement, but just have methods that return an Iterator. ATM I can think of names like IteratorCreatorType or IteratorLikeType 😅

[pjfanning]

Maybe we could merge this PR as it is and then proceed with the JavaType changes independently? We can always uptake the JavaType changes for this use case later.

[pjfanning]

I see that @cowtowncoder made this point about the JavaType changes in another comment.

[cowtowncoder]

Right, +1 for separate approach.

As to IteratorType, could also consider IterableType (although it has same possible issue wrt naming).
Perhaps IterationType would be a better choice.

Also on MapLikeType, CollectionLikeType -- these would not be super types; Maps are not iterators (nor Collections; nor MapLike/CollectionLike). Partly since not all Map(like) types implement actual iterator/iterable (in fact, most don't).
Or put another way: in case something implements Iterator (etc) but is also Map (or Map-like), our JavaType would be Map[Like]Type. Iterator tends to be more an add-on (or mix-in) type.

[pjfanning]

@JooHyukKim should we also add deserialization tests? If you feel this is too much for this PR but suspect that we do need more work on the deserialization, could you log a follow up issue?

[JooHyukKim]

@JooHyukKim should we also add deserialization tests? If you feel this is too much for this PR but suspect that we do need more work on the deserialization, could you log a follow up issue?

@pjfanning I certainly agree we need deserialization counterpart. And yes, let's (I will) create a follow up issue, because I think this already affects broad scope 😄

[cowtowncoder]

On this PR specifically, we could consider doing:

1. Hard-coded approach for 2.15 or 2.16 initially
2. Clean up later on if and when JavaType adds detection functionality.

[pjfanning]

@JooHyukKim could you create a shared method for checking the class for these Iterator.class.isAssignableFrom(cls) || Stream.class.isAssignableFrom(cls) ?

This check appears twice so it would be good to share the check (just in case we need to add more classes to the check).

[JooHyukKim]

@JooHyukKim could you create a shared method for checking the class for these Iterator.class.isAssignableFrom(cls) || Stream.class.isAssignableFrom(cls) ?

This check appears twice so it would be good to share the check (just in case we need to add more classes to the check).

Sure 👍🏻, Sounds great!

[JooHyukKim]

Come to think of it, related issues have been around a long time. Should we rebase this PR agaisnt 2.15? 🤔

[pjfanning]

Not a big deal but I wrote FasterXML/jackson-module-scala#636 to try out this problem with Scala Iterators and there is an issue and this fix won't help (for Scala Iterators). That doesn't mean we shouldn't proceed - it just highlights that adding a JavaType based solution in the future would be useful.

[JooHyukKim]

To keep track of things, let's link this to FasterXML/jackson-databind#3926

[cowtowncoder]

Let's change these to throws Exception since JsonProcessingException is removed from 3.0 and JacksonException (that exists) does not extend IOException any longer.
So use of plain Exception allows cleaner merging.

[JooHyukKim]

Great point 👍🏻👍🏻 Done

[JooHyukKim]

Hmmmm, Sounds like what Jdk8StreamSerialization302Test, does. Maybe move this test case there?

@cowtowncoder I was refering to this class

EDIT: Maybe the class name doesn't reflect what it really tests. Any suggestion? 🤔

[JooHyukKim]

Changed to IterationType302Test