From dfb15cf139ef9366962e10d79d16916ce2c028a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20P=C3=A9rez=20Manr=C3=ADquez?= Date: Tue, 24 May 2022 11:00:28 -0400 Subject: [PATCH 1/3] enhances for argtype coercion --- src/RewriteHandler.ts | 2 +- src/rewriters/FieldArgTypeRewriter.ts | 112 +++++++++++++++++++++----- src/rewriters/Rewriter.ts | 7 +- 3 files changed, 97 insertions(+), 24 deletions(-) diff --git a/src/RewriteHandler.ts b/src/RewriteHandler.ts index 505448d..92b3dd2 100644 --- a/src/RewriteHandler.ts +++ b/src/RewriteHandler.ts @@ -39,7 +39,7 @@ export default class RewriteHandler { const isMatch = rewriter.matches(nodeAndVars, parents); if (isMatch) { rewrittenVariables = rewriter.rewriteVariables(rewrittenNodeAndVars, rewrittenVariables); - rewrittenNodeAndVars = rewriter.rewriteQuery(rewrittenNodeAndVars); + rewrittenNodeAndVars = rewriter.rewriteQuery(rewrittenNodeAndVars, rewrittenVariables); const simplePath = extractPath([...parents, rewrittenNodeAndVars.node]); let paths: ReadonlyArray> = [simplePath]; const fragmentDef = parents.find(({ kind }) => kind === 'FragmentDefinition') as diff --git a/src/rewriters/FieldArgTypeRewriter.ts b/src/rewriters/FieldArgTypeRewriter.ts index 294c0ec..11427fb 100644 --- a/src/rewriters/FieldArgTypeRewriter.ts +++ b/src/rewriters/FieldArgTypeRewriter.ts @@ -1,4 +1,12 @@ -import { ArgumentNode, ASTNode, FieldNode, parseType, TypeNode, VariableNode } from 'graphql'; +import { + ArgumentNode, + ValueNode, + ASTNode, + FieldNode, + parseType, + TypeNode, + VariableNode +} from 'graphql'; import { NodeAndVarDefs, nodesMatch } from '../ast'; import { identifyFunc } from '../utils'; import Rewriter, { RewriterOpts, Variables } from './Rewriter'; @@ -7,7 +15,17 @@ interface FieldArgTypeRewriterOpts extends RewriterOpts { argName: string; oldType: string; newType: string; - coerceVariable?: (variable: any) => any; + coerceVariable?: (variable: any, context: { variables: Variables; args: ArgumentNode[] }) => any; + /** + * EXPERIMENTAL: + * This allows to coerce value of argument when their value is not stored in a variable + * but comes in the query node itself. + * NOTE: At the moment, the user has to return the ast value node herself. + */ + coerceArgumentValue?: ( + variable: any, + context: { variables: Variables; args: ArgumentNode[] } + ) => any; } /** @@ -18,7 +36,13 @@ class FieldArgTypeRewriter extends Rewriter { protected argName: string; protected oldTypeNode: TypeNode; protected newTypeNode: TypeNode; - protected coerceVariable: (variable: any) => any; + // Passes context with rest of arguments and variables. + // Quite useful for variable coercion that depends on other arguments/variables + // (e.g., [offset, limit] to [pageSize, pageNumber] coercion) + protected coerceVariable; + // (Experimental): Used to coerce arguments whose value + // does not come in a variable. + protected coerceArgumentValue; constructor(options: FieldArgTypeRewriterOpts) { super(options); @@ -26,6 +50,7 @@ class FieldArgTypeRewriter extends Rewriter { this.oldTypeNode = parseType(options.oldType); this.newTypeNode = parseType(options.newType); this.coerceVariable = options.coerceVariable || identifyFunc; + this.coerceArgumentValue = options.coerceArgumentValue || identifyFunc; } public matches(nodeAndVars: NodeAndVarDefs, parents: ASTNode[]) { @@ -34,34 +59,81 @@ class FieldArgTypeRewriter extends Rewriter { const { variableDefinitions } = nodeAndVars; // is this a field with the correct fieldName and arguments? if (node.kind !== 'Field') return false; - if (node.name.value !== this.fieldName || !node.arguments) return false; + + // If the fieldName doesnt match, but there are matchConditions. + // matchConditions should have higher priority than fieldName to determine a match. + if ((node.name.value !== this.fieldName && !this.matchConditions) || !node.arguments) + return false; // is there an argument with the correct name and type in a variable? const matchingArgument = node.arguments.find(arg => arg.name.value === this.argName); - if (!matchingArgument || matchingArgument.value.kind !== 'Variable') return false; - const varRef = matchingArgument.value.name.value; - // does the referenced variable have the correct type? - for (const varDefinition of variableDefinitions) { - if (varDefinition.variable.name.value === varRef) { - return nodesMatch(this.oldTypeNode, varDefinition.type); + if (!matchingArgument) return false; + + // argument value is stored in a variable + if (matchingArgument.value.kind === 'Variable') { + const varRef = matchingArgument.value.name.value; + // does the referenced variable have the correct type? + for (const varDefinition of variableDefinitions) { + if (varDefinition.variable.name.value === varRef) { + return nodesMatch(this.oldTypeNode, varDefinition.type); + } } } + // argument value comes in query doc. + else { + const argRef = matchingArgument.value; + return nodesMatch(this.oldTypeNode, parseType(argRef.kind)); + } + return false; } - public rewriteQuery({ node, variableDefinitions }: NodeAndVarDefs) { - const varRefName = this.extractMatchingVarRefName(node as FieldNode); - const newVarDefs = variableDefinitions.map(varDef => { - if (varDef.variable.name.value !== varRefName) return varDef; - return { ...varDef, type: this.newTypeNode }; - }); - return { node, variableDefinitions: newVarDefs }; + public rewriteQuery( + { node: astNode, variableDefinitions }: NodeAndVarDefs, + variables: Variables + ) { + const node = astNode as FieldNode; + const varRefName = this.extractMatchingVarRefName(node); + // If argument value is stored in a variable + if (varRefName) { + const newVarDefs = variableDefinitions.map(varDef => { + if (varDef.variable.name.value !== varRefName) return varDef; + return { ...varDef, type: this.newTypeNode }; + }); + return { node, variableDefinitions: newVarDefs }; + } + // If argument value is not stored in a variable but in the query node. + else { + const matchingArgument = (node.arguments || []).find(arg => arg.name.value === this.argName); + if (node.arguments && matchingArgument) { + const args = [...node.arguments]; + const newValue = this.coerceArgumentValue(matchingArgument.value, { variables, args }); + /** + TODO: If somewhow we can get the schema here, we could make the coerceArgumentValue + even easier, as we would be able to construct the ast node for the argument value. + as of now, the user has to take care of correctly constructing the argument value ast node herself. + + const schema = makeExecutableSchema({typeDefs}) + const myCustomType = schema.getType("MY_CUSTOM_TYPE_NAME") + const newArgValue = astFromValue(newValue, myCustomType) + Object.assign(matchingArgument, { value: newArgValue }) + + */ + Object.assign(matchingArgument, { value: newValue }); + } + return { node, variableDefinitions }; + } } - public rewriteVariables({ node }: NodeAndVarDefs, variables: Variables) { + public rewriteVariables({ node: astNode }: NodeAndVarDefs, variables: Variables) { + const node = astNode as FieldNode; if (!variables) return variables; - const varRefName = this.extractMatchingVarRefName(node as FieldNode); - return { ...variables, [varRefName]: this.coerceVariable(variables[varRefName]) }; + const varRefName = this.extractMatchingVarRefName(node); + const args = [...(node.arguments ? node.arguments : [])]; + return { + ...variables, + [varRefName]: this.coerceVariable(variables[varRefName], { variables, args }) + }; } private extractMatchingVarRefName(node: FieldNode) { diff --git a/src/rewriters/Rewriter.ts b/src/rewriters/Rewriter.ts index f33d659..ccca7db 100644 --- a/src/rewriters/Rewriter.ts +++ b/src/rewriters/Rewriter.ts @@ -28,7 +28,8 @@ abstract class Rewriter { public matches(nodeAndVarDefs: NodeAndVarDefs, parents: ReadonlyArray): boolean { const { node } = nodeAndVarDefs; - if (node.kind !== 'Field' || node.name.value !== this.fieldName) return false; + if (node.kind !== 'Field' || (node.name.value !== this.fieldName && !this.matchConditions)) + return false; const root = parents[0]; if ( root.kind === 'OperationDefinition' && @@ -48,11 +49,11 @@ abstract class Rewriter { return true; } - public rewriteQuery(nodeAndVarDefs: NodeAndVarDefs): NodeAndVarDefs { + public rewriteQuery(nodeAndVarDefs: NodeAndVarDefs, _?: Variables): NodeAndVarDefs { return nodeAndVarDefs; } - public rewriteVariables(nodeAndVarDefs: NodeAndVarDefs, variables: Variables): Variables { + public rewriteVariables(_: NodeAndVarDefs, variables: Variables): Variables { return variables; } From b73c68a48cfe46aafd13f4f220c0a73bd1f12840 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20P=C3=A9rez=20Manr=C3=ADquez?= Date: Tue, 24 May 2022 15:48:07 -0400 Subject: [PATCH 2/3] Adds unit tests for new functionalities --- src/rewriters/FieldArgTypeRewriter.ts | 48 +++++-- src/rewriters/Rewriter.ts | 18 ++- test/functional/rewriteFieldArgType.test.ts | 140 ++++++++++++++++++++ 3 files changed, 189 insertions(+), 17 deletions(-) diff --git a/src/rewriters/FieldArgTypeRewriter.ts b/src/rewriters/FieldArgTypeRewriter.ts index 11427fb..aac6854 100644 --- a/src/rewriters/FieldArgTypeRewriter.ts +++ b/src/rewriters/FieldArgTypeRewriter.ts @@ -5,8 +5,11 @@ import { FieldNode, parseType, TypeNode, - VariableNode + VariableNode, + Kind, + isValueNode } from 'graphql'; +import Maybe from 'graphql/tsutils/Maybe'; import { NodeAndVarDefs, nodesMatch } from '../ast'; import { identifyFunc } from '../utils'; import Rewriter, { RewriterOpts, Variables } from './Rewriter'; @@ -25,7 +28,7 @@ interface FieldArgTypeRewriterOpts extends RewriterOpts { coerceArgumentValue?: ( variable: any, context: { variables: Variables; args: ArgumentNode[] } - ) => any; + ) => Maybe; } /** @@ -39,10 +42,16 @@ class FieldArgTypeRewriter extends Rewriter { // Passes context with rest of arguments and variables. // Quite useful for variable coercion that depends on other arguments/variables // (e.g., [offset, limit] to [pageSize, pageNumber] coercion) - protected coerceVariable; + protected coerceVariable: ( + variable: any, + context: { variables: Variables; args: ArgumentNode[] } + ) => any; // (Experimental): Used to coerce arguments whose value // does not come in a variable. - protected coerceArgumentValue; + protected coerceArgumentValue: ( + variable: any, + context: { variables: Variables; args: ArgumentNode[] } + ) => Maybe; constructor(options: FieldArgTypeRewriterOpts) { super(options); @@ -60,10 +69,9 @@ class FieldArgTypeRewriter extends Rewriter { // is this a field with the correct fieldName and arguments? if (node.kind !== 'Field') return false; - // If the fieldName doesnt match, but there are matchConditions. - // matchConditions should have higher priority than fieldName to determine a match. - if ((node.name.value !== this.fieldName && !this.matchConditions) || !node.arguments) - return false; + // does this field contain arguments? + if (!node.arguments) return false; + // is there an argument with the correct name and type in a variable? const matchingArgument = node.arguments.find(arg => arg.name.value === this.argName); @@ -81,8 +89,15 @@ class FieldArgTypeRewriter extends Rewriter { } // argument value comes in query doc. else { - const argRef = matchingArgument.value; - return nodesMatch(this.oldTypeNode, parseType(argRef.kind)); + const argValueNode = matchingArgument.value; + return isValueNode(argValueNode); + // Would be ideal to do a nodesMatch in here, however argument value nodes + // have different format for their values than when passed as variables. + // For instance, are parsed with Kinds as "graphql.Kind" (e.g., INT="IntValue") and not "graphql.TokenKinds" (e.g., INT="Int") + // So they might not match correctly. Also they dont contain additional parsed syntax + // as the non-optional symbol "!". So just return true if the argument.value is a ValueNode. + // + // return nodesMatch(this.oldTypeNode, parseType(argRef.kind)); } return false; @@ -119,7 +134,7 @@ class FieldArgTypeRewriter extends Rewriter { Object.assign(matchingArgument, { value: newArgValue }) */ - Object.assign(matchingArgument, { value: newValue }); + if (newValue) Object.assign(matchingArgument, { value: newValue }); } return { node, variableDefinitions }; } @@ -132,13 +147,18 @@ class FieldArgTypeRewriter extends Rewriter { const args = [...(node.arguments ? node.arguments : [])]; return { ...variables, - [varRefName]: this.coerceVariable(variables[varRefName], { variables, args }) + ...(varRefName + ? { [varRefName]: this.coerceVariable(variables[varRefName], { variables, args }) } + : {}) }; } private extractMatchingVarRefName(node: FieldNode) { - const matchingArgument = (node.arguments || []).find(arg => arg.name.value === this.argName); - return ((matchingArgument as ArgumentNode).value as VariableNode).name.value; + const matchingArgument = ( + (node.arguments || []).find(arg => arg.name.value === this.argName) + ); + const variableNode = matchingArgument.value; + return variableNode.kind === Kind.VARIABLE && variableNode.name.value; } } diff --git a/src/rewriters/Rewriter.ts b/src/rewriters/Rewriter.ts index ccca7db..4ed5d29 100644 --- a/src/rewriters/Rewriter.ts +++ b/src/rewriters/Rewriter.ts @@ -6,7 +6,7 @@ export type Variables = { [key: string]: any } | undefined; export type RootType = 'query' | 'mutation' | 'fragment'; export interface RewriterOpts { - fieldName: string; + fieldName?: string; rootTypes?: RootType[]; matchConditions?: matchCondition[]; } @@ -16,19 +16,31 @@ export interface RewriterOpts { * Extend this class and overwrite its methods to create a new rewriter */ abstract class Rewriter { - protected fieldName: string; protected rootTypes: RootType[] = ['query', 'mutation', 'fragment']; + protected fieldName?: string; protected matchConditions?: matchCondition[]; constructor({ fieldName, rootTypes, matchConditions }: RewriterOpts) { this.fieldName = fieldName; this.matchConditions = matchConditions; + if (!this.fieldName && !this.matchConditions) { + throw new Error( + 'Neither a fieldName or matchConditions were provided. Please choose to pass either one in order to be able to detect which fields to rewrite.' + ); + } if (rootTypes) this.rootTypes = rootTypes; } public matches(nodeAndVarDefs: NodeAndVarDefs, parents: ReadonlyArray): boolean { const { node } = nodeAndVarDefs; - if (node.kind !== 'Field' || (node.name.value !== this.fieldName && !this.matchConditions)) + + // If no fieldName is provided, check for defined matchConditions. + // This avoids having to define one rewriter for many fields individually. + // Alternatively, regex matching for fieldName could be implemented. + if ( + node.kind !== 'Field' || + (this.fieldName ? node.name.value !== this.fieldName : !this.matchConditions) + ) return false; const root = parents[0]; if ( diff --git a/test/functional/rewriteFieldArgType.test.ts b/test/functional/rewriteFieldArgType.test.ts index 9f8d173..af47e1c 100644 --- a/test/functional/rewriteFieldArgType.test.ts +++ b/test/functional/rewriteFieldArgType.test.ts @@ -1,3 +1,4 @@ +import { FieldNode, astFromValue, GraphQLInt, Kind } from 'graphql'; import RewriteHandler from '../../src/RewriteHandler'; import FieldArgTypeRewriter from '../../src/rewriters/FieldArgTypeRewriter'; import { gqlFmt } from '../testUtils'; @@ -98,6 +99,145 @@ describe('Rewrite field arg type', () => { }); }); + it('variable coercion comes with additional variables and arguments as context.', () => { + const query = gqlFmt` + query doTheThings($arg1: String!, $arg2: String) { + things(identifier: $arg1, arg2: $arg2, arg3: "blah") { + cat + } + } + `; + const expectedRewritenQuery = gqlFmt` + query doTheThings($arg1: Int!, $arg2: String) { + things(identifier: $arg1, arg2: $arg2, arg3: "blah") { + cat + } + } + `; + + const handler = new RewriteHandler([ + new FieldArgTypeRewriter({ + fieldName: 'things', + argName: 'identifier', + oldType: 'String!', + newType: 'Int!', + coerceVariable: (_, { variables = {}, args }) => { + expect(args.length).toBe(3); + expect(args[0].kind).toBe('Argument'); + expect(args[0].value.kind).toBe(Kind.VARIABLE); + expect(args[1].kind).toBe('Argument'); + expect(args[1].value.kind).toBe(Kind.VARIABLE); + expect(args[2].kind).toBe('Argument'); + expect(args[2].value.kind).toBe(Kind.STRING); + const { arg2 = 0 } = variables; + return parseInt(arg2, 10); + } + }) + ]); + expect(handler.rewriteRequest(query, { arg1: 'someString', arg2: '123' })).toEqual({ + query: expectedRewritenQuery, + variables: { + arg1: 123, + arg2: '123' + } + }); + }); + + it('can be passed a coerceArgumentValue function to change argument values.', () => { + const query = gqlFmt` + query doTheThings { + things(identifier: "123", arg2: "blah") { + cat + } + } + `; + const expectedRewritenQuery = gqlFmt` + query doTheThings { + things(identifier: 123, arg2: "blah") { + cat + } + } + `; + + const handler = new RewriteHandler([ + new FieldArgTypeRewriter({ + fieldName: 'things', + argName: 'identifier', + oldType: 'String!', + newType: 'Int!', + coerceArgumentValue: argValue => { + const value = argValue.value; + const newArgValue = astFromValue(parseInt(value, 10), GraphQLInt); + return newArgValue; + } + }) + ]); + + expect(handler.rewriteRequest(query)).toEqual({ + query: expectedRewritenQuery + }); + }); + + it('should fail if neither a fieldName or matchConditions are provided', () => { + try { + new FieldArgTypeRewriter({ + argName: 'identifier', + oldType: 'String!', + newType: 'Int!' + }); + } catch (error) { + console.log(error.message); + expect( + error.message.includes('Neither a fieldName or matchConditions were provided') + ).toEqual(true); + } + }); + + it('allows matching using matchConditions when fieldName is not provided.', () => { + const query = gqlFmt` + query doTheThings($arg1: String!, $arg2: String) { + things(identifier: $arg1, arg2: $arg2) { + cat + } + } + `; + const expectedRewritenQuery = gqlFmt` + query doTheThings($arg1: Int!, $arg2: String) { + things(identifier: $arg1, arg2: $arg2) { + cat + } + } + `; + + // Tests a dummy regex to match the "things" field. + const fieldNameRegExp = '.hings'; + + const handler = new RewriteHandler([ + new FieldArgTypeRewriter({ + argName: 'identifier', + oldType: 'String!', + newType: 'Int!', + matchConditions: [ + nodeAndVars => { + const node = nodeAndVars.node as FieldNode; + const { + name: { value: fieldName } + } = node; + return fieldName.search(new RegExp(fieldNameRegExp)) !== -1; + } + ], + coerceVariable: val => parseInt(val, 10) + }) + ]); + expect(handler.rewriteRequest(query, { arg1: '123', arg2: 'blah' })).toEqual({ + query: expectedRewritenQuery, + variables: { + arg1: 123, + arg2: 'blah' + } + }); + }); + it('works on deeply nested fields', () => { const query = gqlFmt` query doTheThings($arg1: String!, $arg2: String) { From 6717867bc07ba39626fcc60ae86511aa6b59e404 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20P=C3=A9rez=20Manr=C3=ADquez?= Date: Thu, 26 May 2022 10:17:54 -0400 Subject: [PATCH 3/3] fixes linting issues --- src/rewriters/FieldArgTypeRewriter.ts | 51 +++++++++++++-------------- src/rewriters/Rewriter.ts | 7 ++-- 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/src/rewriters/FieldArgTypeRewriter.ts b/src/rewriters/FieldArgTypeRewriter.ts index aac6854..dbac77a 100644 --- a/src/rewriters/FieldArgTypeRewriter.ts +++ b/src/rewriters/FieldArgTypeRewriter.ts @@ -1,13 +1,13 @@ import { ArgumentNode, - ValueNode, ASTNode, FieldNode, + isValueNode, + Kind, parseType, TypeNode, - VariableNode, - Kind, - isValueNode + ValueNode, + VariableNode } from 'graphql'; import Maybe from 'graphql/tsutils/Maybe'; import { NodeAndVarDefs, nodesMatch } from '../ast'; @@ -118,26 +118,23 @@ class FieldArgTypeRewriter extends Rewriter { return { node, variableDefinitions: newVarDefs }; } // If argument value is not stored in a variable but in the query node. - else { - const matchingArgument = (node.arguments || []).find(arg => arg.name.value === this.argName); - if (node.arguments && matchingArgument) { - const args = [...node.arguments]; - const newValue = this.coerceArgumentValue(matchingArgument.value, { variables, args }); - /** - TODO: If somewhow we can get the schema here, we could make the coerceArgumentValue - even easier, as we would be able to construct the ast node for the argument value. - as of now, the user has to take care of correctly constructing the argument value ast node herself. - - const schema = makeExecutableSchema({typeDefs}) - const myCustomType = schema.getType("MY_CUSTOM_TYPE_NAME") - const newArgValue = astFromValue(newValue, myCustomType) - Object.assign(matchingArgument, { value: newArgValue }) - - */ - if (newValue) Object.assign(matchingArgument, { value: newValue }); - } - return { node, variableDefinitions }; + const matchingArgument = (node.arguments || []).find(arg => arg.name.value === this.argName); + if (node.arguments && matchingArgument) { + const args = [...node.arguments]; + const newValue = this.coerceArgumentValue(matchingArgument.value, { variables, args }); + /** + * TODO: If somewhow we can get the schema here, we could make the coerceArgumentValue + * even easier, as we would be able to construct the ast node for the argument value. + * as of now, the user has to take care of correctly constructing the argument value ast node herself. + * + * const schema = makeExecutableSchema({typeDefs}) + * const myCustomType = schema.getType("MY_CUSTOM_TYPE_NAME") + * const newArgValue = astFromValue(newValue, myCustomType) + * Object.assign(matchingArgument, { value: newArgValue }) + */ + if (newValue) Object.assign(matchingArgument, { value: newValue }); } + return { node, variableDefinitions }; } public rewriteVariables({ node: astNode }: NodeAndVarDefs, variables: Variables) { @@ -154,10 +151,10 @@ class FieldArgTypeRewriter extends Rewriter { } private extractMatchingVarRefName(node: FieldNode) { - const matchingArgument = ( - (node.arguments || []).find(arg => arg.name.value === this.argName) - ); - const variableNode = matchingArgument.value; + const matchingArgument = (node.arguments || []).find( + arg => arg.name.value === this.argName + ) as ArgumentNode; + const variableNode = matchingArgument.value as VariableNode; return variableNode.kind === Kind.VARIABLE && variableNode.name.value; } } diff --git a/src/rewriters/Rewriter.ts b/src/rewriters/Rewriter.ts index 4ed5d29..f138596 100644 --- a/src/rewriters/Rewriter.ts +++ b/src/rewriters/Rewriter.ts @@ -40,8 +40,9 @@ abstract class Rewriter { if ( node.kind !== 'Field' || (this.fieldName ? node.name.value !== this.fieldName : !this.matchConditions) - ) + ) { return false; + } const root = parents[0]; if ( root.kind === 'OperationDefinition' && @@ -61,11 +62,11 @@ abstract class Rewriter { return true; } - public rewriteQuery(nodeAndVarDefs: NodeAndVarDefs, _?: Variables): NodeAndVarDefs { + public rewriteQuery(nodeAndVarDefs: NodeAndVarDefs, variables: Variables): NodeAndVarDefs { return nodeAndVarDefs; } - public rewriteVariables(_: NodeAndVarDefs, variables: Variables): Variables { + public rewriteVariables(nodeAndVarDefs: NodeAndVarDefs, variables: Variables): Variables { return variables; }