Add Query Watching #312

BenSchwab · 2017-03-10T04:20:00Z

closes #304

This is a WIP query watching PR. There will be many more tests, naming will be reconsidered, TODOs fixed etc...

But wanted to get early thoughts on general design.

I played around with couple ways, but settled on have an the flow ApolloCall.toWatcher() -> ApolloWatcher

The reason is that in order to watch a query you need to execute the query at least once to generate the dependentKeys. So a user would first need to configure the "root query" with whatever cache control they want, and then transform it into a watcher.

Then they call enqueueAndWatch which performs the initial fetch. Upon successful fetch, the watcher then subscribes to change keys events on the cache.

Importantly, refetch cachePolicy is separate than the initial fetch policy.

One thing to take a look at: whenever a query is refetched, we actually unsubscribe from cache changed keys event, until the query is returned with new dependentKeys. This is because when there is an outgoing request, the current dependentKeys are "stale". When we move to more sophisticated threading model this will be important.

sav007 · 2017-03-10T20:30:05Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloWatcher.java

+  }
+
+  @Nonnull public WatcherSubscription enqueueAndWatch(@Nullable final ApolloCall.Callback<T> callback) {
+    this.callback = callback;


Should we add safe guard if we already watching? I mean we don't support multiple subscriptions right?

sav007 · 2017-03-10T20:32:38Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloWatcher.java

+  }
+
+  private void fetch() {
+    activeCall.enqueue(callbackProxy(callback, activeCall));


Should be careful, active call can't be run multiple times, should it be activeCall.clone()?

enqueueAndWatch should only be called once -- I can add some safeguards for that. But enqueueAndWatch just calls the ApolloCall that this was transformed from, so that should never have been executed before.

sav007 · 2017-03-10T20:39:07Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloWatcher.java

+
+  @Nonnull public RealApolloWatcher<T> refetch() {
+    activeCall.cancel(); //Todo: is this necessary / good? We don't want people to chain refetch().refetch()
+    activeCall = activeCall.clone().cacheControl(refetchCacheControl);


I guess this line should be moved to fetch function, it will be easier to understand that we actually doing activeCall.clone() before enqueue

So we actually aren't cloning before enqueueAndWatch only from refetch. Per other comments, it is only possible to transform a ApolloCall to an ApolloWatcher if it has not been executed. However, calls to refetch will clone the ApolloCall because in this case it has been used. I haven't convinced myself that refetch as a lot of value -- but added it because it is part of the iOS watcher interface.

sav007 · 2017-03-10T20:40:34Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloCall.java

@@ -74,7 +76,7 @@ private RealApolloCall(Operation operation, HttpUrl serverUrl, Call.Factory http
    return executeNetworkRequest();
  }

-  @Nonnull @Override public ApolloCall<T> enqueue(@Nullable final Callback<T> callback) {
+  @Nonnull @Override public void enqueue(@Nullable final Callback<T> callback) {


what the reason to remove return value?

And can we have like this: apollocall.enqueue(callback).watch() ?

We need to guarantee that the a Call has been executed when we watch. Otherwise we have no dependentKeys to monitor. So I don't like relying on have the user chain apollocall.enqueue().watch(). Instead I have it has apolloCall.toWatcher().enqueueAndWatch().

Is there a value in having enqueue return an ApolloCall? OkHttp has void return null. It just seems dangerous. Once you call enqueue many functions on that ApolloCall now explode, and returning it seems to imply that the object is still safe to use.

Is there a value in having enqueue return an ApolloCall

no just was curious, thx makes sense!!

sav007 · 2017-03-10T20:43:26Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/RealCache.java

@@ -9,22 +15,35 @@
 public final class RealCache implements Cache {
  private final CacheStore cacheStore;
  private final CacheKeyResolver cacheKeyResolver;
+  private Map<RecordChangeSubscriber, Set<String>> subscribers;


Should we make RecordChangeSubscriber as weak ref? So WeakHashMap?
Then it will be user responsibility to keep around subscribtion?

👍 Good Idea!

sav007 · 2017-03-10T21:43:33Z

One more question do we have a vision how query watching will utilize RxJava?

digitalbuddha · 2017-03-10T21:44:53Z

It's going to use rxjava fromEmitter/create api which can wrap any async call into rx. I have good idea how to get that piece in.

digitalbuddha · 2017-03-10T21:48:28Z

Here was original idea for rx support. I don't see why it won't work for this too. https://gist.github.com/digitalbuddha/3c1dbccf059f0279e46e540b2c5fd89e

sav007 · 2017-03-11T15:54:23Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloWatcher.java

+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+final class RealApolloWatcher<T extends Operation.Data> implements ApolloWatcher<T> {


Do we need to have cancel or dispose method, to cancel watching via RealApolloWatcher as well not only via WatcherSubscription

sav007 · 2017-03-11T15:56:19Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloWatcher.java

+    return this;
+  }
+
+  @Nonnull public RealApolloWatcher<T> refetch() {


I guess we can merge refetchCacheControlwith this method, so to have 2 API calls:
public RealApolloWatcher<T> refetch(@Nonnull CacheControl cacheControl)
public RealApolloWatcher<T> refetch()

Overloading seems fair -- but we need to keep refetchCacheControl because it plays a roll in enqueueAndWatch because when dependenKeys change refetchCacheControl is used to make the new request. So call.toWatcher().refetchCacheControl(CACHE_ONLY).enqueueAndWatch(..)

My intention was to make it as much immutable as possible, after we called enqueueAndWatch what if we change the cache control refetchCacheControl? Should we add safe guard to prevent user from changing cache control after he called enqueueAndWatch ?

sav007 · 2017-03-11T15:58:55Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloCall.java

  }

-  @Nonnull @Override public ApolloCall<T> httpCacheControl(@Nonnull HttpCacheControl httpCacheControl) {
+  @Nonnull @Override public RealApolloWatcher<T> toWatcher() {


What happens if I call this multiple times for the same RealApolloCall

With clone, this would be acceptable. I don't see a real harm in allowing it, but it does go against the explicit single-use immutable style of the other methods.

sav007 · 2017-03-11T16:02:59Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloCall.java

  }

-  @Nonnull @Override public ApolloCall<T> httpCacheControl(@Nonnull HttpCacheControl httpCacheControl) {
+  @Nonnull @Override public RealApolloWatcher<T> toWatcher() {
+    return new RealApolloWatcher<>(this, cache);


shouldn't we call new RealApolloWatcher<>(clone(), cache)?

sav007 · 2017-03-11T16:05:22Z

apollo-runtime/src/main/java/com/apollographql/android/cache/normalized/RealCache.java

@@ -12,33 +20,41 @@
  private final CacheStore cacheStore;
  private final CacheKeyResolver cacheKeyResolver;
  private final ReadWriteLock lock;
+  private Map<RecordChangeSubscriber, Set<String>> subscribers;


sav007 · 2017-03-12T22:22:55Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloWatcher.java

+    this.cache = cache;
+  }
+
+  public void enqueueAndWatch(@Nullable final ApolloCall.Callback<T> callback) {


nit: @OverRide

sav007 · 2017-03-12T22:23:09Z

apollo-runtime/src/main/java/com/apollographql/android/impl/RealApolloWatcher.java

+    activeCall.enqueue(callbackProxy(this.callback, activeCall));
+  }
+
+  @Nonnull public RealApolloWatcher<T> refetchCacheControl(@Nonnull CacheControl cacheControl) {


nit: @OverRide

sav007 · 2017-03-12T22:29:30Z

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

-  @Nonnull ApolloCall<T> enqueue(@Nullable Callback<T> callback);
+  void enqueue(@Nullable Callback<T> callback);
+
+  @Nonnull ApolloWatcher<T> toWatcher();


nit: maybe simple watcher() ?

sav007

Small comments, but lgtm. Great work

BenSchwab · 2017-03-13T02:15:01Z

Thanks for the reviews!

BenSchwab requested review from brianPlummer, sav007 and digitalbuddha March 10, 2017 04:30

BenSchwab force-pushed the bschwab/query-watch branch from 67cd4cb to 902a8aa Compare March 10, 2017 18:19

sav007 reviewed Mar 10, 2017

View reviewed changes

BenSchwab added 3 commits March 10, 2017 17:03

WIP ApolloWatcher

0de46a8

WIP

2f8cb78

WIP

a93c1f0

BenSchwab force-pushed the bschwab/query-watch branch from 902a8aa to a93c1f0 Compare March 11, 2017 07:20

clean some things up

a66cd8a

BenSchwab changed the title ~~[WIP] Add Query Watching~~ Add Query Watching Mar 11, 2017

BenSchwab requested a review from sav007 March 11, 2017 07:30

sav007 reviewed Mar 11, 2017

View reviewed changes

BenSchwab added 2 commits March 12, 2017 12:00

Add cancel, final, remove subscription

a4766bc

Don't allow setting refetch policy after first enqueue

9ea4965

sav007 reviewed Mar 12, 2017

View reviewed changes

sav007 approved these changes Mar 12, 2017

View reviewed changes

watcher, @OverRide

d3dfa50

remoe unused dateutil

54cab77

BenSchwab merged commit 5200ea3 into master Mar 13, 2017

BenSchwab deleted the bschwab/query-watch branch March 13, 2017 02:39

BenSchwab mentioned this pull request Mar 15, 2017

Fix duplicate cache write caused by bad merge #325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Query Watching #312

Add Query Watching #312

BenSchwab commented Mar 10, 2017 •

edited

Loading

sav007 Mar 10, 2017 •

edited

Loading

BenSchwab Mar 10, 2017

sav007 Mar 10, 2017

BenSchwab Mar 10, 2017 •

edited

Loading

sav007 Mar 10, 2017

BenSchwab Mar 10, 2017 •

edited

Loading

sav007 Mar 10, 2017

sav007 Mar 10, 2017

BenSchwab Mar 10, 2017 •

edited

Loading

sav007 Mar 10, 2017

sav007 Mar 10, 2017

BenSchwab Mar 10, 2017

sav007 commented Mar 10, 2017

digitalbuddha commented Mar 10, 2017

digitalbuddha commented Mar 10, 2017

sav007 Mar 11, 2017

sav007 Mar 11, 2017

BenSchwab Mar 11, 2017 •

edited

Loading

sav007 Mar 12, 2017

sav007 Mar 11, 2017

BenSchwab Mar 12, 2017

sav007 Mar 11, 2017

sav007 Mar 11, 2017

sav007 Mar 12, 2017

sav007 Mar 12, 2017

sav007 Mar 12, 2017

sav007 left a comment

BenSchwab commented Mar 13, 2017

Add Query Watching #312

Add Query Watching #312

Conversation

BenSchwab commented Mar 10, 2017 • edited Loading

sav007 Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sav007 commented Mar 10, 2017

digitalbuddha commented Mar 10, 2017

digitalbuddha commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sav007 left a comment

Choose a reason for hiding this comment

BenSchwab commented Mar 13, 2017

BenSchwab commented Mar 10, 2017 •

edited

Loading

sav007 Mar 10, 2017 •

edited

Loading

BenSchwab Mar 10, 2017 •

edited

Loading

BenSchwab Mar 10, 2017 •

edited

Loading

BenSchwab Mar 10, 2017 •

edited

Loading

BenSchwab Mar 11, 2017 •

edited

Loading