square / retrofit

A type-safe HTTP client for Android and the JVM
https://square.github.io/retrofit/
Apache License 2.0
43.11k stars 7.3k forks source link

Add Means of Intercepting and Rewiting Requests and Responses Fully #261

Closed JakeWharton closed 9 years ago

JakeWharton commented 11 years ago

This will facilitate signing, changing, replacing, etc. See #197 and #224 for more things it will need to support.

Like dealing with headers in the existing implementation, these have a want of being both global and local to an API method which, while not difficult, clutters the APIs.

Draft:

// Read the content of a request and add a signing header.
@POST("/foo")
@RequestInterceptor(OauthSigningInterceptor.class)
Response doSomething();
// Grab child JSON from the response (e.g., {"data": {realdata}}) and use it for deserialization.
@GET("/foo")
@ResponseInterceptor(DeclutterInterceptor.class)
Foo doSomething();
// Set global interceptors for adding auth headers, etc.
RestAdapter ra = new RestAdpater.Builder()
    .setGlobalRequestInterceptor(..)
    .setGlobalResponseInterceptor(..)
    .build()
codefromthecrypt commented 11 years ago

would these be an array? I presume this is a method (and not also a type) annotation?

swankjesse commented 11 years ago

Could the interceptor handle both? Guice's AOP interceptors look like this:

  class LoggingInterceptor implements Interceptor {
    Response intercept(Request request, Call call) {
      log("request = " + request);
      Response response = call.proceed(request);
      log("response = " + response);
      return response;
    }
  }
codefromthecrypt commented 11 years ago

yeah I suppose it could, where s/Call/Client

SeanPONeil commented 11 years ago

If there's a global interceptor set on the RestAdapter as well as an interceptor local to an API method, in what order would they get executed?

stephanenicolas commented 11 years ago

I hope I am not just adding noise here as I didn't follow all the thread, but I just face a problem where I would like to know the headers' content and get my object parsed by retrofit, and found this issue.

@SeanPONeil another possibility would be that the local one would completely override the global one. I got the feeling that I would prefer this option. If the local interceptor wants to get some behavior from the global one, then it should just extend from it or delegate some methods to an instance of it.

BTW, wouldn't it be possible to add the global interceptor to the interface itself via an annotation ? Same for converter, error handler and so on ?

JakeWharton commented 11 years ago

Using annotations prohibits custom instance creation. For the converter example, how do I get GsonConverter to use my own Gson object with a ton of custom TypeAdapters. The only solution we were able to come up with was to have a registry where you map converter classes to instances. This ultimately didn't amount to less code because you still supply your own instance at the builder but now are required to annotate every method. Besides, in practice, how many APIs are using multiple serialization formats. We currently talk to JSON and proto backends and simply use two RestAdapter instances that share a client and executor.

stephanenicolas commented 11 years ago

Ok for the last point Jake, I was more thinking in terms of homogeneity than real use cases. It would still be possible to get a specific class and use it via annotation to achieve the same as the customization of a specific instance, but that would be quite a constraint. So drop the last line of my previous post, that was not my main point.

luke-han commented 10 years ago

Is this going to support the asynchronous RequestInterceptor? I asked it before in the Google+ community.

dannyroa commented 10 years ago

@JakeWharton: I can't find the method setGlobalResponseInterceptor you referenced at the start of this thread. Is it deprecated? Thanks.

JakeWharton commented 10 years ago

It doesn't exist. This is a proposal.

letroll commented 10 years ago

does anyone know if there is something new for the method setGlobalResponseInterceptor? I want to use it to extract the data in my response, as indicated by @adriancole in https://github.com/square/retrofit/issues/197

artworkad commented 10 years ago

WOW, need this :+1:

hubsmoke commented 10 years ago

It would be nice to preprocess the response in order to create and return a more convenient/different Java object than what is determined by the structure of the response. However, the workaround of creating a wrapper class isn't bad

For instance:

interface MyApi {
  @GET("/something")
  VerboseComplicated<Something<String>> getSomething();
}

It would be nice to be able to do instead:

interface MyApi {
  @GET("/something")
  @RequestInterceptor(VerboseComplicatedSomethingStringInterceptor.class)
  String getSomething();
}

But I'm fine doing the former and just making another class that would be exposed to consumers of the API:

class MyEasyApi {
  private RestAdapter restAdapter;
  public MyEasyApi(String url) {
    restAdapter = new RestAdapter.Builder().setEndpoint(url).build();
  }
  String getSomething() {
    return restAdapter.getSomething().getAsSomething().getAsString();
  }
}

A disadvantage of doing it this way is that if the interface is refactored, then so will the wrapping class and vice versa. Another disadvantage is that you have to include all the VerboseComplicated<Something<T>> classes in your project, rather than deserializing them manually directly from JSON. However, I suppose you could always just parse the TypedInput from the Response object and get it that way. In all, it's not even that big of a deal for smaller APIs, but I could see it creating lots of annoying boilerplate for big APIs that all have a { useless: { root: 'element' } }

YuraLaguta commented 10 years ago

+1 Need this, is someone already working on it?

JakeWharton commented 10 years ago

Yes.

On Thu, Oct 16, 2014 at 3:54 PM, Yurii notifications@github.com wrote:

+1 Need this, is someone already working on it?

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/261#issuecomment-59443629.

rifatdover commented 10 years ago

:+1: Need it also :)

2dxgujun commented 10 years ago

+1

Christophe668 commented 9 years ago

+1

Cameronjmayfield commented 9 years ago

+1

LisaAbate commented 9 years ago

+1

ghost commented 9 years ago

+1, I think this can be pretty usefull, espacially when authentication need body content to generate security token.

JakeWharton commented 9 years ago

That's an HTTP client interceptor job, not Retrofit's job. You'll be able to do that with OkHttp 2.2. On Dec 19, 2014 9:36 AM, "ingeniwi" notifications@github.com wrote:

+1, I think this can be pretty usefull, espacially when authentication need body content to generate security token.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/261#issuecomment-67669946.

ghost commented 9 years ago

Thx for the update, so from now I guess this is not possible to do such with OkHttp 2.1.0 ?

JakeWharton commented 9 years ago

You'd need to use the version on the master branch of OkHttp for interceptors.

On Sat Dec 20 2014 at 2:55:59 AM ingeniwi notifications@github.com wrote:

Thx for the update, so from now I guess this is not possible to do such with OkHttp 2.1.0 ?

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/261#issuecomment-67732112.

ghost commented 9 years ago

+1

smaspe commented 9 years ago

That's an HTTP client interceptor job, not Retrofit's job. You'll be able to do that with OkHttp 2.2.

Right, but having some information regarding the request, may be via annotations, would be nice, as not all requests need the same kind of authentication (e.g. a request requesting a token vs a request using the token as auth).

This is what looked nice in the @RequestInterceptor(OauthSigningInterceptor.class) example, is that each request can specify its mean of authentication.

CateyesLin commented 9 years ago

+1 I need to sign request before it really send.

JakeWharton commented 9 years ago

Use OkHttp interceptors for that!

On Wed, May 27, 2015, 11:49 PM Cateyes notifications@github.com wrote:

+1 I need to sign request before it really send.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/261#issuecomment-106198393.

CateyesLin commented 9 years ago

I need more complete request headers, post body to sign. The RequestInterceptor.RequestFacade cannot do this.

jdreesen commented 9 years ago

Not retrofit.RequestInterceptor.RequestFacade but OkHttp interceptors ;)

CateyesLin commented 9 years ago

Oh, Great! I'll take a look.

BTW There is still legacy examples on http://square.github.io/retrofit/.

JakeWharton commented 9 years ago

Legacy how? It reflects the released version.

On Thu, May 28, 2015, 12:30 AM Cateyes notifications@github.com wrote:

Oh, Great! I'll take a look.

BTW There still legacy examples on http://square.github.io/retrofit/.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/261#issuecomment-106209577.

CateyesLin commented 9 years ago

Sorry I saw master branch instead of tag 1.9.0

JakeWharton commented 9 years ago

Rewriting is for OkHttp interceptors. For modifying objects coming in and out there #816.