Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Decode from stream instead of string #43

Open
VincentJoshuaET opened this issue Sep 30, 2021 · 17 comments
Open

Decode from stream instead of string #43

VincentJoshuaET opened this issue Sep 30, 2021 · 17 comments

Comments

@VincentJoshuaET
Copy link

VincentJoshuaET commented Sep 30, 2021

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
Copy link
Owner

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
Copy link
Owner

This is blocked on:

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

@FunkyMuse
Copy link

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
Copy link
Owner

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

@mthmulders
Copy link

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

@JakeWharton
Copy link
Owner

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

@gajicm93
Copy link

gajicm93 commented Aug 3, 2022

@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
Copy link
Owner

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
Copy link

ahulyk commented Aug 18, 2022

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

@JakeWharton
Copy link
Owner

If the response if absolutely huge it would, yes.

@ahulyk
Copy link

ahulyk commented Aug 18, 2022

@JakeWharton cool, thanks

@ahulyk
Copy link

ahulyk commented Aug 26, 2022

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

@JakeWharton
Copy link
Owner

JakeWharton commented Aug 26, 2022

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

@JakeWharton
Copy link
Owner

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

@JakeWharton
Copy link
Owner

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

@AlexTrotsenko
Copy link

@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
Copy link
Owner

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

janseeger added a commit to sipgate/dachlatten that referenced this issue Jan 9, 2024
This implements for Jake Wharton's converter as streaming variant (only
one direction). For now that's all we need.
JakeWharton/retrofit2-kotlinx-serialization-converter#43
janseeger added a commit to sipgate/dachlatten that referenced this issue Jan 9, 2024
This implements for Jake Wharton's converter as streaming variant (only
one direction). For now that's all we need.
JakeWharton/retrofit2-kotlinx-serialization-converter#43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants