Change CacheControl to Interceptor based Fetcher #556

BenSchwab · 2017-07-10T17:36:22Z

Motivation

The interceptor architecture we use for executing a request is loosely similar to the future direction Apollo JS is going: https://github.com/apollographql/apollo-fetch. If I understand correctly, the vision is that Apollo Fetch will make it easy to write fetch policies like CACHE_FIRST or more complicated policies like CACHE_FIRST_IF_NOT_STALE_BUT_IF_NO_NETWORK_ALLOW_STALE.

However, shipping with every possible fetch strategy is impossible -- thus our current CacheControl approach based on a set of enums, could be made more flexible. This PR proposes making CacheControl an ApolloInterceptor that contains logic of how to fetch the request. (And changes the name to Fetcher to better align with JS).

Fetcher

Just an ApolloInterceptor
Uses FetchOptions to configure fetching preferences
Can change CacheHeaders and a boolean fetchFromCache to control how the data will be fetched
Thus CACHE_FIRST is implemented by first proceeding the chain with a FetchOptions with fetchFromCache = true and catching an exception. If exception it will try to proceed with fetchFromCache = false

Benefits

ApolloCacheInterceptor has less logic - all fetching logic is contained in the Fetcher
Fairly easy to implement a custom Fetcher for specific needs
In basic use case, API is no more complicated

 githuntFeedCall = application.apolloClient()
        .query(feedQuery)
        .fetcher(ApolloFetchers.CACHE_FIRST);

Mutli-response

Notably this PR enables a fetcher to respond multiple times. The rationale behind this is that when you use Apollo with a persistent sql cache, currently the only fetch policy that works is NETWORK_FIRST which negates the value of the sql / in memory cache. This is because if you use CACHE_FIRST, after the first cache population, the data will never be automatically refreshed. This could be handled with some sort of "staleness" system, but again, this would result in the cache data always being held until it is stale. Rather, it would be useful to allow a request to fulfill from cache first, and then also, fetch updated data from the network.

This does complicate the callback API, particularly with ApolloRx, as we need to know when a request will return no more data (onCompleted).

This PR ships with one double response fetch policy CACHE_AND_NETWORK.

Additional work

Integrate with ApolloSupport
Add tests
Add documentation
Make sure cancel and dispose works properly

Previous discussions: https://airbnb.quip.com/BVbnAztIGoLm

closes #432

BenSchwab · 2017-07-10T17:37:57Z

cc @kompfner @martijnwalraven for thoughts from iOS perspective

BenSchwab · 2017-07-10T17:41:35Z

@sav007 @digitalbuddha Let me know what you think about going in this direction, if things seem good from a conceptual perspective, I'll move forward with writing all the tests, documentation, and code cleanup

sav007 · 2017-07-10T21:13:19Z

I love this idea, clean and flexible

sav007 · 2017-07-10T21:14:23Z

apollo-runtime/src/main/java/com/apollographql/apollo/fetcher/Fetcher.java

+
+public interface Fetcher {
+
+  ApolloInterceptor create(ApolloLogger logger);


should it be renamed for clarity to smth provideInterceptor?

sav007 · 2017-07-10T21:17:38Z

apollo-runtime/src/main/java/com/apollographql/apollo/fetcher/Fetcher.java

+import com.apollographql.apollo.interceptor.ApolloInterceptor;
+import com.apollographql.apollo.internal.util.ApolloLogger;
+
+public interface Fetcher {


Should it be called ResponseFetcher?

digitalbuddha · 2017-07-10T23:08:01Z

I like the concept but am not crazy with the builder method name

.fetcher(ApolloFetchers.CACHE_FIRST) feels weird. It's not a fetcher but a cache policy or a fetch policy. When I see a builder method called fetcher I expect to pass in something that has an implementation for fetch.

digitalbuddha · 2017-07-10T23:09:42Z

Ok next time I'll look at code and not just description of pr. It does fetch! I'm with Ivan love it.

sav007 · 2017-07-11T02:38:45Z

apollo-runtime/src/main/java/com/apollographql/apollo/fetcher/CacheAndNetworkFetcher.java

+
+  private synchronized void handleNetworkError(ApolloException exception) {
+    networkException = Optional.of(exception);
+    dispatch();


should we consider to not dispatch if result if it was canceled (disposed)

sav007 · 2017-07-11T02:40:52Z

We need to make sure that cancel on RealApolloCall works as expected, all chained interceptors are been disposed

BenSchwab · 2017-07-12T19:55:50Z

Divided this into runtime changes, rx changes, and test changes.

~~Also, took a stab at #560 as this PR required me to address cancellation in the new fetchers.~~

~~To avoid the memory leaks #560 during cancelation a made the callback a WeakReference.~~

I don't think the WeakReference approach is a good idea. It as causing issues in tests, and probably could surface other issues. Let's fix 560 in a separate PR to avoid doing too much here.

There is a slight change in behavior of cancellation:

If cancel is called before enqueue or execute, there will be an exception
If cancel is called during execute, there will be an exception
If cancel is called during enqueue there will be no exception, and callback will not be triggered

The change is in point (3) which would previously trigger onError. This felt weird to me, as if I call cancel I don't expect any callbacks (Rx follows this model).

I also more aggressively check if dispose has been called on interceptors to avoid doing unnecessary work.

sav007 · 2017-07-15T16:29:15Z

apollo-runtime/src/main/java/com/apollographql/apollo/fetcher/ApolloResponseFetcher.java

+import com.apollographql.apollo.internal.fetcher.NetworkFirstFetcher;
+import com.apollographql.apollo.internal.fetcher.NetworkOnlyFetcher;
+
+public final class ApolloResponseFetcher {


nit: should it be named ApolloResponseFetchers, like in java Executors, in Rx Schedulers?

sav007 · 2017-07-15T16:33:48Z

apollo-runtime/src/main/java/com/apollographql/apollo/interceptor/FetchOptions.java

+  public final boolean fetchFromCache;
+  public final CacheHeaders cacheHeaders;
+
+  public static final FetchOptions NETWORK = new FetchOptions(false, CacheHeaders.NONE);


nit: NETWORK_ONLY?

sav007 · 2017-07-15T16:45:27Z

Just curios if FetchOptions is really necessary, is behaviour inside the chained interceptors provided by ResponseFetcher is not enough?

some code review changes fix runtime checkstyle runtime fix remove weak reference changes runtime changes

fixes to rx

remove unused test test fixes test changes mend

BenSchwab · 2017-07-15T21:09:50Z

While, I believe it would be possible to write a custom Fetcher (ApolloInterceptor) that could resolve from network / cache, ideally user's can just proceed with the combo of CacheInterceptor/ParseInterceptor/ServerIntercept that we already assemble. It seemed to me the easiest way to pass a configuration to CacheInterceptor to instruct it how to proceed is an options object with fetchFromCache and CacheHeaders both of which many custom fetch strategies would change to achieve different effects (adding a CacheHeader like "ALLOW_STALE" after an initial cache and network miss)

sav007 · 2017-07-16T03:07:14Z

We should update java doc for Callback: public abstract void onResponse(@Nonnull Response<T> response); to mention that this can be called multiple times, user should be prepared for this

sav007 · 2017-07-16T03:11:44Z

And we need to mention that Callback.onCompleted won't be called in case of error

sav007 · 2017-07-16T03:14:06Z

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

   *
-   * @param cacheControl the CacheControl strategy to set
+   * @param fetcher the {@link ResponseFetcher} to use.
   * @return The ApolloCall object with the provided CacheControl strategy


@return The ApolloCall object with the provided CacheControl strategy ->
@return The ApolloCall object with the provided fetch strategy

sav007

Just couple comments on JavaDocs. Other than that lgtm

sav007 · 2017-07-16T03:16:10Z

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

      return this;
    }

    /**
-     * Set the default {@link CacheHeaders} strategy that will be passed
+     * Set the default {@link ResponseFetcher} that will create a {@link ResponseFetcher} to be passed


smth wrong here ResponseFetcher will create a ResponseFetcher

sav007 · 2017-07-16T03:34:31Z

apollo-rx2support/src/main/java/com/apollographql/apollo/rx2/Rx2Apollo.java

@@ -66,30 +63,39 @@ private Rx2Apollo() {
  }

  /**
-   * Converts an {@link ApolloCall} to a synchronous Single.
+   * Converts an {@link ApolloCall} to an {@link Observable}.


I guess we should provide more info why it's Observable on cache and network fetch strategy example

sav007 · 2017-07-16T03:35:11Z

apollo-rxsupport/src/main/java/com/apollographql/apollo/rx/RxApollo.java

@@ -81,30 +79,50 @@ private RxApollo() {
  }

  /**
-   * Converts an {@link ApolloCall} to a Single.
+   * Converts an {@link ApolloCall} to a Observable.


the same as above

BenSchwab · 2017-07-17T19:44:34Z

@sav007 I guess I should bump the version number again since this is a breaking change? Not sure we have enough new stuff since that last release to publish an official release

sav007 · 2017-07-17T20:51:49Z

but we have't released 0.4.0 yet, why do we need to bump it to 0.5.0?

BenSchwab · 2017-07-17T21:52:04Z

Ok, I wasn't sure if we were trying to keep snapshot builds being semver capatabilble. But if not, let's wait to bump!

This reverts commit f59e15b.

BenSchwab requested review from sav007, digitalbuddha and marwanad July 10, 2017 17:36

BenSchwab added ✨ Type: Feature 🚗 Runtime Type: Discussion labels Jul 10, 2017

sav007 reviewed Jul 10, 2017

View reviewed changes

sav007 reviewed Jul 11, 2017

View reviewed changes

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch from 8062820 to 69fc87e Compare July 11, 2017 21:53

BenSchwab mentioned this pull request Jul 12, 2017

Make sure RealApolloCall doesn't hold reference to user callback after complete or cancel #560

Closed

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch from f7513a5 to ef5bd57 Compare July 12, 2017 06:09

BenSchwab added Status: In Progress and removed Type: Discussion labels Jul 12, 2017

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch from ef5bd57 to 5193bd0 Compare July 12, 2017 19:48

BenSchwab added Status: Review Needed and removed Status: In Progress labels Jul 12, 2017

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch from 5193bd0 to da88541 Compare July 12, 2017 19:51

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch 2 times, most recently from ba6a547 to 0660a7c Compare July 12, 2017 22:05

BenSchwab changed the title ~~[WIP] Change CacheControl to Interceptor based Fetcher~~ Change CacheControl to Interceptor based Fetcher Jul 12, 2017

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch 2 times, most recently from f2547db to 5595cb0 Compare July 13, 2017 17:35

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch 4 times, most recently from ae780cf to c7af7cc Compare July 14, 2017 16:38

sav007 reviewed Jul 15, 2017

View reviewed changes

BenSchwab added 4 commits July 15, 2017 14:07

runtime changes

8213436

some code review changes fix runtime checkstyle runtime fix remove weak reference changes runtime changes

apollo rx support changes

1bbc765

fixes to rx

Test changes

98e930f

remove unused test test fixes test changes mend

Code review

f47f63c

BenSchwab force-pushed the bschwab--interceptor-fetch-policy branch from c7af7cc to f47f63c Compare July 15, 2017 21:07

sav007 reviewed Jul 16, 2017

View reviewed changes

sav007 approved these changes Jul 16, 2017

View reviewed changes

BenSchwab added 5 commits July 16, 2017 14:31

Add more context to RxJava javadocs, fix defaultResponseFetcher

ff30c1f

Enhance Javadoc for Callback

38eafe9

fix checkstyle

2f77832

Make cache miss throw exception

a6e0c12

fix test from merge

afde7ca

bump version to 0.5.0-SNAPSHOT

f59e15b

BenSchwab added 2 commits July 17, 2017 14:52

Revert "bump version to 0.5.0-SNAPSHOT"

179fd5c

This reverts commit f59e15b.

Fix bad javadoc in ApolloCall

82d8a3e

BenSchwab merged commit caaae7e into master Jul 18, 2017

BenSchwab deleted the bschwab--interceptor-fetch-policy branch July 18, 2017 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change CacheControl to Interceptor based Fetcher #556

Change CacheControl to Interceptor based Fetcher #556

BenSchwab commented Jul 10, 2017 •

edited

Loading

BenSchwab commented Jul 10, 2017

BenSchwab commented Jul 10, 2017

sav007 commented Jul 10, 2017

sav007 Jul 10, 2017

sav007 Jul 10, 2017 •

edited

Loading

digitalbuddha commented Jul 10, 2017

digitalbuddha commented Jul 10, 2017

sav007 Jul 11, 2017

sav007 commented Jul 11, 2017

BenSchwab commented Jul 12, 2017 •

edited

Loading

sav007 Jul 15, 2017 •

edited

Loading

sav007 Jul 15, 2017

sav007 commented Jul 15, 2017

BenSchwab commented Jul 15, 2017

sav007 commented Jul 16, 2017

sav007 commented Jul 16, 2017

sav007 Jul 16, 2017

sav007 left a comment

sav007 Jul 16, 2017

sav007 Jul 16, 2017

sav007 Jul 16, 2017

BenSchwab commented Jul 17, 2017

sav007 commented Jul 17, 2017

BenSchwab commented Jul 17, 2017


		public interface Fetcher {

		ApolloInterceptor create(ApolloLogger logger);

Change CacheControl to Interceptor based Fetcher #556

Change CacheControl to Interceptor based Fetcher #556

Conversation

BenSchwab commented Jul 10, 2017 • edited Loading

Motivation

Fetcher

Benefits

Mutli-response

Additional work

BenSchwab commented Jul 10, 2017

BenSchwab commented Jul 10, 2017

sav007 commented Jul 10, 2017

sav007 Jul 10, 2017

Choose a reason for hiding this comment

sav007 Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

digitalbuddha commented Jul 10, 2017

digitalbuddha commented Jul 10, 2017

sav007 Jul 11, 2017

Choose a reason for hiding this comment

sav007 commented Jul 11, 2017

BenSchwab commented Jul 12, 2017 • edited Loading

sav007 Jul 15, 2017 • edited Loading

Choose a reason for hiding this comment

sav007 Jul 15, 2017

Choose a reason for hiding this comment

sav007 commented Jul 15, 2017

BenSchwab commented Jul 15, 2017

sav007 commented Jul 16, 2017

sav007 commented Jul 16, 2017

sav007 Jul 16, 2017

Choose a reason for hiding this comment

sav007 left a comment

Choose a reason for hiding this comment

sav007 Jul 16, 2017

Choose a reason for hiding this comment

sav007 Jul 16, 2017

Choose a reason for hiding this comment

sav007 Jul 16, 2017

Choose a reason for hiding this comment

BenSchwab commented Jul 17, 2017

sav007 commented Jul 17, 2017

BenSchwab commented Jul 17, 2017

BenSchwab commented Jul 10, 2017 •

edited

Loading

sav007 Jul 10, 2017 •

edited

Loading

BenSchwab commented Jul 12, 2017 •

edited

Loading

sav007 Jul 15, 2017 •

edited

Loading