Prepare for query re-fetching #491

sav007 · 2017-05-13T22:25:26Z

Introduce separate interfaces for query and mutation calls.
Generate OperationName type for operations to be used as identifier for refetching.

Part of #452

Introduce separate interfaces for query and mutation calls. Generate OperationName type for operations to be used as identifier for refetching.

sav007 · 2017-05-14T16:15:13Z

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

+   */
+  @Nonnull ApolloMutationCall<T> refetchQueries(@Nonnull OperationName... operationNames);
+
+  @Nonnull @Override ApolloMutationCall<T> cacheControl(@Nonnull CacheControl cacheControl);


@BenSchwab do you think cacheControl makes sense for mutation as it defines fetch strategies and makes sense for queries only?

Though cacheHeaders makes sense cos it defines if response for this mutation should be cached or not.

how does caching a mutation work? Like what happens if you try same mutation again will you then get cached result instead of it hitting server?

We shouldn't not hit the when we run mutations, but we should store the results to cache. So it's should WRITE ONLY. That's why I guess we need to remove cacheControl and just have cacheHeaders as they tells to save to cache or not

Yes, cacheControl does not make sense for mutations. It will always be NETWORK_ONLY. If we write to the cache can be controlled by cacheHeaders. It's generally important to write mutations results to the cache, as this is what triggers updates to apolloWatchers.

digitalbuddha · 2017-05-14T20:50:50Z

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

@@ -62,7 +60,7 @@ public void testQueryWatcherUpdated_SameQuery_DifferentResults() throws IOExcept
    EpisodeHeroName query = EpisodeHeroName.builder().episode(Episode.EMPIRE).build();
    server.enqueue(mockResponse("EpisodeHeroNameResponseWithId.json"));

-    ApolloWatcher<EpisodeHeroName.Data> watcher = apolloClient.newCall(query).watcher();
+    ApolloQueryWatcher<EpisodeHeroName.Data> watcher = apolloClient.query(query).watcher();


can you watch anything other than a query?

Not now, we watch only Queries

digitalbuddha · 2017-05-14T20:54:36Z

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

+  private <D extends Operation.Data, T, V extends Operation.Variables> RealApolloCall<T> newCall(
+      @Nonnull Operation<D, T, V> operation) {
+    ResponseFieldMapper responseFieldMapper = responseFieldMapper(operation);
+    return new RealApolloCall<T>(operation, serverUrl, httpCallFactory, httpCache, defaultHttpCachePolicy, moshi,


this constructor is wild! is there a way we can encapsulate somehow. Of the top of my head I see 2 options: builder or organization of params into a few objects.

digitalbuddha · 2017-05-14T20:56:15Z

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

+    ResponseFieldMapper responseFieldMapper;
+    synchronized (responseFieldMapperPool) {
+      responseFieldMapper = responseFieldMapperPool.get(operation.getClass());
+      if (responseFieldMapper == null) {


Sorry if this is out of scope for this pr but is there a way way can do a null object pattern or some other way to show absence without using null. also missing nullability annotations

VisheshVadhera · 2017-05-15T06:53:07Z

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

+     *
+     * @param query the operation which needs to be performed
+     * @return prepared {@link ApolloQueryCall} call to be executed at some point in the future
+     */


We need to change the docs to:

/** * Creates and prepares a new {@link ApolloMutationCall} call. * * @param mutation the mutation which needs to be performed * @return prepared {@link ApolloMutationCall} call to be executed at some point in the future */

VisheshVadhera · 2017-05-15T06:55:37Z

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

@@ -34,14 +34,14 @@ private Rx2Apollo() {
  }

  /**
-   * Converts an {@link ApolloWatcher} to an asynchronous Observable.
+   * Converts an {@link ApolloQueryWatcher} to an asynchronous Observable.
   *
   * @param watcher the ApolloWatcher to convert.


We need to change it to:

@param watcher the ApolloQueryWatcher to convert.

VisheshVadhera · 2017-05-15T06:57:19Z

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

@@ -35,27 +35,27 @@ private RxApollo() {
  }

  /**
-   * Converts an {@link ApolloWatcher} into an Observable. Honors the back pressure from downstream with the back
+   * Converts an {@link ApolloQueryWatcher} into an Observable. Honors the back pressure from downstream with the back
   * pressure strategy {@link rx.Emitter.BackpressureMode#LATEST}.
   *
   * @param watcher the ApolloWatcher to convert


@param watcher the ApolloQueryWatcher to convert.

VisheshVadhera · 2017-05-15T06:57:46Z

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

    return from(watcher, Emitter.BackpressureMode.LATEST);
  }

  /**
-   * Converts an {@link ApolloWatcher} into an Observable.
+   * Converts an {@link ApolloQueryWatcher} into an Observable.
   *
   * @param watcher          the ApolloWatcher to convert


@param watcher the ApolloQueryWatcher to convert.

sav007 · 2017-05-17T19:54:13Z

@BenSchwab @digitalbuddha @marwanad any comments?

BenSchwab

Is OperationName sufficiently unique?
I forget if we allow two querries to have the same name so long as they are in different packages?

sav007 · 2017-05-18T16:53:28Z

@BenSchwab just checked we don;t allow to have the same names for queries even in separate packages:

There can be only one operation named "CheckoutByIdQuery".
There can be only one operation named "CheckoutByIdQuery".

But it even if we allow to have the same names, for refetch we are going to use OperationName object itself not string name:

public final class TestQuery implements Query<TestQuery.Data, Optional<TestQuery.Data>, Operation.Variables> {
...

  private static final OperationName OPERATION_NAME = new OperationName() {
    @Override
    public String name() {
      return "TestQuery";
    }
  };

  @Override
  public OperationName name() {
    return OPERATION_NAME;
  }

So it will be unique for every query

BenSchwab · 2017-05-18T17:27:57Z

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

+public interface ApolloMutationCall<T> extends ApolloCall<T> {
+
+  /**
+   * <p>Sets a list of GraphQL query names to be re-fetched once this mutation completed.</p>


Is this comment correct? Why do you need to obtain a ApolloQueryWatcher first?

We will use ApolloQueryWatcher to be notified when this mutation operation completed. So in order to listen for refetch user should have watcher first right?

Well, we might want to refetch a query you aren't watching. Like if you have a post feed page, and a child page that creates a post, we might want to refetch the post feed query to update the normalized cache, even if we aren't watching

Then if you happen to be watching the post page, and something did change, the watcher will be triggered when the refetched query is written to the normalized cache

I was imagining you would just pass the actual queries to be refetched here instead of an identifier.

Also, what would happen if I was listening to the same query that was executed twice with different variables. Seems like the tags would collide.

Yeah, makes sense. Tags can easily collide. So we just provide list of Query objects to be refetched, so the only open question how to handle errors, just ignore? Mutation finished with success, but one of the refetched query failed.

At the same time even if more than one watcher with the same name registered we will trigger them both

Yeah, I think we would just have to ignore errors. Log them to our ApolloLogger of course.

Good point that we could just refetch all watchers with same tag. It's possible that we would be fetching more than the user actually wants, but if we batch the refetch, it doesn't seem like a big deal.

BenSchwab

Changes to the interfaces LGTM -- in terms of the refetching API, I would still lean towards being able to register Query objects vs tags just for the fact that it's unclear to me how watcher's will be used in practice, and being more generic seems like a plus. But if you want to land this, we can revisit when we implement the refetching behavior.

Prepare for query re-fetching.

817fa5a

Introduce separate interfaces for query and mutation calls. Generate OperationName type for operations to be used as identifier for refetching.

sav007 added Status: Review Needed 🚗 Runtime labels May 13, 2017

sav007 requested review from BenSchwab, digitalbuddha and marwanad May 13, 2017 22:25

sav007 added 3 commits May 13, 2017 18:32

Disable http cache for mutations

ec8de53

Update

0b0de42

Update

32cd7a0

sav007 commented May 14, 2017

View reviewed changes

Update

b87a506

digitalbuddha reviewed May 14, 2017

View reviewed changes

VisheshVadhera reviewed May 15, 2017

View reviewed changes

sav007 added 2 commits May 15, 2017 23:43

Update

cace53a

Merge branch 'master' into feature-452/refetch-query-prepare

ed36b20

BenSchwab reviewed May 18, 2017

View reviewed changes

BenSchwab approved these changes May 19, 2017

View reviewed changes

sav007 merged commit e035c02 into apollographql:master May 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for query re-fetching #491

Prepare for query re-fetching #491

sav007 commented May 13, 2017

sav007 May 14, 2017

digitalbuddha May 14, 2017

sav007 May 15, 2017

BenSchwab May 15, 2017

digitalbuddha May 14, 2017

sav007 May 15, 2017

digitalbuddha May 14, 2017

digitalbuddha May 14, 2017

VisheshVadhera May 15, 2017 •

edited

Loading

VisheshVadhera May 15, 2017

VisheshVadhera May 15, 2017 •

edited

Loading

VisheshVadhera May 15, 2017

sav007 commented May 17, 2017

BenSchwab left a comment

sav007 commented May 18, 2017

BenSchwab May 18, 2017

sav007 May 18, 2017

BenSchwab May 18, 2017

BenSchwab May 18, 2017

BenSchwab May 18, 2017

sav007 May 19, 2017

sav007 May 19, 2017

BenSchwab May 19, 2017

BenSchwab left a comment

Prepare for query re-fetching #491

Prepare for query re-fetching #491

Conversation

sav007 commented May 13, 2017

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

VisheshVadhera May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VisheshVadhera May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sav007 commented May 17, 2017

BenSchwab left a comment

Choose a reason for hiding this comment

sav007 commented May 18, 2017

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

BenSchwab left a comment

Choose a reason for hiding this comment

VisheshVadhera May 15, 2017 •

edited

Loading

VisheshVadhera May 15, 2017 •

edited

Loading