From 6cc8f96cdb7dcebebb38f30b7d64b08dfd83fb18 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 8 Sep 2015 18:18:39 -0700 Subject: [PATCH] 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 | 4 +- .../__tests__/getRelayQueries-test.js | 43 +++++++++++++++---- src/container/getRelayQueries.js | 4 +- src/mutation/RelayMutation.js | 2 +- src/query/RelayFragmentReference.js | 16 ++++--- .../__tests__/RelayFragmentReference-test.js | 30 +++++++++++++ src/query/__tests__/buildRQL-test.js | 34 ++++++++------- src/query/buildRQL.js | 32 +++++++------- 8 files changed, 113 insertions(+), 52 deletions(-) diff --git a/src/container/RelayContainer.js b/src/container/RelayContainer.js index d09bcb7d6e294..623937c06fa71 100644 --- a/src/container/RelayContainer.js +++ b/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 diff --git a/src/container/__tests__/getRelayQueries-test.js b/src/container/__tests__/getRelayQueries-test.js index 32acecd448541..9500edd16deea 100644 --- a/src/container/__tests__/getRelayQueries-test.js +++ b/src/container/__tests__/getRelayQueries-test.js @@ -35,7 +35,7 @@ describe('getRelayQueries', () => { render() { return
; } - } + }; 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', + }); }); }); diff --git a/src/container/getRelayQueries.js b/src/container/getRelayQueries.js index 9336d6df197b4..44474568d55e7 100644 --- a/src/container/getRelayQueries.js +++ b/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, diff --git a/src/mutation/RelayMutation.js b/src/mutation/RelayMutation.js index cd599e3d507c4..c6a54861434bc 100644 --- a/src/mutation/RelayMutation.js +++ b/src/mutation/RelayMutation.js @@ -377,7 +377,7 @@ function buildMutationFragment( ): GraphQL.Fragment { var fragment = buildRQL.Fragment( fragmentBuilder, - Object.keys(variables) + variables ); invariant( fragment, diff --git a/src/query/RelayFragmentReference.js b/src/query/RelayFragmentReference.js index 0aca762a8f8c0..307a90b1db324 100644 --- a/src/query/RelayFragmentReference.js +++ b/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; } diff --git a/src/query/__tests__/RelayFragmentReference-test.js b/src/query/__tests__/RelayFragmentReference-test.js index e627d895a70bd..89cb83c20c247 100644 --- a/src/query/__tests__/RelayFragmentReference-test.js +++ b/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); + }); }); diff --git a/src/query/__tests__/buildRQL-test.js b/src/query/__tests__/buildRQL-test.js index 7f18cac20e6b5..8cb2919b6c1c8 100644 --- a/src/query/__tests__/buildRQL-test.js +++ b/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 ' + diff --git a/src/query/buildRQL.js b/src/query/buildRQL.js index 1d6f63c0d5aa2..abe3b8669a845 100644 --- a/src/query/buildRQL.js +++ b/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; + (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;