Skip to content

Commit

Permalink
simplify fragment args execution
Browse files Browse the repository at this point in the history 
by creating a map for each fragment with fragment variables with the properly combined local and global variables

background: this was in the original PR graphql#4015, was taken out by me when i misunderstood and thought that when a fragment variable shadows an operation variable, the operation variable could still leak through if the fragment variable had no default, and I thought it was necessary within getArgumentValues to still have the signatures

advantages to the original approach, which this PR restores: does not change the signature of getVariableValues, getDirectiveValues, valueFromAST

disadvantages:
creating a combined variable map of local and global variables may be a waste if the global variables are not actually used in that fragment
  • Loading branch information
@yaacovCR
yaacovCR committed Sep 8, 2024
1 parent 1dbdadc commit efbaf64
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 98 deletions.
82 changes: 35 additions & 47 deletions src/execution/collectFields.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,10 @@ export interface DeferUsage {
parentDeferUsage: DeferUsage | undefined;
}

export interface FragmentVariables {
signatures: ObjMap<GraphQLVariableSignature>;
values: ObjMap<unknown>;
}

export interface FieldDetails {
node: FieldNode;
deferUsage?: DeferUsage | undefined;
fragmentVariables?: FragmentVariables | undefined;
fragmentVariableValues?: { [variable: string]: unknown } | undefined;
}

export type FieldGroup = ReadonlyArray<FieldDetails>;
Expand Down Expand Up @@ -136,14 +131,14 @@ export function collectSubfields(
for (const fieldDetail of fieldGroup) {
const selectionSet = fieldDetail.node.selectionSet;
if (selectionSet) {
const { deferUsage, fragmentVariables } = fieldDetail;
const { deferUsage, fragmentVariableValues } = fieldDetail;
collectFieldsImpl(
context,
selectionSet,
subGroupedFieldSet,
newDeferUsages,
deferUsage,
fragmentVariables,
fragmentVariableValues,
);
}
}
Expand All @@ -161,7 +156,7 @@ function collectFieldsImpl(
groupedFieldSet: AccumulatorMap<string, FieldDetails>,
newDeferUsages: Array<DeferUsage>,
deferUsage?: DeferUsage,
fragmentVariables?: FragmentVariables,
fragmentVariableValues?: { [variable: string]: unknown },
): void {
const {
schema,
Expand All @@ -172,31 +167,32 @@ function collectFieldsImpl(
visitedFragmentNames,
} = context;

const scopedVariableValues = fragmentVariableValues ?? variableValues;

for (const selection of selectionSet.selections) {
switch (selection.kind) {
case Kind.FIELD: {
if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) {
if (!shouldIncludeNode(scopedVariableValues, selection)) {
continue;
}
groupedFieldSet.add(getFieldEntryKey(selection), {
node: selection,
deferUsage,
fragmentVariables,
fragmentVariableValues,
});
break;
}
case Kind.INLINE_FRAGMENT: {
if (
!shouldIncludeNode(selection, variableValues, fragmentVariables) ||
!shouldIncludeNode(scopedVariableValues, selection) ||
!doesFragmentConditionMatch(schema, selection, runtimeType)
) {
continue;
}

const newDeferUsage = getDeferUsage(
operation,
variableValues,
fragmentVariables,
fragmentVariableValues ?? variableValues,
selection,
deferUsage,
);
Expand All @@ -208,7 +204,7 @@ function collectFieldsImpl(
groupedFieldSet,
newDeferUsages,
deferUsage,
fragmentVariables,
fragmentVariableValues,
);
} else {
newDeferUsages.push(newDeferUsage);
Expand All @@ -218,7 +214,7 @@ function collectFieldsImpl(
groupedFieldSet,
newDeferUsages,
newDeferUsage,
fragmentVariables,
fragmentVariableValues,
);
}

Expand All @@ -229,16 +225,18 @@ function collectFieldsImpl(

const newDeferUsage = getDeferUsage(
operation,
variableValues,
fragmentVariables,
scopedVariableValues,
selection,
deferUsage,
);

if (
!newDeferUsage &&
(visitedFragmentNames.has(fragName) ||
!shouldIncludeNode(selection, variableValues, fragmentVariables))
!shouldIncludeNode(
fragmentVariableValues ?? variableValues,
selection,
))
) {
continue;
}
Expand All @@ -252,17 +250,20 @@ function collectFieldsImpl(
}

const fragmentVariableSignatures = fragment.variableSignatures;
let newFragmentVariables: FragmentVariables | undefined;
let newFragmentVariableValues:
| { [variable: string]: unknown }
| undefined;
if (fragmentVariableSignatures) {
newFragmentVariables = {
signatures: fragmentVariableSignatures,
values: experimentalGetArgumentValues(
selection,
Object.values(fragmentVariableSignatures),
variableValues,
fragmentVariables,
),
};
newFragmentVariableValues = experimentalGetArgumentValues(
selection,
Object.values(fragmentVariableSignatures),
scopedVariableValues,
);
for (const [variableName, value] of Object.entries(variableValues)) {
if (!fragment.variableSignatures?.[variableName]) {
newFragmentVariableValues[variableName] = value;
}
}
}

if (!newDeferUsage) {
Expand All @@ -273,7 +274,7 @@ function collectFieldsImpl(
groupedFieldSet,
newDeferUsages,
deferUsage,
newFragmentVariables,
newFragmentVariableValues,
);
} else {
newDeferUsages.push(newDeferUsage);
Expand All @@ -283,7 +284,7 @@ function collectFieldsImpl(
groupedFieldSet,
newDeferUsages,
newDeferUsage,
newFragmentVariables,
newFragmentVariableValues,
);
}
break;
Expand All @@ -300,16 +301,10 @@ function collectFieldsImpl(
function getDeferUsage(
operation: OperationDefinitionNode,
variableValues: { [variable: string]: unknown },
fragmentVariables: FragmentVariables | undefined,
node: FragmentSpreadNode | InlineFragmentNode,
parentDeferUsage: DeferUsage | undefined,
): DeferUsage | undefined {
const defer = getDirectiveValues(
GraphQLDeferDirective,
node,
variableValues,
fragmentVariables,
);
const defer = getDirectiveValues(GraphQLDeferDirective, node, variableValues);

if (!defer) {
return;
Expand All @@ -335,16 +330,10 @@ function getDeferUsage(
* directives, where `@skip` has higher precedence than `@include`.
*/
function shouldIncludeNode(
node: FragmentSpreadNode | FieldNode | InlineFragmentNode,
variableValues: { [variable: string]: unknown },
fragmentVariables: FragmentVariables | undefined,
node: FragmentSpreadNode | FieldNode | InlineFragmentNode,
): boolean {
const skip = getDirectiveValues(
GraphQLSkipDirective,
node,
variableValues,
fragmentVariables,
);
const skip = getDirectiveValues(GraphQLSkipDirective, node, variableValues);
if (skip?.if === true) {
return false;
}
Expand All @@ -353,7 +342,6 @@ function shouldIncludeNode(
GraphQLIncludeDirective,
node,
variableValues,
fragmentVariables,
);
if (include?.if === false) {
return false;
Expand Down
8 changes: 3 additions & 5 deletions src/execution/execute.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,8 +752,7 @@ function executeField(
const args = experimentalGetArgumentValues(
fieldGroup[0].node,
fieldDef.args,
exeContext.variableValues,
fieldGroup[0].fragmentVariables,
fieldGroup[0].fragmentVariableValues ?? exeContext.variableValues,
);

// The resolve function's optional third argument is a context value that
Expand Down Expand Up @@ -1061,8 +1060,7 @@ function getStreamUsage(
const stream = getDirectiveValues(
GraphQLStreamDirective,
fieldGroup[0].node,
exeContext.variableValues,
fieldGroup[0].fragmentVariables,
fieldGroup[0].fragmentVariableValues ?? exeContext.variableValues,
);

if (!stream) {
Expand Down Expand Up @@ -1091,7 +1089,7 @@ function getStreamUsage(
const streamedFieldGroup: FieldGroup = fieldGroup.map((fieldDetails) => ({
node: fieldDetails.node,
deferUsage: undefined,
fragmentVariables: fieldDetails.fragmentVariables,
fragmentVariableValues: fieldDetails.fragmentVariableValues,
}));

const streamUsage = {
Expand Down
20 changes: 4 additions & 16 deletions src/execution/values.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import type { GraphQLSchema } from '../type/schema.js';
import { coerceInputValue } from '../utilities/coerceInputValue.js';
import { valueFromAST } from '../utilities/valueFromAST.js';

import type { FragmentVariables } from './collectFields.js';
import type { GraphQLVariableSignature } from './getVariableSignature.js';
import { getVariableSignature } from './getVariableSignature.js';

Expand Down Expand Up @@ -156,7 +155,6 @@ export function experimentalGetArgumentValues(
node: FieldNode | DirectiveNode | FragmentSpreadNode,
argDefs: ReadonlyArray<GraphQLArgument | GraphQLVariableSignature>,
variableValues: Maybe<ObjMap<unknown>>,
fragmentVariables?: Maybe<FragmentVariables>,
): { [argument: string]: unknown } {
const coercedValues: { [argument: string]: unknown } = {};

Expand Down Expand Up @@ -188,12 +186,9 @@ export function experimentalGetArgumentValues(

if (valueNode.kind === Kind.VARIABLE) {
const variableName = valueNode.name.value;
const scopedVariableValues = fragmentVariables?.signatures[variableName]
? fragmentVariables.values
: variableValues;
if (
scopedVariableValues == null ||
!Object.hasOwn(scopedVariableValues, variableName)
variableValues == null ||
!Object.hasOwn(variableValues, variableName)
) {
if (argDef.defaultValue !== undefined) {
coercedValues[name] = argDef.defaultValue;
Expand All @@ -206,7 +201,7 @@ export function experimentalGetArgumentValues(
}
continue;
}
isNull = scopedVariableValues[variableName] == null;
isNull = variableValues[variableName] == null;
}

if (isNull && isNonNullType(argType)) {
Expand All @@ -217,12 +212,7 @@ export function experimentalGetArgumentValues(
);
}

const coercedValue = valueFromAST(
valueNode,
argType,
variableValues,
fragmentVariables?.values,
);
const coercedValue = valueFromAST(valueNode, argType, variableValues);
if (coercedValue === undefined) {
// Note: ValuesOfCorrectTypeRule validation should catch this before
// execution. This is a runtime check to ensure execution does not
Expand Down Expand Up @@ -254,7 +244,6 @@ export function getDirectiveValues(
directiveDef: GraphQLDirective,
node: { readonly directives?: ReadonlyArray<DirectiveNode> | undefined },
variableValues?: Maybe<ObjMap<unknown>>,
fragmentVariables?: Maybe<FragmentVariables>,
): undefined | { [argument: string]: unknown } {
const directiveNode = node.directives?.find(
(directive) => directive.name.value === directiveDef.name,
Expand All @@ -265,7 +254,6 @@ export function getDirectiveValues(
directiveNode,
directiveDef.args,
variableValues,
fragmentVariables,
);
}
}
Loading

0 comments on commit efbaf64

Please sign in to comment.