Feature application interceptors #428

VisheshVadhera · 2017-04-18T10:09:25Z

Closes #346

… feature-extensible-interceptors

sav007 · 2017-04-18T20:46:54Z

apollo-runtime/src/main/java/com/apollographql/apollo/ApolloClient.java

@@ -84,6 +88,7 @@ private ApolloClient(Builder builder) {
    this.defaultHttpCacheControl = builder.defaultHttpCacheControl;
    this.defaultCacheControl = builder.defaultCacheControl;
    this.logger = builder.apolloLogger;
+    this.applicationInterceptors = builder.applicationInterceptors;


~~pls wrap this into Collections.unmodifiableList() or even earlier in ApolloClient.Builder~~

Never mind we don't set in builder the list of interceptors but add them one by one

Well, the initial thought was to wrap it in an unmodifiable list, but then I realized that we are not exposing the list of interceptors in any way to the clients, so it might as well just remain a simple List.

sav007 · 2017-04-18T20:55:34Z

apollo-runtime/src/main/java/com/apollographql/apollo/internal/RealApolloCall.java

+        applicationInterceptors);
+  }
+
+  private List<ApolloInterceptor> getInterceptors() {


just a nit: we usually omit get prefixes. So I guess we can make this method to return RealApolloInterceptorChain so to have smth like this:

private ApolloInterceptorChain prepareInterceptorChain(Operation operation, List< ApolloInterceptor> applicationInterceptors) { ... return new RealApolloInterceptorChain(operation, interceptors); }

sav007 · 2017-04-18T20:58:10Z

apollo-integration/src/test/java/com/apollographql/apollo/InterceptorTest.java

+
+  @Test
+  public void syncApplicationInterceptorCanShortCircuitResponses() throws IOException, ApolloException {
+    mockWebServer.shutdown();


Do we need this?

Yeah, we do need this, so as to ensure that response is indeed the intercepted response and not from the server.

sav007 · 2017-04-18T21:00:23Z

apollo-integration/src/test/java/com/apollographql/apollo/InterceptorTest.java

+        .build();
+  }
+
+  @NonNull private InterceptorResponse getInterceptorResponse(EpisodeHeroName query) {


nit: we don't like get prefixes :)

sav007 · 2017-04-18T21:04:08Z

apollo-integration/src/test/java/com/apollographql/apollo/InterceptorTest.java

+
+    Response<EpisodeHeroName.Data> actualResponse = client.newCall(query).execute();
+
+    assertThat(expectedResponse.parsedResponse.get()).isEqualTo(actualResponse);


would be nice if we check that other interceptors aren't called.

I am wondering how can we test that the other interceptors aren't called? Maybe check that the httpResponse & cachedResponse are equal to the fake ones that we pass to the interceptedResponse? But that would tell us nothing about other interceptors being called or not called.

sav007

Just minor comments.
Overall we don't really use get prefixes.
Maybe test should go to apollo-runtime itself as it more junit test then integration (it doesn't need to run apollo compiler etc)

VisheshVadhera · 2017-04-19T06:00:51Z

@sav007 It would be difficult to move this class to the apollo-runtime module because InterceptorTest class needs objects like NamedCountdownLatch & EpisodeHeroName which are only available in the apollo-integration module. Moving it to apollo-runtime would mean adding apollo-integration as a dependency for apollo-runtime which we don't want.

sav007

Pls change withCustomTypeAdapter to addCustomTypeAdapter and applicationInterceptor to addApplicationInterceptor in ApolloClient builder be consistent and that is what OkHttp.Builder does.

…adhera/apollo-android into feature-extensible-interceptors

VisheshVadhera · 2017-04-19T20:59:14Z

@sav007 Changed the method names as requested. If everything else looks fine, we can go ahead and merge it.

VisheshVadhera · 2017-04-19T21:14:08Z

@sav007 Oh btw, do you have any inputs on how we can test that the rest of the interceptors are not called when we short circuit responses?

sav007 · 2017-04-19T21:28:09Z

create a unit tests for RealApolloInterceptorChain add couple interceptors and make one of them to return value right away. Then callinterceptorChain.proceed() and check if only those interceptors who supposed to be are called

… feature-extensible-interceptors

…short circuiting

VisheshVadhera · 2017-04-20T11:19:54Z

@sav007 Added the test which checks that other interceptors are not called when one of the the interceptors short circuits the response. Also made changes to the docs of the corresponding builder method to reflect the same.

sav007 · 2017-04-20T15:15:22Z

@VisheshVadhera sorry to request so much, but could you please add additional test to verify if com.apollographql.apollo.interceptor.ApolloInterceptorChain#dispose is called for application interceptors as well.

VisheshVadhera · 2017-04-20T15:17:33Z

Np, I'll be happy to add them, the more tests we have the better it is for the project.

…

On 20 Apr 2017 8:45 pm, "Ivan Savytskyi" ***@***.***> wrote: @VisheshVadhera <https://github.com/VisheshVadhera> sorry to request so much, but could you please add additional test to verify if com.apollographql.apollo.interceptor.ApolloInterceptorChain#dispose is called for application interceptors as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#428 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD1DEA9xf0ubMM68j9-f1TURW5VlZ04Jks5rx3aLgaJpZM4NAI6_> .

VisheshVadhera · 2017-04-20T17:56:22Z

@sav007 Added a test case for the dispose method, plus a couple more for asyncInterceptors. Also I realized we don't have tests for ApolloInterceptorChain. Want me to open another PR?

digitalbuddha · 2017-04-20T17:57:03Z

lgtm

digitalbuddha · 2017-04-20T18:04:36Z

Yup please do that in the next pr to keep this one from growing anymore

… feature-extensible-interceptors

sav007 · 2017-04-21T22:56:56Z

Merging?

digitalbuddha · 2017-04-21T23:06:33Z

Lgtm

…

On Apr 21, 2017 6:56 PM, "Ivan Savytskyi" ***@***.***> wrote: Merging? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#428 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEUX3MBiNaSwew37DkD8nX-NnyLFWrQVks5ryTQ5gaJpZM4NAI6_> .

VisheshVadhera added 20 commits April 16, 2017 17:37

Add interceptors to the ApolloClient

f39d01a

Add network interceptors to the ApolloClient

88ee9f1

Add interceptorTest class

66fa18a

Docs for interceptor methods

211f7a3

Merge branch 'master' of github.com:apollographql/apollo-android into…

6cbdbbc

… feature-extensible-interceptors

First test for Interceptor. Yay!

4d8af52

Delete networkInterceptors

286a11e

Add rewrite response test

1d69254

Add tests for async interceptors

488df3c

Add test for multiple requests

1d83fe4

Fix a failing test

ec58cc5

Add test for runtime exception

58f745e

TODO: Write the remaining two tests

bc9b016

Reformat the order of tests

4f806a7

Merge branch 'master' of github.com:apollographql/apollo-android into…

abecfc4

… feature-extensible-interceptors

Fix space

dbd7718

Fix codestyle

641e739

Fix codestyle

4669d55

shutdown mock webserver on teardown

5f804a5

Refactor code

b2ac0e7

sav007 reviewed Apr 18, 2017

View reviewed changes

Code review

37112cc

sav007 approved these changes Apr 19, 2017

View reviewed changes

Change customTypeAdapter & applicationInterceptor builder method names

a12e264

Merge branch 'feature-extensible-interceptors' of github.com:VisheshV…

0aa0340

…adhera/apollo-android into feature-extensible-interceptors

VisheshVadhera added 4 commits April 20, 2017 16:08

Merge branch 'master' of github.com:apollographql/apollo-android into…

efdf9db

… feature-extensible-interceptors

Rename the test class

851f994

Add test which checks that subsequent interceptors are not called on …

a5858da

…short circuiting

Improve the documentation of the interceptor builder method

e0df529

VisheshVadhera added 3 commits April 20, 2017 22:21

Add test for dispose

38b4210

Add NPE & RuntimeException tests for async interceptor

dc684fe

Replace builder with apolloClient creator method

7e484ca

Merge branch 'master' of github.com:apollographql/apollo-android into…

a43bd0a

… feature-extensible-interceptors

sav007 merged commit 367104e into apollographql:master Apr 21, 2017

VisheshVadhera deleted the feature-extensible-interceptors branch May 4, 2017 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature application interceptors #428

Feature application interceptors #428

VisheshVadhera commented Apr 18, 2017

sav007 Apr 18, 2017 •

edited

Loading

sav007 Apr 18, 2017

VisheshVadhera Apr 19, 2017

sav007 Apr 18, 2017

sav007 Apr 18, 2017

VisheshVadhera Apr 19, 2017

sav007 Apr 18, 2017

sav007 Apr 18, 2017

VisheshVadhera Apr 19, 2017 •

edited

Loading

sav007 left a comment

VisheshVadhera commented Apr 19, 2017 •

edited

Loading

sav007 left a comment

VisheshVadhera commented Apr 19, 2017

VisheshVadhera commented Apr 19, 2017

sav007 commented Apr 19, 2017

VisheshVadhera commented Apr 20, 2017

sav007 commented Apr 20, 2017

VisheshVadhera commented Apr 20, 2017 via email

VisheshVadhera commented Apr 20, 2017

digitalbuddha commented Apr 20, 2017

digitalbuddha commented Apr 20, 2017

sav007 commented Apr 21, 2017

digitalbuddha commented Apr 21, 2017 via email


		Response<EpisodeHeroName.Data> actualResponse = client.newCall(query).execute();

		assertThat(expectedResponse.parsedResponse.get()).isEqualTo(actualResponse);

Feature application interceptors #428

Feature application interceptors #428

Conversation

VisheshVadhera commented Apr 18, 2017

sav007 Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

sav007 Apr 18, 2017

Choose a reason for hiding this comment

VisheshVadhera Apr 19, 2017

Choose a reason for hiding this comment

sav007 Apr 18, 2017

Choose a reason for hiding this comment

sav007 Apr 18, 2017

Choose a reason for hiding this comment

VisheshVadhera Apr 19, 2017

Choose a reason for hiding this comment

sav007 Apr 18, 2017

Choose a reason for hiding this comment

sav007 Apr 18, 2017

Choose a reason for hiding this comment

VisheshVadhera Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

sav007 left a comment

Choose a reason for hiding this comment

VisheshVadhera commented Apr 19, 2017 • edited Loading

sav007 left a comment

Choose a reason for hiding this comment

VisheshVadhera commented Apr 19, 2017

VisheshVadhera commented Apr 19, 2017

sav007 commented Apr 19, 2017

VisheshVadhera commented Apr 20, 2017

sav007 commented Apr 20, 2017

VisheshVadhera commented Apr 20, 2017 via email

VisheshVadhera commented Apr 20, 2017

digitalbuddha commented Apr 20, 2017

digitalbuddha commented Apr 20, 2017

sav007 commented Apr 21, 2017

digitalbuddha commented Apr 21, 2017 via email

sav007 Apr 18, 2017 •

edited

Loading

VisheshVadhera Apr 19, 2017 •

edited

Loading

VisheshVadhera commented Apr 19, 2017 •

edited

Loading