JakeWharton/retrofit2-kotlinx-serialization-converter

Decode from stream instead of string

VincentJoshuaET opened this issue · 17 comments

VincentJoshuaET commented 
class FromString(override val format: StringFormat) : Serializer() {
    override fun <T> fromResponseBody(loader: DeserializationStrategy<T>, body: ResponseBody): T {
      val string = body.string()
      return format.decodeFromString(loader, string)
    }

    override fun <T> toRequestBody(contentType: MediaType, saver: SerializationStrategy<T>, value: T): RequestBody {
      val string = format.encodeToString(saver, value)
      return RequestBody.create(contentType, string)
    }
  }

to

class FromStream(override val format: StringFormat) : Serializer() {
    override fun <T> fromResponseBody(loader: DeserializationStrategy<T>, body: ResponseBody): T {
      val stream = body.byteStream()
      return format.decodeFromStream(loader, stream)
    }

    override fun <T> toRequestBody(contentType: MediaType, saver: SerializationStrategy<T>, value: T): RequestBody {
      val stream = ByteArrayOutputStream()
      format.encodeToStream(saver, value, stream)
      return RequestBody.create(contentType, stream.toByteArray())
    }
  }
JakeWharton commented

I don't see much reason to change the encoding since both end up buffering. We can switch the decoding, however. Want to send a PR?

JakeWharton commented

This is blocked on:

  • Streaming being available for all format types, not just a single one
  • The streaming API being stable
FunkyMuse commented

This is blocked on:

* [ ]  Streaming being available for all format types, not just a single one

* [ ]  The streaming API being stable

Is there any update on this or where can we track this?

JakeWharton commented

On the kotlinx.serialization repo. I'm not sure if there are bugs for either, but you could create them.

  • 👍1
mthmulders commented

I believe streaming support has landed in kotlinx.serialization 1.3.0: Kotlin/kotlinx.serialization#204.

JakeWharton commented

Yes, but only for JSON. This issue was filed after 1.3.0 was released.

gajicm93 commented

@JakeWharton I see that 1.4.0 now supports Okio BufferedSource and BufferedSink. Is this converter gonna be updated with that or is Retrofit maybe planning on releasing an official converter? Thanks.

JakeWharton commented

No, nothing has really changed. Streaming still only works for JSON not all text formats. And the APIs this library uses are still marked as unstable which means we cannot upstream it yet.

  • 👍4
ahulyk commented

@JakeWharton Will the usage of Okio BufferedSource and BufferedSink APIs reduce memory footprint during serialization/deserialization?

JakeWharton commented

If the response if absolutely huge it would, yes.

ahulyk commented

@JakeWharton cool, thanks

ahulyk commented

@JakeWharton we have stable kotlinx.serialization v1.4.0 could we expect the support of Okio in the near future?

JakeWharton commented

No. It's only supported for Json and not arbitrary StringFormats.

  • 👍1
JakeWharton commented

I suppose we could add a Json-specific overload of the factory function for now to enable it.

JakeWharton commented

Although Json is in a separate library so it would require a separate artifact...

AlexTrotsenko commented

@JakeWharton given that you are considering separate artifact for JSON - do you think it is realistic to have an "official" retrofit-converter for JSON with kotlinx-serialization? As far as I see, JSON support seams to be stable in kotlinx-serialization now.

JakeWharton commented

No, the APIs we use are still unstable for any format.

  • 👍1