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

Support serialization of Iterator and Stream #597

Merged
merged 6 commits into from
May 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package com.fasterxml.jackson.dataformat.xml.util;

import java.util.Collection;

import com.fasterxml.jackson.databind.JavaType;

import java.util.Collection;
import java.util.Iterator;
import java.util.stream.Stream;

public class TypeUtil
{
/**
Expand All @@ -12,8 +14,11 @@ public class TypeUtil
*/
public static boolean isIndexedType(JavaType type)
{
if (type.isContainerType()) {
Class<?> cls = type.getRawClass();
Class<?> cls = type.getRawClass();
if (type.isContainerType()
|| Iterator.class.isAssignableFrom(cls)
|| Stream.class.isAssignableFrom(cls)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, neither of them are container types :/

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?"

Copy link
Member Author

Choose a reason for hiding this comment

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

would isCollectionLikeType() something that could be considered here?

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@JooHyukKim JooHyukKim May 15, 2023

Choose a reason for hiding this comment

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

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 😅

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

) {
// One special case; byte[] will be serialized as base64-encoded String, not real array, so:
// (actually, ditto for char[]; thought to be a String)
if (cls == byte[].class || cls == char[].class) {
Expand All @@ -31,7 +36,8 @@ public static boolean isIndexedType(JavaType type)

public static boolean isIndexedType(Class<?> cls)
{
return (cls.isArray() && cls != byte[].class && cls != char[].class)
|| Collection.class.isAssignableFrom(cls);
return (cls.isArray() && cls != byte[].class && cls != char[].class)
|| Collection.class.isAssignableFrom(cls) || Iterator.class.isAssignableFrom(cls)
|| Stream.class.isAssignableFrom(cls);
pjfanning marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public void testStringList() throws IOException
// [dataformat-xml#148]
public void testIteratorSerialization() throws Exception
{
assertEquals("<Bean148><item>2</item><item>1</item><item>0</item></Bean148>",
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved
assertEquals("<Bean148><list><item>2</item><item>1</item><item>0</item></list></Bean148>",
MAPPER.writeValueAsString(new Bean148()).trim());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
package com.fasterxml.jackson.dataformat.xml.ser;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.xml.XmlMapper;
import com.fasterxml.jackson.dataformat.xml.XmlTestBase;
import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper;
import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;
import java.util.stream.Stream;

// [dataformat-xml#302] : Unable to serialize top-level Java8 Stream
public class Jdk8StreamSerialization302Test extends XmlTestBase {
Copy link
Member Author

@JooHyukKim JooHyukKim May 21, 2023

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to IterationType302Test


final ObjectMapper OBJECT_MAPPER = new XmlMapper();

public static class StreamWrapper329 {
private Stream<String> data;

@JacksonXmlElementWrapper(localName = "elements")
@JacksonXmlProperty(localName = "element")
public Stream<String> getData() {
return data;
}

public void setData(Stream<String> data) {
this.data = data;
}
}

public static class CollectionWrapper329 {
private Collection<String> data;

@JacksonXmlElementWrapper(localName = "elements")
@JacksonXmlProperty(localName = "element")
public Collection<String> getData() {
return data;
}

public void setData(Collection<String> data) {
this.data = data;
}
}

public static class IteratorWrapper329 {
private Iterator<String> data;

@JacksonXmlElementWrapper(localName = "elements")
@JacksonXmlProperty(localName = "element")
public Iterator<String> getData() {
return data;
}

public void setData(Iterator<String> data) {
this.data = data;
}
}

public void testCollectionSerialization() throws JsonProcessingException {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point 👍🏻👍🏻 Done

Collection<String> list = new ArrayList<>();
list.add("a");
list.add("b");

assertEquals("<ArrayList><item>a</item><item>b</item></ArrayList>",
OBJECT_MAPPER.writeValueAsString(list));
}

public void testListSerialization() throws JsonProcessingException {
List<String> list = new ArrayList<>();
list.add("a");
list.add("b");
assertEquals("<ArrayList><item>a</item><item>b</item></ArrayList>",
OBJECT_MAPPER.writeValueAsString(list));
}

public void testListIteratorSerialization() throws JsonProcessingException {
List<String> list = new ArrayList<>();
list.add("a");
list.add("b");
Iterator<String> listItr = list.iterator();

assertEquals("<Itr><item>a</item><item>b</item></Itr>",
OBJECT_MAPPER.writeValueAsString(listItr));
}


public void testStreamIteratorSerialization() throws JsonProcessingException {
assertEquals("<Adapter><item>a</item><item>b</item></Adapter>",
OBJECT_MAPPER.writeValueAsString(Stream.of("a", "b").iterator()));
}

// [dataformat-xml#329] : Jackson ignores JacksonXmlElementWrapper on Stream
public void testCollectionWrapperSerialization329() throws JsonProcessingException {
Collection<String> collection = new ArrayList<>();
collection.add("a");
collection.add("b");
CollectionWrapper329 wrapper = new CollectionWrapper329();
wrapper.setData(collection);

assertEquals(
"<CollectionWrapper329><elements>" +
"<element>a</element>" +
"<element>b</element>" +
"</elements></CollectionWrapper329>",
OBJECT_MAPPER.writeValueAsString(wrapper));
}

// [dataformat-xml#329] : Jackson ignores JacksonXmlElementWrapper on Stream
public void testIteratorWrapperSerialization329() throws JsonProcessingException {
Collection<String> collection = new ArrayList<>();
collection.add("a");
collection.add("b");
IteratorWrapper329 wrapper = new IteratorWrapper329();
wrapper.setData(collection.iterator());

assertEquals(
"<IteratorWrapper329><elements>" +
"<element>a</element>" +
"<element>b</element>" +
"</elements></IteratorWrapper329>",
OBJECT_MAPPER.writeValueAsString(wrapper));
}

/*
/**********************************************************
JooHyukKim marked this conversation as resolved.
Show resolved Hide resolved
/* Stream tests, requires Jdk8Module
/**********************************************************

public void testStreamSerialization() throws JsonProcessingException {
String jsonStr = OBJECT_MAPPER.writeValueAsString(Stream.of("a", "b"));
assertEquals("<Head><item>a</item><item>b</item></Head>", jsonStr);
}

// [dataformat-xml#329] : Jackson ignores JacksonXmlElementWrapper on Stream
public void testStreamWrapperSerialization329() throws JsonProcessingException {
StreamWrapper329 wrapper = new StreamWrapper329();
wrapper.setData(Stream.of("a", "b"));

assertEquals(
"<StreamWrapper329><elements>" +
"<element>a</element>" +
"<element>b</element>" +
"</elements></StreamWrapper329>",
OBJECT_MAPPER.writeValueAsString(wrapper));
}
*/
}