Skip to content

Conversation

kostya05983
Copy link
Contributor

Hello @cowtowncoder , I add sequence serializer and deserializer, but I can't it register properly it but it works only when you mark your field with JsonDesirializer. Any suggestion why doesn't KotlinModule use it? I tried it regitster in setupModule section.

@apatrida
Copy link
Member

@kostya05983 is there an issue with serializing a class holding a Sequence because the sequence will be consumed and not useable again by anyone else. That seems unsafe. Deserializing into a Sequence is kinda similar and doesn't have a lot of value if it is really not lazy anyway. Thoughts?

And yes, you need to register Serializers/Deserializers which I can help with but I wanted to talk about this first.

apatrida pushed a commit that referenced this pull request Oct 26, 2019
@apatrida apatrida merged commit 21c74d0 into FasterXML:2.10 Oct 26, 2019
@juan-palacios
Copy link

The approach in this implementation seems to translate the sequence into a list with the toList extension function. The thing is that in Kotlin we have things like the kotlin.sequences.sequence function which is documented as:

Builds a [Sequence] lazily yielding values one by one.

Which in combination with yieldAll allows us to build a Sequence that lazily loads values and streams them to the Iterator. This is very helpful when we want to safely stream an unknown number of items using a paged API.

Calling toList on the Sequence effectively loads all the items into memory to dump them into an ArrayList, which defeats the purpose.

I found that I can use a JsonGenerator to manually iterate over the Sequence, but I feel like the native support for sequences shouldn't assume that Sequence and List are interchangeable.

Or is there more context around the decision on how to handle sequences that I'm missing?

@kostya05983
Copy link
Contributor Author

Yes, your way to iterate sequence and serialise it is better than call just toList. Can you provide a pr?)

@juan-palacios
Copy link

I've created this issue to track the change: #368

I'll submit a PR shortly

@juan-palacios
Copy link

@kostya05983 here's the PR #369

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