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.

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

Adding integration for Retrofit #1037

Merged
merged 20 commits into from Oct 24, 2018

Conversation

Projects
None yet
3 participants
@leandroBorgesFerreira
Collaborator

leandroBorgesFerreira commented Oct 9, 2018

This PR is a adapter for Arrow's Async classes.

Example of use:

interface ApiClientTest {
  @GET("test")
  fun test(): CallK<ResponseMock>

  @GET("testIO")
  fun testIO(): IO<ResponseMock>
}

Using this plugin, it will be possible to set IO as a return type for a endpoint. It will be also possible the set CallK to create polymorphic return types that can be concretised in a latter moment.

leandroBorgesFerreira added some commits Oct 9, 2018

@codecov

This comment has been minimized.

codecov bot commented Oct 9, 2018

Codecov Report

Merging #1037 into master will increase coverage by 0.12%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1037      +/-   ##
============================================
+ Coverage     42.66%   42.79%   +0.12%     
- Complexity      737      746       +9     
============================================
  Files           354      359       +5     
  Lines          9787     9823      +36     
  Branches       1061     1065       +4     
============================================
+ Hits           4176     4204      +28     
- Misses         5298     5303       +5     
- Partials        313      316       +3
Impacted Files Coverage Δ Complexity Δ
.../integrations/retrofit/adapter/ResponseCallback.kt 100% <100%> (ø) 3 <3> (?)
...egrations/retrofit/adapter/CallKind2CallAdapter.kt 100% <100%> (ø) 3 <3> (?)
...rations/retrofit/adapter/CallKindAdapterFactory.kt 100% <100%> (ø) 0 <0> (?)
...tlin/arrow/integrations/retrofit/adapter/syntax.kt 41.66% <41.66%> (ø) 0 <0> (?)
...otlin/arrow/integrations/retrofit/adapter/CallK.kt 75% <75%> (ø) 3 <3> (?)
...ics/src/main/kotlin/arrow/optics/instances/mapk.kt 86.36% <0%> (-4.55%) 0% <0%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da9b48a...9ac5402. Read the comment docs.

import retrofit2.Response
data class CallK<R>(val call: Call<R>) {
fun <F> async(async: Async<F>): Kind<F, R> =

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

I'd replace this version with the one that uses Kind<F, Response<R>> as the return type. I wouldn't impose the semantics of handleResponse because you don't give users the chance to inspect other fields of the response that may be relevant.

private fun <R> handleResponse(response: Response<R>) =
if (response.isSuccessful) {
response.body() ?: throw IllegalStateException("The request returned a null body")

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

Don't throw, use raiseException instead.

This comment has been minimized.

@leandroBorgesFerreira

leandroBorgesFerreira Oct 10, 2018

Collaborator

Do you mean raiseError? I don't see a raiseException in the code.

But, anyway, then I would need to wrap an then unwrap the result to conform with the signature:

invoke(f: () -> A)

defer is already catching any kind of Throwable. Wouldn't throw make the code simpler?

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

Then, in this case defer should behave differently than catch. Defer returns a value, catch will throw.

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

catch does not return a kinded value but deffer does. In whatever case, we should not throw exceptions since these integrations are used by others as an example and Arrow provides mechanisms to avoid throwing exceptions and instead of favors representing them as value. You don't need catch wrapping those if you just call raiseError instead of throwing exceptions.

import retrofit2.Response
class ResponseCallback<R>(private val proc: (Either<Throwable, R>) -> Unit) : Callback<R> {
override fun onResponse(call: Call<R>, response: Response<R>) {

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

Same as mentioned above, I wouldn't unpack the Response<R> and would let users decide instead. This can be an extension function instead, something like fun <R> Response<R>.unwrapBody(): Kind<F, R>.

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

The use would be: getNetworkRequest("Bla").async(IO.async()).flatMap { it.unwrapBody(IO.monadError()) }

This comment has been minimized.

@leandroBorgesFerreira

leandroBorgesFerreira Oct 10, 2018

Collaborator

You're right, I am imposing an semantics on the client and we should let then decide between Response<R> and just R. But there's no need to create an extension function for this, or to change the API. The API could be the same, we just need the to read the ReturnType at the Async2CallAdapterFactory and then create the right adapter for each response. If one uses CallK him/she gets the current solution, but if he/she uuses CallK<Response> him/she gets yours. WDYT?

This comment has been minimized.

@leandroBorgesFerreira

leandroBorgesFerreira Oct 10, 2018

Collaborator

That's pretty much how the RxJava2 plugin works

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

My concern is with discoverability, as it won't be immediately apparent that CallK<Response<X>> has different semantics than any other value. I'd rather it was a different function of the CallK API, like asyncResponse for example.

@RunWith(KTestJUnitRunner::class)
class Async2CallAdapterFactoryTest : UnitSpec() {
private val retrofit = retrofit(HttpUrl.parse("http://localhost:1")!!)

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

This can be cached outside of the test, same as NO_ANNOTATION.

@@ -0,0 +1,19 @@
dependencies {
def retrofitVersion = "2.4.0"

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

I'd move this to the main build.gradle.

compile project(':arrow-effects')
compile "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlinVersion"
compile project(':arrow-annotations')
compile "com.squareup.retrofit2:retrofit:2.4.0"

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

Use retrofitVersion here.

@pakoito

I'd say split the API into returning a Kind<F, Response<R>> from a CallK and an extfun to unpack Response<R> into a Kind<F, R>. One is a general solution, the other one depends on your context so it's a helper.

if (body != null) {
proc(body.right())
} else {
proc(IllegalStateException("The request returned a null body").left())

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

I've been thinking about it. Doesn't POST and HEAD request imply the possibility of an empty body? Or does retrofit return null for failures to deserialize? Maybe it should be R? or Option<R> instead.

This comment has been minimized.

@leandroBorgesFerreira

leandroBorgesFerreira Oct 10, 2018

Collaborator

Post would be an IO or CallK. It wouldn't return null, it would return Unit. Error to deserialize go straight to onFailure. But I should include tests for those cases

@@ -0,0 +1,38 @@
package arrow.integrations.retrofit.adapter

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

For what I can see in the file name this is in a folder with . but each one of the sub packages should be on its own folder

class Async2CallAdapterFactory : CallAdapter.Factory() {
companion object {
fun create() = Async2CallAdapterFactory()

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

All return types in Arrow are always fully typed and we don't rely on inference.

}
}
private fun <R> handleResponse(response: Response<R>) =

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

type return types of all functions

private fun <R> handleResponse(response: Response<R>) =
if (response.isSuccessful) {
response.body() ?: throw IllegalStateException("The request returned a null body")

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

catch does not return a kinded value but deffer does. In whatever case, we should not throw exceptions since these integrations are used by others as an example and Arrow provides mechanisms to avoid throwing exceptions and instead of favors representing them as value. You don't need catch wrapping those if you just call raiseError instead of throwing exceptions.

import retrofit2.CallAdapter
import java.lang.reflect.Type
class IO2CallAdapter<R>(private val type: Type) : CallAdapter<R, IO<R>> {

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

I believe you can define these adapters polymorphically so they can support a lot more data types than just Call and IO. Something along the lines of:

class GenericCallAdapter<F, A>(private val type: Type, private val delay: Async<F>) : CallAdapter<A, Kind<F, A>>, Async<F> by delay {
  override fun adapt(call: Call<A>): Kind<F, A> = async { proc -> call.enqueue(ResponseCallback(proc)) }
   override fun responseType(): Type = type
}

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

You may then make them concrete where needed by passing in the async instance of each of the data types you wish to support. With a generic adapter you may already be supporting Observable, Flux, Deferred and many others

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

That's what the other adapter, CallK, does. We cannot create and register an adapter because retrofit is implemented by doing runtime checks on a Class return using reflection, and it doesn't get all the generics we need for us to create our own internally. CallK fixes that, check the code.

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

If CallK is already polymorphic why do we need then an IO call adapter? Couldn't the user use the CallK adapter to get back responses on IO?

This comment has been minimized.

@pakoito

pakoito Oct 10, 2018

Member

To avoid the extra step if users want to. I'm on the fence about it because we have to decide which semantics we'll need to enforce for it.

This comment has been minimized.

@raulraja

raulraja Oct 10, 2018

Member

The way I see it this needs arrow-retrofit-coroutines, arrow-retrofit-rx2, etc. for each one of the currently supported effect types and with those modules you get the specialization for the runtime you are gonna support

This comment has been minimized.

@leandroBorgesFerreira

leandroBorgesFerreira Oct 10, 2018

Collaborator

My idea for the IO adapter is to create a way for a developer be able to use the plugin without understanding how to create polymorphic code, which is not a very simple concept.
I don't mind removing the adapter, but we should keep in mind that documentation must teach how to concretise the type for someone that is not very used to advanced concepts.

@leandroBorgesFerreira

This comment has been minimized.

Collaborator

leandroBorgesFerreira commented Oct 10, 2018

Shouldn't have pushed code... Still working on the solution.

@raulraja

This comment has been minimized.

Member

raulraja commented Oct 10, 2018

@leandroBorgesFerreira no worries, we will squash and merge when ready, intermediate broken commits are fine.

@leandroBorgesFerreira

This comment has been minimized.

Collaborator

leandroBorgesFerreira commented Oct 11, 2018

I believe I took care of all the observations. Let me know if there's still more to do

pakoito and others added some commits Oct 11, 2018

@pakoito

LGTM, it turned out juust right :)

fun <F> catch(monadError: MonadError<F, Throwable>): Kind<F, Response<R>> = call.runSyncCatch(monadError)
}
fun <F, A> Call<A>.runInAsyncContext(AC: Async<F>): Kind<F, Response<A>> =

This comment has been minimized.

@pakoito

pakoito Oct 19, 2018

Member

Move these to Ext.kt probably.

@@ -0,0 +1,18 @@
package arrow.integrations.retrofit.adapter

This comment has been minimized.

@pakoito

pakoito Oct 19, 2018

Member

Worth renaming to syntax.kt to be consistent

@pakoito

This comment has been minimized.

Member

pakoito commented Oct 19, 2018

Approved from me, with one suggestion to move some methods. @raulraja needs to approve it too.

We can do the docs later :)

@raulraja

Awesome!, we should do something similar for ktor soon.
Please do not merge this until I give the go ahead as this is going to need a rebase from master once I merge #1040
thanks!

@leandroBorgesFerreira

This comment has been minimized.

Collaborator

leandroBorgesFerreira commented Oct 19, 2018

No problem. Won't merge

@raulraja

This comment has been minimized.

Member

raulraja commented Oct 19, 2018

@leandroBorgesFerreira This is ready to go once you rebase from master

leandroBorgesFerreira and others added some commits Oct 19, 2018

apError.raiseError(HttpException(this))
}
fun <F, A> Call<A>.runInAsyncContext(AC: Async<F>): Kind<F, Response<A>> =

This comment has been minimized.

@pakoito

pakoito Oct 22, 2018

Member

runAsync? I believe this name is a holdover from when Async was called AsyncContext. Have you seen it somewhere else?

This comment has been minimized.

@leandroBorgesFerreira

leandroBorgesFerreira Oct 22, 2018

Collaborator

I just didn't know any good name haha. runAsync looks good

import java.lang.reflect.ParameterizedType
import java.lang.reflect.Type
class Async2CallAdapterFactory : CallAdapter.Factory() {

This comment has been minimized.

@pakoito

pakoito Oct 22, 2018

Member

Now that we don't do IO, what about just CallKindAdapterFactory?

leandroBorgesFerreira added some commits Oct 22, 2018

@leandroBorgesFerreira leandroBorgesFerreira merged commit 85e6260 into arrow-kt:master Oct 24, 2018

3 checks passed

codecov/patch 77.77% of diff hit (target 42.66%)
Details
codecov/project 42.79% (+0.12%) compared to da9b48a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@raulraja raulraja referenced this pull request Nov 2, 2018

Merged

Release 0.8.0 #1080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment