Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add support for custom Gson instances #645

Merged
merged 3 commits into from
Jun 27, 2019
Merged

✨ Add support for custom Gson instances #645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5edf7c7
Add support for custom Gson instances
lostintime Jun 1, 2019
d30af2c
Remove Needless blank line
lostintime Jun 2, 2019
2b2fd6f
Move default Gson() instance to gsonDeserializer() arguments
lostintime Jun 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 45 additions & 8 deletions fuel-gson/src/main/kotlin/com/github/kittinunf/fuel/gson/FuelGson.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@ import java.io.Reader
inline fun <reified T : Any> Request.responseObject(noinline handler: ResponseResultHandler<T>) =
response(gsonDeserializer(), handler)

/**
* Asynchronously gets a response object of [T] wrapped in [Result] via [handler] using custom [Gson] instance
*
* @param gson [Gson] custom Gson deserializer instance
* @param handler [ResponseResultHandler<T>] the handler that is called upon success
* @return [CancellableRequest] request that can be cancelled
*/
inline fun <reified T : Any> Request.responseObject(gson: Gson, noinline handler: ResponseResultHandler<T>) =
Copy link
Owner

@kittinunf kittinunf Jun 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we don't have anything particularly to against this, this obviously also does the job. But what do you think about what we have done in the fuel-moshi extension?

Here is the link to the master version

https://github.com/kittinunf/fuel/blob/master/fuel-moshi/src/main/kotlin/com/github/kittinunf/fuel/moshi/FuelMoshi.kt#L15

In short, in Moshi extension, we just expose the Moshi.Builder() directly. What do you think about it (exposing the GsonBuilder)? I think it might be just easier and consistent to do the same thing?

Copy link
Author

@lostintime lostintime Jun 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't Moshi.Builder() mutable? I'm afraid users can end messing things up, ex: you may want to decode same type from different APIs with different formats, I usually prefer to not expose unsafe APIs, but completely support you about consistency

Of course both options may be provided - shared mutable builder and explicit argument.

Also users may fallback to using gsonDeserializer(Gson) directly, in this case that function is not needed.

This is how my temporary solution currently looks:

inline fun <reified T : Any> Gson.deserializer(): ResponseDeserializable<T> =
    object : ResponseDeserializable<T> {
        override fun deserialize(reader: Reader): T? = fromJson<T>(reader, object : TypeToken<T>() {}.type)
    }

Fuel
    .get("path/to/my/service")
    .timeoutRead(timeoutMillis)
    .responseObject(gson.deserializer<List<MyResponse>>()) { result ->
        Either.run {
            result.fold(success = { cb(right(it)) }, failure = { cb(left(it)) })
        }
    }

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, I think you are right. While we are exposing the mutable builder, it provides the ease of use (with the power of users can mess things up of course). But what you said is correct about exposing the safe API is usually a better choice.

I think it depends on how much compromisation we would love to make. I think maybe exposing 2 APIs options also makes sense and giving flexibility.

As you are obviously one of the users! 😄, WDYT? In whatever way, we come to the conclusion, I will open a PR for moshi to standardize this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for having explicit arguments for responseObject() functions and this will be consistent with FuelJackson implementation.

I would strongly discourage using shared builder, but ... :) - to keep it consistent - or need to add it to fuel-gson also, or deprecate it from other implementations - which is a breaking change: so this is more a decision for project owner, my view on the project is very limited.

We can keep it as is for now and it may be added later, when somebody request it: not giving it now - is ok ... but giving it now and getting it back later - will be a breaking change

Copy link
Collaborator

@SleeplessByte SleeplessByte Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to:

  • deprecate shared builder (don't remove, yet), add default builder options to both (or all integrations)
  • add function call time builder argument that defaults to a new builder using the default options.

In that case we can communicate to set the default options once and once only -- and we create a new builder each time --

it still has the issue that someone might mutate defaultOptions as the go, but that would be on them.

Copy link
Owner

@kittinunf kittinunf Jun 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do that then? @lostintime would you mind doing us a favor of providing both options? It could be in separated PR though. We will be taking care of some other extensions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, (was a bit busy these days :)

So, just to confirm - do you want me to add a val defaultGson = GsonBuilder() instance and mark it as deprecated from the beginning? or just add default value to gsonDeserializer(gson = Gson()) function, and use it everywhere needed?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just add default value to gsonDeserializer(gson = Gson()) function, and use it everywhere needed?

Ah I meant this!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved Gson() instance to gsonDeserializer agrument.

I've also changed gsonDeserializerOf to:

inline fun <reified T : Any> gsonDeserializerOf(clazz: Class<T>) = gsonDeserializer<T>()

It seemed redundant to me and there was one ore Gson instance used, but I'm not sure this is right, or maybe add one more gson argument here also, with default value?

Copy link
Owner

@kittinunf kittinunf Jun 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds great with a default value. But I think I am happy to see this getting in. 🎉

response(gsonDeserializer(gson), handler)

/**
* Asynchronously gets a response object of [T] or [com.github.kittinunf.fuel.core.FuelError] via [handler]
*
Expand All @@ -30,6 +40,17 @@ inline fun <reified T : Any> Request.responseObject(noinline handler: ResponseRe
inline fun <reified T : Any> Request.responseObject(handler: ResponseHandler<T>) =
response(gsonDeserializer(), handler)

/**
* Asynchronously gets a response object of [T] or [com.github.kittinunf.fuel.core.FuelError] via [handler]
* using custom [Gson] instance
*
* @param gson [Gson] custom Gson deserializer instance
* @param handler [Handle<T>] the handler that is called upon success
* @return [CancellableRequest] request that can be cancelled
*/
inline fun <reified T : Any> Request.responseObject(gson: Gson, handler: ResponseHandler<T>) =
lostintime marked this conversation as resolved.
Show resolved Hide resolved
response(gsonDeserializer(gson), handler)

/**
* Synchronously get a response object of [T]
*
Expand All @@ -38,26 +59,42 @@ inline fun <reified T : Any> Request.responseObject(handler: ResponseHandler<T>)
inline fun <reified T : Any> Request.responseObject() =
response(gsonDeserializer<T>())

/**
* Synchronously get a response object of [T]
*
* @param gson [Gson] custom Gson deserializer instance
* @return [Triple<Request, Response, Result<T, FuelError>>] the deserialized result
*/
inline fun <reified T : Any> Request.responseObject(gson: Gson) =
response(gsonDeserializer<T>(gson))

/**
* Generate a [ResponseDeserializable<T>] that can deserialize json of [T]
*
* @return [ResponseDeserializable<T>] the deserializer
*/
inline fun <reified T : Any> gsonDeserializerOf(clazz: Class<T>) = object : ResponseDeserializable<T> {
override fun deserialize(reader: Reader): T? = Gson().fromJson<T>(reader, clazz)
}
inline fun <reified T : Any> gsonDeserializerOf(clazz: Class<T>) = gsonDeserializer<T>()

inline fun <reified T : Any> gsonDeserializer() = object : ResponseDeserializable<T> {
override fun deserialize(reader: Reader): T? = Gson().fromJson<T>(reader, object : TypeToken<T>() {}.type)
inline fun <reified T : Any> gsonDeserializer(gson: Gson = Gson()) = object : ResponseDeserializable<T> {
override fun deserialize(reader: Reader): T? = gson.fromJson<T>(reader, object : TypeToken<T>() {}.type)
}

/**
* Serializes [src] to json and sets the body as application/json
*
* @param src [Any] the source to serialize
* @param gson [Gson] custom Gson deserializer instance
* @return [Request] the modified request
*/
inline fun <reified T : Any> Request.jsonBody(src: T) =
this.jsonBody(Gson().toJson(src, object : TypeToken<T>() {}.type)
inline fun <reified T : Any> Request.jsonBody(src: T, gson: Gson) =
this.jsonBody(gson.toJson(src, object : TypeToken<T>() {}.type)
.also { Fuel.trace { "serialized $it" } } as String
)
)

/**
* Serializes [src] to json and sets the body as application/json
*
* @param src [Any] the source to serialize
* @return [Request] the modified request
*/
inline fun <reified T : Any> Request.jsonBody(src: T) = this.jsonBody(src, Gson())
58 changes: 58 additions & 0 deletions fuel-gson/src/test/kotlin/com/github/kittinunf/fuel/FuelGsonTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,21 @@ import com.github.kittinunf.fuel.gson.responseObject
import com.github.kittinunf.fuel.test.MockHttpTestCase
import com.github.kittinunf.fuel.test.MockReflected
import com.google.gson.Gson
import com.google.gson.GsonBuilder
import com.google.gson.JsonDeserializationContext
import com.google.gson.JsonDeserializer
import com.google.gson.JsonElement
import com.google.gson.JsonPrimitive
import com.google.gson.JsonSerializationContext
import com.google.gson.JsonSerializer
import com.google.gson.reflect.TypeToken
import org.hamcrest.CoreMatchers.equalTo
import org.hamcrest.CoreMatchers.not
import org.hamcrest.CoreMatchers.notNullValue
import org.junit.Assert.assertThat
import org.junit.Assert.fail
import org.junit.Test
import java.lang.reflect.Type
import java.net.HttpURLConnection

private typealias IssuesList = List<IssueInfo>
Expand All @@ -30,6 +38,32 @@ private data class IssueInfo(
fun specialMethod() = "$id: $title"
}

private sealed class IssueType {
object Bug : IssueType()
object Feature : IssueType()

companion object : JsonSerializer<IssueType>, JsonDeserializer<IssueType> {
override fun serialize(src: IssueType, typeOfSrc: Type, context: JsonSerializationContext): JsonElement =
when (src) {
is Bug -> JsonPrimitive("bug")
is Feature -> JsonPrimitive("feature")
}

override fun deserialize(json: JsonElement, typeOfT: Type?, context: JsonDeserializationContext?): IssueType =
if (json.isJsonPrimitive() && (json as JsonPrimitive).isString) {
when (json.asString) {
"bug" -> Bug
"feature" -> Feature
else -> throw Error("Not a bug or a feature, what is it?")
}
} else {
throw Error("String expected")
}
}
}

private typealias IssueTypeList = List<IssueType>

class FuelGsonTest : MockHttpTestCase() {

data class HttpBinUserAgentModel(var userAgent: String = "")
Expand Down Expand Up @@ -252,4 +286,28 @@ class FuelGsonTest : MockHttpTestCase() {
assertThat(issues.size, equalTo(data.size))
assertThat(issues.first().specialMethod(), equalTo("1: issue 1"))
}

@Test
fun testCustomGsonInstance() {
val gson = GsonBuilder()
.registerTypeAdapter(IssueType::class.java, IssueType.Companion)
.create()

val data = listOf(
IssueType.Bug,
IssueType.Feature
)

val (_, _, result) = reflectedRequest(Method.POST, "json-body")
.jsonBody(data, gson)
.responseObject<MockReflected>()

val (reflected, error) = result
val body = reflected!!.body!!.string!!
val types: IssueTypeList = gson.fromJson(body, object : TypeToken<IssueTypeList>() {}.type)
assertThat("Expected types, actual error $error", types, notNullValue())
assertThat(body, equalTo("[\"bug\",\"feature\"]"))
assertThat(types.size, equalTo(data.size))
assertThat(types.first(), equalTo<IssueType>(IssueType.Bug))
}
}