JakeWharton / retrofit2-kotlinx-serialization-converter

A Retrofit 2 Converter.Factory for Kotlin serialization.
Apache License 2.0
1.04k stars 62 forks source link

Decode from stream instead of string #43

Open VincentJoshuaET opened 3 years ago

VincentJoshuaET commented 3 years ago
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 3 years ago

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 3 years ago

This is blocked on:

FunkyMuse commented 2 years ago

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 2 years ago

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

mthmulders commented 2 years ago

I believe streaming support has landed in kotlinx.serialization 1.3.0: https://github.com/Kotlin/kotlinx.serialization/issues/204.

JakeWharton commented 2 years ago

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

gajicm93 commented 2 years ago

@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 2 years ago

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.

ahulyk commented 2 years ago

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

JakeWharton commented 2 years ago

If the response if absolutely huge it would, yes.

ahulyk commented 2 years ago

@JakeWharton cool, thanks

ahulyk commented 2 years ago

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

JakeWharton commented 2 years ago

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

JakeWharton commented 2 years ago

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

JakeWharton commented 2 years ago

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

AlexTrotsenko commented 2 years ago

@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 2 years ago

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