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

Need support for Kotlin Iterables #4

Closed
pijusn opened this issue Apr 3, 2015 · 12 comments
Closed

Need support for Kotlin Iterables #4

pijusn opened this issue Apr 3, 2015 · 12 comments

Comments

@pijusn
Copy link

pijusn commented Apr 3, 2015

I just implemented custom Iterable and Iterator classes which I tried passing into a (default) mapper. It serializes it as an object rather than an array. It does, however, serialize it as an array if java.lang.Iterable and java.util.Iterator are implemented.

Java and Kotlin Iterables are pretty much the same. I believe, this module should also add serializer for Kotlin iterables.

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

Can you tell me more about your use case, because under the covers there is no difference between a Kotlin class implementing the trait Iterator and a Java one implementing java.util.Iterator. The byte code is the same. Even with delegation, it is the same. Jackson cannot serialize a top-level object of unknown types as anything other than a bean unless you tell it otherwise. Many classes implement iterator but are not intended to only be an iterator. I tested in Java and in Kotlin with basically this code for both, and ended with exactly the same results. Is your use case different?

class IteratorTests {
    class TinyPerson(val name: String, val age: Int)
    class KotlinPersonIterator(private val personList: List<TinyPerson>) : Iterator<TinyPerson> by personList.iterator() {}

    val mapper: ObjectMapper = jacksonObjectMapper().configure(SerializationFeature.INDENT_OUTPUT, false)

    Test fun testKotlinIterator() {
        val expected = """[{"name":"Fred","age":10},{"name":"Max","age":11}]"""
        val people = KotlinPersonIterator(listOf(TinyPerson("Fred", 10), TinyPerson("Max", 11)))
        val kotlinJson = mapper.writerFor<ObjectWriter>(object : TypeReference<Iterator<TinyPerson>>() {}).writeValueAsString(people)
        assertThat(kotlinJson, equalTo(expected))
    }
}

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

When inside another class, same issue, needs to know how to serialize it:

  class Company(val name: String, [JsonSerialize(`as` = javaClass<java.util.Iterator<TinyPerson>>())] val people: KotlinPersonIterator)

    Test fun testKotlinIteratorAsField() {
        val expected = """{"name":"KidVille","people":[{"name":"Fred","age":10},{"name":"Max","age":11}]}"""
        val people = KotlinPersonIterator(listOf(TinyPerson("Fred", 10), TinyPerson("Max", 11)))
        val company = Company("KidVille", people)
        val kotlinJson = mapper.writeValueAsString(company)
        assertThat(kotlinJson, equalTo(expected))
    }

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

Oh wait, the Java test does work as you describe, how odd. Checking more...

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

Note the same test in Java (this is why I'm so happy in Kotlin, so verbose!):

public class IteratorTestsInJava {
    class TinyPerson {
        private String name;
        private int age;

        public TinyPerson(String name, int age) {
            this.name = name;
            this.age = age;
        }

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }

        public int getAge() {
            return age;
        }

        public void setAge(int age) {
            this.age = age;
        }
    }

    class JavaPersonIterator implements Iterator<TinyPerson> {
        private List<TinyPerson> personList;
        private Iterator<TinyPerson> personIterator;

        public JavaPersonIterator(List<TinyPerson> personList) {
            this.personList = personList;
            this.personIterator = personList.iterator();
        }

        @Override
        public boolean hasNext() {
            return personIterator.hasNext();
        }

        @Override
        public TinyPerson next() {
            return personIterator.next();
        }

        @Override
        public void remove() {
            throw new NotImplementedException();
        }
    }

    private ObjectMapper mapper = new ObjectMapper().configure(SerializationFeature.INDENT_OUTPUT, false);

    @Test
    public void testKotlinIterator() throws Exception {
        String expected = "[{\"name\":\"Fred\",\"age\":10},{\"name\":\"Max\",\"age\":11}]";
        JavaPersonIterator people1 = new JavaPersonIterator(Arrays.asList(new TinyPerson("Fred", 10), new TinyPerson("Max", 11)));
        String javaJsonNoType = mapper.writeValueAsString(people1);
        assertEquals(expected, javaJsonNoType);

        JavaPersonIterator people2 = new JavaPersonIterator(Arrays.asList(new TinyPerson("Fred", 10), new TinyPerson("Max", 11)));
        String javaJsonWithType = mapper.writerFor(new TypeReference<Iterator<TinyPerson>>() {
        }).writeValueAsString(people2);
        assertEquals(expected, javaJsonWithType);
    }

    class Company {
        private String name;
        private JavaPersonIterator people;

        public Company(String name, JavaPersonIterator people) {
            this.name = name;
            this.people = people;
        }

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }

        public JavaPersonIterator getPeople() {
            return people;
        }

        public void setPeople(JavaPersonIterator people) {
            this.people = people;
        }
    }

    @Test
    public void testJavaIteratorAsField() throws Exception {
        String expected = "{\"name\":\"KidVille\",[{\"name\":\"Fred\",\"age\":10},{\"name\":\"Max\",\"age\":11}]}";
        JavaPersonIterator people = new JavaPersonIterator(Arrays.asList(new TinyPerson("Fred", 10), new TinyPerson("Max", 11)));
        Company company = new Company("KidVille", people);
        String javaJson = mapper.writeValueAsString(company);
        assertEquals(expected, javaJson);
    }

}

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

Ah, the problem is in BeanSerializerFactory.java (Jackson) it has this check:

if (beanDesc.hasKnownClassAnnotations()) {
                return builder.createDummy();
            }

which changes the behaviour when it sees the KotlinClass annotation, it doesn't discriminate against what class annotations it sees... :-(

It does't call the annotation inspector to let it modify or filter the list. then it is used more blindly later.

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

@cowtowncoder any advice on this one? all Kotlin classes have a KotlinClass annotation with runtime-type information. So when it is seen the BeanSerializerFactory.java calling beanDesc.hasKnownClassAnnotations() returns true, it doesn't care that the annotation is some random thing it does not recognize.

Its good to have the annotation present later, because we use it when looking at constructors and such. But we don't want it counted as something that interferes with Jackson thinking it is a bean, or not. Not sure what the logic for looking at class annotations to create a dummy bean serializer was for, seems like that should be AFTER trying to see if it is an iterator or other known type.

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

(note the annotation inspector _hasAnnotation method doesn't help either because the check is if the collection of annotations size is > 0.

@apatrida
Copy link
Member

apatrida commented Apr 4, 2015

@PiusLT you have a workaround above by specifying how to serialize either the top level or a field, and I will work on resolving the issue as I can find a way to make it work more the same as java.

@cowtowncoder
Copy link
Member

@jaysonminard as far as I remember, using @JacksonAnnotation does not have harmful side effects so it should be ok to add. But then again it should not be necessary.

I do recall a few issues with handling of Iterator, and the main problem is this: given class X that implementas Iterator, should it be serialized as Collection? Quite often it should not, as Iterator is commonly used as sort of add-on to augment processing, but the type is more of a POJO.
This is why there are various heuristics to try to reason right approach.
So this is the background on why such checks are done, instead of simply just using Iterator for class by default.
Given the complexity it may make sense for Kotlin module to try to detect these cases when providing Serializers, in its Serializers implementation (if it already registers one), and not relying on core databind.

This is something that may make sense to discuss on dev list. I remember having some trouble with Guava Iterators and/or iterables.

@apatrida
Copy link
Member

apatrida commented Jun 8, 2015

I'll look at how Guava handles it and can cover Kotlin iterators in the same way. Subclasses of the user's, that's another story.

@cowtowncoder
Copy link
Member

Ok let me know, I may be able to help here. Iterators are nasty as they are essentially mix-ins, secondary interfaces, and quite often it's unclear if it's to be used as the serialization mechanism or not. Many POJOs implement it for convenience. This is the reason why it has such a low precedence if I recall correctly.

@ealymbaev
Copy link

I am using Retrofit and Jackson converter. I cannot apply your fix there, as mapping code is incapsulated there.
When are you planning to release the correct fix?

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