JakeWharton / ThreeTenABP

An adaptation of the JSR-310 backport for Android.
Apache License 2.0
3.55k stars 133 forks source link

Make it compatible with Retrofit #67

Closed feinstein closed 6 years ago

feinstein commented 6 years ago

I am having issues using this library with Retrofit and GSON.

Since some of the classes, like ZoneId are abstract and GSON can't create them, I am getting exceptions on Retrofit when I try do deserialize my webservice JSON response.

It would be great to have an adapter that could be plugged into Retrofit to provide GSON the ability to deal with this library.

JakeWharton commented 6 years ago

Looks like https://github.com/aaronhe42/ThreeTen-Backport-Gson-Adapter exists.

Regardless, though, this is a request for ThreeTenBP and not this library which merely repackages its timezone database.

feinstein commented 6 years ago

I found that adapter, but I thought that maybe since you are one of the developers of Retrofit and this library, maybe you would want to make something streamlined for Retrofit, but I will use that adapter anyways, thank you Jake.

JakeWharton commented 6 years ago

Retrofit doesn't know anything about what types are being serialized by Gson or whatever serialization library to choose.

On Thu, Jan 4, 2018 at 12:31 PM feinstein notifications@github.com wrote:

I found that adapter, but I thought that maybe since you are one of the developers of Retrofit and this library, maybe you would want to make something streamlined for Retrofit, but I will use that adapter anyways, thank you Jake.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/JakeWharton/ThreeTenABP/issues/67#issuecomment-355346194, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEXXniiMm-4eTJWdd334TV3s99VO3ks5tHQrpgaJpZM4RTQvu .

feinstein commented 6 years ago

I know, but since it's so easy to add a Converter on Retrofit, I was hoping there could be an easy plug and play solution for this as well, like a wrapper around the GsonConverterFactory, or something similar.... Not that registering an adapter with GSON is a great deal of work, just not as simple as it could be.

feinstein commented 6 years ago

FYI: The https://github.com/aaronhe42/ThreeTen-Backport-Gson-Adapter doesn't work.

It calls GSON JsonElement.getAsString() which is basically an UnsupportedOperationException.

So now I will have to write my own adapter :/

JakeWharton commented 6 years ago

If you switch to Moshi (Gson's vastly superior successor) it's only like 4 lines of code.

On Thu, Jan 4, 2018, 8:56 PM feinstein notifications@github.com wrote:

FYI: The https://github.com/aaronhe42/ThreeTen-Backport-Gson-Adapter doesn't work.

It calls GSON JsonElement.getAsString() which is basically an UnsupportedOperationException.

So now I will have to write my own adapter :/

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/JakeWharton/ThreeTenABP/issues/67#issuecomment-355452475, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEdw2d-KxtWXZh1KXccW8ZJEC22Tnks5tHYFpgaJpZM4RTQvu .

feinstein commented 6 years ago

When I was searching for JSON solutions I decided to not use Moshi because it appeared to be 2x slower than GSON, at least from this benchmark: https://medium.com/@IlyaEremin/android-json-parsers-comparison-2017-8b5221721e31

Maybe the benchmark was flawed?

I will try Moshi then, if it fixes my issue I can cope with the slower performance. Thank you for the recommendation Jake!

JakeWharton commented 6 years ago

If you're using Retrofit you're using OkHttp which uses Okio which is what Moshi uses which means there's no intermediate byte[] copies between layers so it's faster. Most Gson vs. Moshi benchmarks either deal with Strings where it's slower or actually use a real network where the network behavior contributes instability to the timings.

I can provide example Moshi adapters when I get home.

On Thu, Jan 4, 2018, 9:18 PM feinstein notifications@github.com wrote:

When I was searching for JSON solutions I decided to not use Moshi because it appeared to be 2x slower than GSON, at least from this benchmark: https://medium.com/@IlyaEremin/android-json-parsers-comparison-2017-8b5221721e31

Maybe the benchmark was flawed?

I will try Moshi then, if it fixes my issue I can cope with the slower performance. Thank you for the recommendation Jake!

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/JakeWharton/ThreeTenABP/issues/67#issuecomment-355455409, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEaESw9FF__g56DfZ__JH4l1mhuBLks5tHYaSgaJpZM4RTQvu .

feinstein commented 6 years ago

Oh, that makes perfect sense!

And yes, adapter examples would be most welcome :D

JakeWharton commented 6 years ago
public @Rfc1123 OffsetDateTime createdAt;
public final class ModelAdapters {
  @FromJson //
  public @Rfc1123 OffsetDateTime rfc1123OffsetDateTimeFromJson(String value) {
    return OffsetDateTime.parse(value, RFC_1123_DATE_TIME);
  }

  @ToJson //
  public String rfc1123OffsetDateTimeToJson(@Rfc1123 OffsetDateTime value) {
    return RFC_1123_DATE_TIME.format(value);
  }
}
Moshi moshi = new Moshi.Builder() //
    .add(new ModelAdapters()) //
    .build();

// ...

.addConverterFactory(MoshiConverterFactory.create(moshi)) //
feinstein commented 6 years ago

Thanks Jake.

I already have it working but I made it differently. I used Jackson's Java time module on my server, so it will convert automatically a ZonedDateTime to a timestamp, like an Instant. Then I made a Moshi adapter to convert that timestamp back to a ZonedDateTime.

The only caveat though is that the timestamp is sent as a double with nanoseconds as the decimals. When Moshi grabs this double with will crash on Parsing, since it's a big double and somehow appears a scientific notation in the string behind the covers, so it won't convert it to a string automatically either, so I had to use a JsonReader as my Adapter's input, in order to get the timestamp double as a string.

It would be nice to have the Moshi adapters to automatically pass numeric values as strings if the adapter has a string parameter, like the JsonReader does.

JakeWharton commented 6 years ago

Can you file a bug on Moshi for that? Preferably with a unit test that demonstrates the problem, if you can.

On Mon, Jan 22, 2018 at 11:06 AM feinstein notifications@github.com wrote:

Thanks Jake.

I already have it working but I made it differently. I used Jackson's Java time module on my server, so it will convert automatically a ZonedDateTime to a timestamp, like an Instant. Then I made a Moshi adapter to convert that timestamp back to a ZonedDateTime.

The only caveat though is that the timestamp is sent as a double with nanoseconds as the decimals. When Moshi grabs this double with will crash on Parsing, since it's a big double and somehow appears a scientific notation in the string behind the covers, so it won't convert it to a string automatically either, so I had to use a JsonReader as my Adapter's input, in order to get the timestamp double as a string.

It would be nice to have the Moshi adapters to automatically pass numeric values as strings if the adapter has a string parameter, like the JsonReader does.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/JakeWharton/ThreeTenABP/issues/67#issuecomment-359472265, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEERAHhrPRgg6GWFkzn4nQEJ5aNqAfks5tNLH2gaJpZM4RTQvu .

feinstein commented 6 years ago

Sure, I will make it later today.

feinstein commented 6 years ago

I tried to reproduce the issue and roll back my code, but now it works, so I have no idea what was going on haha.

Even the @FromJson ZonedDateTime fromJson(String timestamp) Adapter that would crash saying timestamp isn't a String, but a double is now passing the original numeric value as a String so I can simplify my code and ditch the JsonReader apparently.

Thank you for your help @JakeWharton!