Repository: apollographql/apollo-client

Problem statement:
Query diffing with variables
It looks like the `minimizedQueryString` in QueryManager doesn't account for named queries, and therefore variables:

``` js
const query = {
  query,
  variables,
  forceFetch: false,
}

apolloClient.query(query)
  .then((result) => {
    // yada yada
  });
```

If the original query was:

```
query getThing($thingId: Int!) {
  thing(id: $thingId) {
    title
  }
}
```

Assuming none of the data is current in the store, the minimized query string generated currently is:

```
thing(id: $thingId) {
  title
}
```

Which throws an error on the graph server since the variable isn't defined.



Hints:
Ooh, that's awkward. OK, maybe we need to pass around variable type definitions wherever we have `SelectionSetWithRoot` instances.

BTW - we want to do an initial open source launch next Tuesday, so keep the bugs coming and I'll try to fix them ASAP!

@stubailo will do. Thanks!


Patch:
diff --git a/src/QueryManager.ts b/src/QueryManager.ts
--- a/src/QueryManager.ts
+++ b/src/QueryManager.ts
@@ -39,10 +39,15 @@ import {
   printQueryFromDefinition,
 } from './queryPrinting';
 
+import {
+  IdGetter,
+} from './data/extensions';
+
 export class QueryManager {
   private networkInterface: NetworkInterface;
   private store: ApolloStore;
   private reduxRootKey: string;
+  private dataIdFromObject: IdGetter;
 
   private resultCallbacks: { [queryId: number]: QueryResultCallback[] };
 
@@ -52,16 +57,19 @@ export class QueryManager {
     networkInterface,
     store,
     reduxRootKey,
+    dataIdFromObject,
   }: {
     networkInterface: NetworkInterface,
     store: ApolloStore,
     reduxRootKey: string,
+    dataIdFromObject?: IdGetter,
   }) {
     // XXX this might be the place to do introspection for inserting the `id` into the query? or
     // is that the network interface?
     this.networkInterface = networkInterface;
     this.store = store;
     this.reduxRootKey = reduxRootKey;
+    this.dataIdFromObject = dataIdFromObject;
 
     this.resultCallbacks = {};
 
@@ -119,7 +127,7 @@ export class QueryManager {
   public watchQuery({
     query,
     variables,
-    forceFetch = true,
+    forceFetch = false,
     returnPartialData = false,
   }: WatchQueryOptions): WatchedQueryHandle {
     // Generate a query ID
@@ -130,13 +138,14 @@ export class QueryManager {
     this.resultCallbacks[queryId] = [];
 
     const queryString = query;
+    const queryDef = parseQuery(query);
 
     // Parse the query passed in -- this could also be done by a build plugin or tagged
     // template string
     const querySS = {
       id: 'ROOT_QUERY',
       typeName: 'Query',
-      selectionSet: parseQuery(query).selectionSet,
+      selectionSet: queryDef.selectionSet,
     } as SelectionSetWithRoot;
 
     // If we don't use diffing, then these will be the same as the original query
@@ -155,12 +164,17 @@ export class QueryManager {
         throwOnMissingField: false,
         rootId: querySS.id,
         variables,
+        dataIdFromObject: this.dataIdFromObject,
       });
 
       initialResult = result;
 
       if (missingSelectionSets.length) {
-        const diffedQueryDef = queryDefinition(missingSelectionSets);
+        const diffedQueryDef = queryDefinition({
+          missingSelectionSets,
+          variableDefinitions: queryDef.variableDefinitions,
+          name: queryDef.name,
+        });
 
         minimizedQuery = {
           id: 'ROOT_QUERY',
diff --git a/src/data/diffAgainstStore.ts b/src/data/diffAgainstStore.ts
--- a/src/data/diffAgainstStore.ts
+++ b/src/data/diffAgainstStore.ts
@@ -23,6 +23,10 @@ import {
   SelectionSetWithRoot,
 } from '../queries/store';
 
+import {
+  IdGetter,
+} from './extensions';
+
 import {
   SelectionSet,
   Field,
@@ -31,16 +35,19 @@ import {
 export interface QueryDiffResult {
   result: any;
   missingSelectionSets: SelectionSetWithRoot[];
+  mergeUp: boolean;
 }
 
 export function diffQueryAgainstStore({
   store,
   query,
   variables,
+  dataIdFromObject,
 }: {
   store: NormalizedCache,
   query: string
   variables?: Object,
+  dataIdFromObject?: IdGetter,
 }): QueryDiffResult {
   const queryDef = parseQuery(query);
 
@@ -50,6 +57,7 @@ export function diffQueryAgainstStore({
     selectionSet: queryDef.selectionSet,
     throwOnMissingField: false,
     variables,
+    dataIdFromObject,
   });
 }
 
@@ -58,11 +66,13 @@ export function diffFragmentAgainstStore({
   fragment,
   rootId,
   variables,
+  dataIdFromObject,
 }: {
   store: NormalizedCache,
   fragment: string,
   rootId: string,
   variables?: Object,
+  dataIdFromObject?: IdGetter,
 }): QueryDiffResult {
   const fragmentDef = parseFragment(fragment);
 
@@ -72,6 +82,7 @@ export function diffFragmentAgainstStore({
     selectionSet: fragmentDef.selectionSet,
     throwOnMissingField: false,
     variables,
+    dataIdFromObject,
   });
 }
 
@@ -92,23 +103,22 @@ export function diffSelectionSetAgainstStore({
   rootId,
   throwOnMissingField = false,
   variables,
+  dataIdFromObject,
 }: {
   selectionSet: SelectionSet,
   store: NormalizedCache,
   rootId: string,
   throwOnMissingField: Boolean,
   variables: Object,
+  dataIdFromObject?: IdGetter,
 }): QueryDiffResult {
   if (selectionSet.kind !== 'SelectionSet') {
     throw new Error('Must be a selection set.');
   }
 
   const result = {};
-
   const missingSelectionSets: SelectionSetWithRoot[] = [];
-
-  const missingSelections: Field[] = [];
-
+  const missingFields: Field[] = [];
   const storeObj = store[rootId] || {};
 
   selectionSet.selections.forEach((selection) => {
@@ -121,12 +131,21 @@ export function diffSelectionSetAgainstStore({
     const storeFieldKey = storeKeyNameFromField(field, variables);
     const resultFieldKey = resultKeyNameFromField(field);
 
+    // Don't push more than one missing field per field in the query
+    let missingFieldPushed = false;
+    function pushMissingField(missingField: Field) {
+      if (!missingFieldPushed) {
+        missingFields.push(missingField);
+        missingFieldPushed = true;
+      }
+    }
+
     if (! has(storeObj, storeFieldKey)) {
       if (throwOnMissingField) {
         throw new Error(`Can't find field ${storeFieldKey} on object ${storeObj}.`);
       }
 
-      missingSelections.push(field);
+      missingFields.push(field);
 
       return;
     }
@@ -157,10 +176,17 @@ export function diffSelectionSetAgainstStore({
           rootId: id,
           selectionSet: field.selectionSet,
           variables,
+          dataIdFromObject,
         });
 
-        itemDiffResult.missingSelectionSets.forEach(
-          itemSelectionSet => missingSelectionSets.push(itemSelectionSet));
+        if (! itemDiffResult.mergeUp) {
+          itemDiffResult.missingSelectionSets.forEach(
+            itemSelectionSet => missingSelectionSets.push(itemSelectionSet));
+        } else {
+          // XXX merge all of the missing selections from the children to get a more minimal result
+          pushMissingField(field);
+        }
+
         return itemDiffResult.result;
       });
       return;
@@ -173,11 +199,16 @@ export function diffSelectionSetAgainstStore({
         rootId: storeValue,
         selectionSet: field.selectionSet,
         variables,
+        dataIdFromObject,
       });
 
-      // This is a nested query
-      subObjDiffResult.missingSelectionSets.forEach(
-        subObjSelectionSet => missingSelectionSets.push(subObjSelectionSet));
+      if (! subObjDiffResult.mergeUp) {
+        subObjDiffResult.missingSelectionSets.forEach(
+          subObjSelectionSet => missingSelectionSets.push(subObjSelectionSet));
+      } else {
+        // XXX merge all of the missing selections from the children to get a more minimal result
+        pushMissingField(field);
+      }
 
       result[resultFieldKey] = subObjDiffResult.result;
       return;
@@ -186,39 +217,71 @@ export function diffSelectionSetAgainstStore({
     throw new Error('Unexpected number value in the store where the query had a subselection.');
   });
 
+  // Set this to true if we don't have enough information at this level to generate a refetch
+  // query, so we need to merge the selection set with the parent, rather than appending
+  let mergeUp = false;
+
   // If we weren't able to resolve some selections from the store, construct them into
   // a query we can fetch from the server
-  if (missingSelections.length) {
-    const id = storeObj['id'];
-    if (typeof id !== 'string' && rootId !== 'ROOT_QUERY') {
-      throw new Error(
-        `Can't generate query to refetch object ${rootId}, since it doesn't have a string id.`);
-    }
+  if (missingFields.length) {
+    if (dataIdFromObject) {
+      // We have a semantic understanding of IDs
+      const id = dataIdFromObject(storeObj);
+
+      if (typeof id !== 'string' && rootId !== 'ROOT_QUERY') {
+        throw new Error(
+          `Can't generate query to refetch object ${rootId}, since it doesn't have a string id.`);
+      }
+
+      let typeName: string;
 
-    let typeName: string;
+      if (rootId === 'ROOT_QUERY') {
+        // We don't need to do anything interesting to fetch root queries, like have an ID
+        typeName = 'Query';
+      } else if (! storeObj.__typename) {
+        throw new Error(
+          `Can't generate query to refetch object ${rootId}, since __typename wasn't in the store.`);
+      } else {
+        typeName = storeObj.__typename;
+      }
 
-    if (rootId === 'ROOT_QUERY') {
-      // We don't need to do anything interesting to fetch root queries, like have an ID
-      typeName = 'Query';
-    } else if (! storeObj.__typename) {
-      throw new Error(
-        `Can't generate query to refetch object ${rootId}, since __typename wasn't in the store.`);
+      missingSelectionSets.push({
+        id: rootId,
+        typeName,
+        selectionSet: {
+          kind: 'SelectionSet',
+          selections: missingFields,
+        },
+      });
+    } else if (rootId === 'ROOT_QUERY') {
+      const typeName = 'Query';
+
+      missingSelectionSets.push({
+        id: rootId,
+        typeName,
+        selectionSet: {
+          kind: 'SelectionSet',
+          selections: missingFields,
+        },
+      });
     } else {
-      typeName = storeObj.__typename;
+      mergeUp = true;
+
+      missingSelectionSets.push({
+        // Sentinel values, all we need is the selection set
+        id: 'CANNOT_REFETCH',
+        typeName: 'CANNOT_REFETCH',
+        selectionSet: {
+          kind: 'SelectionSet',
+          selections: missingFields,
+        },
+      });
     }
-
-    missingSelectionSets.push({
-      id: rootId,
-      typeName,
-      selectionSet: {
-        kind: 'SelectionSet',
-        selections: missingSelections,
-      },
-    });
   }
 
   return {
     result,
     missingSelectionSets,
+    mergeUp,
   };
 }
diff --git a/src/queryPrinting.ts b/src/queryPrinting.ts
--- a/src/queryPrinting.ts
+++ b/src/queryPrinting.ts
@@ -2,14 +2,16 @@ import {
   print,
   SelectionSet,
   OperationDefinition,
+  VariableDefinition,
+  Name,
 } from 'graphql';
 
 import {
   SelectionSetWithRoot,
 } from './queries/store';
 
-export function printQueryForMissingData(missingSelectionSets: SelectionSetWithRoot[]) {
-  return printQueryFromDefinition(queryDefinition(missingSelectionSets));
+export function printQueryForMissingData(options: QueryDefinitionOptions) {
+  return printQueryFromDefinition(queryDefinition(options));
 }
 
 const idField = {
@@ -32,9 +34,16 @@ export function printQueryFromDefinition(queryDef: OperationDefinition) {
   return print(queryDocumentAst);
 }
 
-export function queryDefinition(
-    missingSelectionSets: SelectionSetWithRoot[]): OperationDefinition {
+export function queryDefinition({
+    missingSelectionSets,
+    variableDefinitions = null,
+    name = null,
+}: QueryDefinitionOptions): OperationDefinition {
   const selections = missingSelectionSets.map((missingSelectionSet: SelectionSetWithRoot, ii) => {
+    if (missingSelectionSet.id === 'CANNOT_REFETCH') {
+      throw new Error('diffAgainstStore did not merge selection sets correctly');
+    }
+
     if (missingSelectionSet.id === 'ROOT_QUERY') {
       if (missingSelectionSet.selectionSet.selections.length > 1) {
         throw new Error('Multiple root queries, cannot print that yet.');
@@ -54,8 +63,8 @@ export function queryDefinition(
   return {
     kind: 'OperationDefinition',
     operation: 'query',
-    name: null,
-    variableDefinitions: null,
+    name,
+    variableDefinitions,
     directives: [],
     selectionSet: {
       kind: 'SelectionSet',
@@ -131,3 +140,9 @@ function inlineFragmentSelection({
     selectionSet,
   };
 }
+
+export type QueryDefinitionOptions = {
+  missingSelectionSets: SelectionSetWithRoot[];
+  variableDefinitions?: VariableDefinition[];
+  name?: Name;
+}


Test patch:
diff --git a/test/QueryManager.ts b/test/QueryManager.ts
--- a/test/QueryManager.ts
+++ b/test/QueryManager.ts
@@ -12,6 +12,7 @@ import {
 } from '../src/store';
 
 import {
+  IdGetter,
   getIdField,
 } from '../src/data/extensions';
 
@@ -456,7 +457,97 @@ describe('QueryManager', () => {
         },
         variables: {},
       },
-    ], done);
+    ], {
+      dataIdFromObject: getIdField,
+    }, done);
+  });
+
+  it('diffs queries, preserving variable declarations', (done) => {
+    testDiffing([
+      {
+        query: `
+          {
+            people_one(id: "1") {
+              __typename,
+              id,
+              name
+            }
+          }
+        `,
+        diffedQuery: `
+          {
+            people_one(id: "1") {
+              __typename,
+              id,
+              name
+            }
+          }
+        `,
+        diffedQueryResponse: {
+          people_one: {
+            __typename: 'Person',
+            id: '1',
+            name: 'Luke Skywalker',
+          },
+        },
+        fullResponse: {
+          people_one: {
+            __typename: 'Person',
+            id: '1',
+            name: 'Luke Skywalker',
+          },
+        },
+        variables: {},
+      },
+      {
+        query: `
+          query getSeveralPeople($lukeId: String!, $vaderId: String!) {
+            luke: people_one(id: $lukeId) {
+              __typename
+              id
+              name
+            }
+            vader: people_one(id: $vaderId) {
+              __typename
+              id
+              name
+            }
+          }
+        `,
+        diffedQuery: `
+          query getSeveralPeople($lukeId: String!, $vaderId: String!) {
+            vader: people_one(id: $vaderId) {
+              __typename
+              id
+              name
+            }
+          }
+        `,
+        diffedQueryResponse: {
+          vader: {
+            __typename: 'Person',
+            id: '4',
+            name: 'Darth Vader',
+          },
+        },
+        fullResponse: {
+          luke: {
+            __typename: 'Person',
+            id: '1',
+            name: 'Luke Skywalker',
+          },
+          vader: {
+            __typename: 'Person',
+            id: '4',
+            name: 'Darth Vader',
+          },
+        },
+        variables: {
+          lukeId: '1',
+          vaderId: '4',
+        },
+      },
+    ], {}, done);
   });
 
   it(`doesn't return data while query is loading`, (done) => {
@@ -689,6 +780,9 @@ function testDiffing(
     // Variables to use in all queries
     variables?: Object,
   }[],
+  config: {
+    dataIdFromObject?: IdGetter,
+  },
   done: () => void
 ) {
   const networkInterface = mockNetworkInterface(queryArray.map(({
@@ -708,6 +802,7 @@ function testDiffing(
       config: { dataIdFromObject: getIdField },
     }),
     reduxRootKey: 'apollo',
+    dataIdFromObject: config.dataIdFromObject,
   });
 
   const steps = queryArray.map(({ query, fullResponse, variables }) => {
diff --git a/test/diffAgainstStore.ts b/test/diffAgainstStore.ts
--- a/test/diffAgainstStore.ts
+++ b/test/diffAgainstStore.ts
@@ -36,7 +36,7 @@ describe('diffing queries against the store', () => {
     }).missingSelectionSets, []);
   });
 
-  it('returns correct selection set when the store is missing one field', () => {
+  it('when the store is missing one field and knows about IDs', () => {
     const firstQuery = `
       {
         people_one(id: "1") {
@@ -73,6 +73,7 @@ describe('diffing queries against the store', () => {
     assert.deepEqual(stripLoc(diffQueryAgainstStore({
       store,
       query: secondQuery,
+      dataIdFromObject: getIdField,
     }).missingSelectionSets), [
       {
         id: 'lukeId',
@@ -97,7 +98,7 @@ describe('diffing queries against the store', () => {
     ]);
   });
 
-  it('generates the right query when the store is missing one field', () => {
+  it('when the store is missing one field and knows about IDs', () => {
     const firstQuery = `
       {
         people_one(id: "1") {
@@ -134,9 +135,12 @@ describe('diffing queries against the store', () => {
     const { missingSelectionSets } = diffQueryAgainstStore({
       store,
       query: secondQuery,
+      dataIdFromObject: getIdField,
     });
 
-    assert.equal(printQueryForMissingData(missingSelectionSets), `{
+    assert.equal(printQueryForMissingData({
+      missingSelectionSets,
+    }), `{
   __node_0: node(id: "lukeId") {
     id
     ... on Person {
@@ -147,7 +151,58 @@ describe('diffing queries against the store', () => {
 `);
   });
 
-  it('generates the right queries when the store is missing multiple nodes', () => {
+  it('when the store is missing one field and doesn\'t know IDs', () => {
+    const firstQuery = `
+      {
+        people_one(id: "1") {
+          __typename
+          id
+          name
+        }
+      }
+    `;
+
+    const result = {
+      people_one: {
+        __typename: 'Person',
+        id: 'lukeId',
+        name: 'Luke Skywalker',
+      },
+    };
+
+    const store = writeQueryToStore({
+      result,
+      query: firstQuery,
+    });
+
+    const secondQuery = `
+      {
+        people_one(id: "1") {
+          name
+          age
+        }
+      }
+    `;
+
+    const { missingSelectionSets } = diffQueryAgainstStore({
+      store,
+      query: secondQuery,
+    });
+
+    // XXX a more efficient diffing algorithm would actually only fetch `age` here. Something to
+    // implement next
+    assert.equal(printQueryForMissingData({
+      missingSelectionSets,
+    }), `{
+  people_one(id: "1") {
+    name
+    age
+  }
+}
+`);
+  });
+
+  it('when the store is missing multiple nodes', () => {
     const firstQuery = `
       {
         people_one(id: "1") {
@@ -188,9 +243,12 @@ describe('diffing queries against the store', () => {
     const { missingSelectionSets } = diffQueryAgainstStore({
       store,
       query: secondQuery,
+      dataIdFromObject: getIdField,
     });
 
-    assert.equal(printQueryForMissingData(missingSelectionSets), `{
+    assert.equal(printQueryForMissingData({
+      missingSelectionSets,
+    }), `{
   __node_0: node(id: "lukeId") {
     id
     ... on Person {
@@ -294,7 +352,9 @@ describe('diffing queries against the store', () => {
       query: secondQuery,
     });
 
-    assert.equal(printQueryForMissingData(missingSelectionSets), `{
+    assert.equal(printQueryForMissingData({
+      missingSelectionSets,
+    }), `{
   people_one(id: "2") {
     __typename
     id
diff --git a/test/queryPrinting.ts b/test/queryPrinting.ts
--- a/test/queryPrinting.ts
+++ b/test/queryPrinting.ts
@@ -23,13 +23,15 @@ describe('printing queries', () => {
     };
 
     // Note - the indentation inside template strings is meaningful!
-    assert.equal(printQueryForMissingData([
-      {
-        id,
-        typeName,
-        selectionSet,
-      },
-    ]), `{
+    assert.equal(printQueryForMissingData({
+      missingSelectionSets: [
+        {
+          id,
+          typeName,
+          selectionSet,
+        },
+      ],
+    }), `{
   __node_0: node(id: "lukeId") {
     id
     ... on Person {