buildRQL ignores unknown route params

Summary: D2391474 added support for "streamlined" route queries definitions of the form ##() => Relay.QL`query { viewer }`##. The implementation pre-allocated fragment references to the root fragment in case they were needed (in case of streamlined query definitions), and passed in *all* route params as variables to the root fragment. This broke the invariant in `RelayFragmentReference` that all variables must be undefined, because route params can be optional.

The fix here is to a) create fragment references only when the route query actually uses the new style and b) only pass route params that correspond to a variable in the root component.

Reviewed By: @yungsters

Differential Revision: D2415143

src/container/RelayContainer.js

@@ -916,7 +916,7 @@ function buildContainerFragment(
 ): GraphQL.Fragment {
   var fragment = buildRQL.Fragment(
     fragmentBuilder,
-    Object.keys(variables)
+    variables
   );
   invariant(
     fragment,
@@ -968,6 +968,8 @@ function create(
     adapter: ContainerConstructor.getFragmentNames,
   });
   ContainerConstructor.hasFragment = fragmentName => !!fragments[fragmentName];
+  ContainerConstructor.hasVariable = variableName =>
+    Object.prototype.hasOwnProperty.call(initialVariables, variableName);
 
   /**
    * Retrieves a reference to the fragment by name. An optional second argument

src/container/__tests__/getRelayQueries-test.js

@@ -35,7 +35,7 @@ describe('getRelayQueries', () => {
       render() {
         return <div/>;
       }
-    }
+    };
 
     MockPageContainer = Relay.createContainer(MockPageComponent, {
       fragments: {
@@ -155,15 +155,40 @@ describe('getRelayQueries', () => {
     ]).toBeWarnedNTimes(1);
   });
 
-  it('includes route parameters when building component fragment', () => {
-    var MockRoute = makeRoute();
-    var params = {id: '123'};
-    var route = new MockRoute(params);
-    MockPageContainer.getFragment = jest.genMockFunction();
+  it('sets root fragment variables to route params', () => {
+    class MockRoute extends Relay.Route {}
+    MockRoute.routeName = 'MockRoute';
+    MockRoute.path = '/';
+    MockRoute.paramDefinitions = {};
+    MockRoute.queries = {
+      first: () => Relay.QL`
+        query {
+          viewer
+        }
+      `,
+    };
+
+    var route = new MockRoute({
+      fragmentParam: 'foo',
+      otherParam: 'bar',
+    });
+
+    var AnotherMockContainer = Relay.createContainer(MockPageComponent, {
+      initialVariables: {
+        fragmentParam: null,
+      },
+      fragments: {
+        first: () => Relay.QL`fragment on Node{id}`,
+      },
+    });
 
-    getRelayQueries(MockPageContainer, route);
+    var queries = getRelayQueries(AnotherMockContainer, route);
 
-    expect(MockPageContainer.getFragment.mock.calls.length).toBe(4);
-    expect(MockPageContainer.getFragment.mock.calls[0][1]).toBe(params);
+    expect(queries.first.getVariables()).toEqual(route.params);
+    // `otherParam` is not passed to the root fragment since the variable
+    // is not defined in the component's `initialVariables`.
+    expect(queries.first.getChildren()[0].getVariables()).toEqual({
+      fragmentParam: 'foo',
+    });
   });
 });

src/container/getRelayQueries.js

@@ -68,8 +68,8 @@ function getRelayQueries(
       var concreteQuery = buildRQL.Query(
         queryBuilder,
         Component,
-        Object.keys(route.params),
-        Component.getFragment(queryName, route.params)
+        queryName,
+        route.params
       );
       invariant(
         concreteQuery !== undefined,

src/mutation/RelayMutation.js

@@ -377,7 +377,7 @@ function buildMutationFragment(
 ): GraphQL.Fragment {
   var fragment = buildRQL.Fragment(
     fragmentBuilder,
-    Object.keys(variables)
+    variables
   );
   invariant(
     fragment,

src/query/RelayFragmentReference.js

@@ -18,8 +18,8 @@ import type RelayMetaRoute from 'RelayMetaRoute';
 import type {Variables} from 'RelayTypes';
 
 var forEachObject = require('forEachObject');
-var getWeakIdForObject = require('getWeakIdForObject');
 var invariant = require('invariant');
+var warning = require('warning');
 
 type Condition = (variables: Variables) => boolean;
 type FragmentGetter = () => GraphQL.QueryFragment;
@@ -199,7 +199,15 @@ class RelayFragmentReference {
         if (GraphQL.isCallVariable(value)) {
           value = variables[value.callVariableName];
         }
-        if (value !== undefined) {
+        if (value === undefined) {
+          warning(
+            false,
+            'RelayFragmentReference: Variable `%s` is undefined in fragment ' +
+            '`%s`.',
+            name,
+            this._getFragment().name
+          );
+        } else {
           innerVariables[name] = value;
         }
       });
@@ -213,10 +221,6 @@ class RelayFragmentReference {
     return innerVariables;
   }
 
-  getFragmentName(): string {
-    return getWeakIdForObject(this._getFragment());
-  }
-
   isTypeConditional(): boolean {
     return this._isTypeConditional;
   }

src/query/__tests__/RelayFragmentReference-test.js

@@ -14,6 +14,8 @@
 var RelayTestUtils = require('RelayTestUtils');
 RelayTestUtils.unmockRelay();
 
+jest.mock('warning');
+
 var GraphQL = require('GraphQL');
 var Relay = require('Relay');
 var RelayFragmentReference = require('RelayFragmentReference');
@@ -23,7 +25,10 @@ describe('RelayFragmentReference', () => {
   var route;
 
   beforeEach(() => {
+    jest.resetModuleRegistry();
+
     route = new RelayMetaRoute('');
+    jest.addMatchers(RelayTestUtils.matchers);
   });
 
   it('creates fragments with default variables', () => {
@@ -136,4 +141,29 @@ describe('RelayFragmentReference', () => {
     expect(reference.getVariables(route, variables)).toEqual(customVariables);
     expect(prepareVariables).toBeCalledWith({size: 'default'}, route);
   });
+
+  it('warns if a variable is undefined', () => {
+    var node = Relay.QL`fragment on Node{id}`;
+    var reference = new RelayFragmentReference(
+      () => node,
+      {},
+      {
+        dynamic: new GraphQL.CallVariable('dynamic'),
+        static: undefined,
+      }
+    );
+    var variables = {};
+    expect(reference.getFragment(variables)).toBe(node);
+    expect(reference.getVariables(route, variables)).toEqual({});
+    expect([
+      'RelayFragmentReference: Variable `%s` is undefined in fragment `%s`.',
+      'static',
+      node.name
+    ]).toBeWarnedNTimes(1);
+    expect([
+      'RelayFragmentReference: Variable `%s` is undefined in fragment `%s`.',
+      'dynamic',
+      node.name
+    ]).toBeWarnedNTimes(1);
+  });
 });

src/query/__tests__/buildRQL-test.js

@@ -45,7 +45,7 @@ describe('buildRQL', () => {
   });
 
   describe('Fragment()', () => {
-    it('throws if the node is not a fragment', () => {
+    it('returns undefined if the node is not a fragment', () => {
       var builder = () => Relay.QL`
         query {
           node(id:"123") {
@@ -63,7 +63,7 @@ describe('buildRQL', () => {
           ${invalid},
         }
       `;
-      expect(() => buildRQL.Fragment(builder, [])).toFailInvariant(
+      expect(() => buildRQL.Fragment(builder, {})).toFailInvariant(
         'RelayQL: Invalid fragment composition, use ' +
         '`${Child.getFragment(\'name\')}`.'
       );
@@ -78,7 +78,7 @@ describe('buildRQL', () => {
           },
         }
       `;
-      var node = buildRQL.Fragment(builder, ['sizeVariable']);
+      var node = buildRQL.Fragment(builder, {sizeVariable: null});
       expect(GraphQL.isFragment(node)).toBe(true);
 
       // Confirm that `${variables.sizeVariable}` is a variable by applying
@@ -105,20 +105,22 @@ describe('buildRQL', () => {
           },
         }
       `;
-      var node1 = buildRQL.Fragment(builder, ['sizeVariable']);
-      var node2 = buildRQL.Fragment(builder, ['sizeVariable']);
+      var node1 = buildRQL.Fragment(builder, {sizeVariable: null});
+      var node2 = buildRQL.Fragment(builder, {sizeVariable: null});
       expect(node1 === node2).toBe(true);
     });
   });
 
   describe('Query()', () => {
-    it('throws if the node is not a query', () => {
+    it('returns undefined if the node is not a query', () => {
       var builder = (Component, variables) => Relay.QL`
         fragment on Node {
           id,
         }
       `;
-      expect(buildRQL.Query(builder, MockContainer, ['id'])).toBe(undefined);
+      expect(
+        buildRQL.Query(builder, MockContainer, 'foo', {})
+      ).toBe(undefined);
     });
 
     it('creates queries with components and variables', () => {
@@ -130,7 +132,7 @@ describe('buildRQL', () => {
           }
         }
       `;
-      var node = buildRQL.Query(builder, MockContainer, ['id']);
+      var node = buildRQL.Query(builder, MockContainer, 'foo', {id: null});
       expect(GraphQL.isQuery(node)).toBe(true);
 
       // Confirm that `${variables.id}` is a variable by applying variable
@@ -155,8 +157,8 @@ describe('buildRQL', () => {
           }
         }
       `;
-      var node1 = buildRQL.Query(builder, MockContainer, ['id']);
-      var node2 = buildRQL.Query(builder, MockContainer, ['id']);
+      var node1 = buildRQL.Query(builder, MockContainer, 'foo', {id: null});
+      var node2 = buildRQL.Query(builder, MockContainer, 'foo', {id: null});
       expect(node1 === node2).toBe(true);
     });
 
@@ -174,8 +176,8 @@ describe('buildRQL', () => {
           }
         }
       `;
-      var node1 = buildRQL.Query(builder, MockContainer, ['id']);
-      var node2 = buildRQL.Query(builder, MockContainer2, ['id']);
+      var node1 = buildRQL.Query(builder, MockContainer, 'foo', {id: null});
+      var node2 = buildRQL.Query(builder, MockContainer2, 'foo', {id: null});
       expect(node1 === node2).toBe(false);
     });
 
@@ -188,8 +190,8 @@ describe('buildRQL', () => {
       var node = buildRQL.Query(
         builder,
         MockContainer,
-        ['id'],
-        MockContainer.getFragment('foo')
+        'foo',
+        {id: null},
       );
       expect(GraphQL.isQuery(node)).toBe(true);
 
@@ -220,8 +222,8 @@ describe('buildRQL', () => {
         buildRQL.Query(
           builder,
           MockContainer,
-          [],
-          MockContainer.getFragment('foo')
+          'foo',
+          {}
         );
       }).toFailInvariant(
         'Relay.QL: Expected query `viewer` to be empty. For example, use ' +

src/query/buildRQL.js

@@ -16,18 +16,17 @@
 var GraphQL = require('GraphQL');
 var Map = require('Map');
 var RelayQL = require('RelayQL');
-var RelayFragmentReference = require('RelayFragmentReference');
 import type {RelayConcreteNode} from 'RelayQL';
 import type {Params} from 'RelayRoute';
 import type {RelayContainer, Variables} from 'RelayTypes';
 
+var filterObject = require('filterObject');
 var invariant = require('invariant');
+var mapObject = require('mapObject');
 
 export type FragmentBuilder = (variables: Variables) => RelayConcreteNode;
 export type QueryBuilder =
-  (Component: RelayContainer, params: Params) => RelayConcreteNode;
-
-type VariableNames = Array<string>;
+  (Component?: RelayContainer, params?: Params) => RelayConcreteNode;
 
 // Cache results of executing fragment query builders.
 var fragmentCache = new Map();
@@ -65,11 +64,11 @@ function isDeprecatedCallWithArgCountGreaterThan(
 var buildRQL = {
   Fragment(
     fragmentBuilder: FragmentBuilder,
-    variableNames: VariableNames
+    values: Variables
   ): ?GraphQL.QueryFragment {
     var node = fragmentCache.get(fragmentBuilder);
     if (!node) {
-      var variables = toVariables(variableNames);
+      var variables = toVariables(values);
       if (isDeprecatedCallWithArgCountGreaterThan(fragmentBuilder, 1)) {
         // TODO: Delete legacy support, (_, query, variables) => query`...`.
         node = (fragmentBuilder: any)(undefined, RelayQL, variables);
@@ -84,8 +83,8 @@ var buildRQL = {
   Query(
     queryBuilder: QueryBuilder,
     Component: any,
-    variableNames: VariableNames,
-    fragmentReference: RelayFragmentReference
+    queryName: string,
+    values: Variables
   ): ?GraphQL.Query {
     var componentCache = queryCache.get(queryBuilder);
     var node;
@@ -96,14 +95,14 @@ var buildRQL = {
       node = componentCache.get(Component);
     }
     if (!node) {
-      var variables = toVariables(variableNames);
+      var variables = toVariables(values);
       if (isDeprecatedCallWithArgCountGreaterThan(queryBuilder, 2)) {
         // TODO: Delete legacy support, (Component, variables, rql) => rql`...`.
         node = queryBuilder(Component, variables, RelayQL);
       } else if (isDeprecatedCallWithArgCountGreaterThan(queryBuilder, 0)) {
         node = queryBuilder(Component, variables);
       } else {
-        node = queryBuilder(Component, variables);
+        node = queryBuilder();
         if (GraphQL.isQuery(node) && node.fragments.length === 0) {
           var rootCall = node.calls[0];
           if (!node.fields.every(field => field.fields.length === 0)) {
@@ -114,11 +113,14 @@ var buildRQL = {
               rootCall.name
             );
           }
+          var fragmentValues = filterObject(values, (_, name) =>
+            Component.hasVariable(name)
+          );
           node = new GraphQL.Query(
             rootCall.name,
             rootCall.value,
             node.fields,
-            [fragmentReference],
+            [Component.getFragment(queryName, fragmentValues)],
             node.metadata,
             node.name
           );
@@ -133,14 +135,10 @@ var buildRQL = {
   },
 };
 
-function toVariables(variableNames: VariableNames): {
+function toVariables(variables: Variables): {
   [key: string]: GraphQL.CallVariable;
 } {
-  var variables = {};
-  variableNames.forEach(name => {
-    variables[name] = new GraphQL.CallVariable(name);
-  });
-  return variables;
+  return mapObject(variables, (_, name) => new GraphQL.CallVariable(name));
 }
 
 module.exports = buildRQL;