Skip to content

Commit eb4ee4d

Browse files
committed
feat: support fragment arguments in Sangria field sets
Bind fragment variables as compact scopes on the same selection-set graph, reject incompatible spread arguments before fieldless-root canonicalization, and share alpha-equivalent bound proofs. Keep charging unweighted selection scans, field-set expansions, and new proofs as work. Replacing the fallback pairwise fragment comparisons with grouped bound proofs permits lowering the configurable default safety budget from 5,000 to 2,000 units while accepting substantially larger fragment-spread sets. This builds on [the fragment arguments proposal](graphql/graphql-spec#1224) and [its GraphQL.js implementation](#4015).
1 parent 6291c16 commit eb4ee4d

13 files changed

Lines changed: 655 additions & 1280 deletions

src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1411,8 +1411,8 @@ describe('Validate: Overlapping fields can be merged', () => {
14111411
message:
14121412
'Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
14131413
locations: [
1414-
{ line: 3, column: 11 },
14151414
{ line: 7, column: 11 },
1415+
{ line: 3, column: 11 },
14161416
],
14171417
},
14181418
]);
@@ -1435,8 +1435,8 @@ describe('Validate: Overlapping fields can be merged', () => {
14351435
message:
14361436
'Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
14371437
locations: [
1438-
{ line: 3, column: 11 },
14391438
{ line: 10, column: 11 },
1439+
{ line: 3, column: 11 },
14401440
],
14411441
},
14421442
]);
@@ -1456,8 +1456,8 @@ describe('Validate: Overlapping fields can be merged', () => {
14561456
message:
14571457
'Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
14581458
locations: [
1459-
{ line: 3, column: 11 },
14601459
{ line: 7, column: 11 },
1460+
{ line: 3, column: 11 },
14611461
],
14621462
},
14631463
]);
@@ -1477,8 +1477,8 @@ describe('Validate: Overlapping fields can be merged', () => {
14771477
message:
14781478
'Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
14791479
locations: [
1480-
{ line: 3, column: 11 },
14811480
{ line: 7, column: 11 },
1481+
{ line: 3, column: 11 },
14821482
],
14831483
},
14841484
]);
@@ -1498,8 +1498,8 @@ describe('Validate: Overlapping fields can be merged', () => {
14981498
message:
14991499
'Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
15001500
locations: [
1501-
{ line: 3, column: 11 },
15021501
{ line: 7, column: 11 },
1502+
{ line: 3, column: 11 },
15031503
],
15041504
},
15051505
]);
@@ -1521,8 +1521,8 @@ describe('Validate: Overlapping fields can be merged', () => {
15211521
message:
15221522
'Fields "stringListArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
15231523
locations: [
1524-
{ line: 4, column: 13 },
15251524
{ line: 9, column: 11 },
1525+
{ line: 4, column: 13 },
15261526
],
15271527
},
15281528
]);
@@ -1544,8 +1544,8 @@ describe('Validate: Overlapping fields can be merged', () => {
15441544
message:
15451545
'Fields "stringListArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
15461546
locations: [
1547-
{ line: 4, column: 13 },
15481547
{ line: 9, column: 11 },
1548+
{ line: 4, column: 13 },
15491549
],
15501550
},
15511551
]);
@@ -1567,8 +1567,8 @@ describe('Validate: Overlapping fields can be merged', () => {
15671567
message:
15681568
'Fields "complexArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
15691569
locations: [
1570-
{ line: 4, column: 13 },
15711570
{ line: 9, column: 11 },
1571+
{ line: 4, column: 13 },
15721572
],
15731573
},
15741574
]);
@@ -1590,8 +1590,8 @@ describe('Validate: Overlapping fields can be merged', () => {
15901590
message:
15911591
'Fields "complexArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
15921592
locations: [
1593-
{ line: 4, column: 13 },
15941593
{ line: 9, column: 11 },
1594+
{ line: 4, column: 13 },
15951595
],
15961596
},
15971597
]);
@@ -1658,9 +1658,9 @@ describe('Validate: Overlapping fields can be merged', () => {
16581658
'Fields "mother" conflict because subfields "doesKnowCommand" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.',
16591659
locations: [
16601660
{ line: 5, column: 13 },
1661-
{ line: 13, column: 13 },
16621661
{ line: 12, column: 11 },
16631662
{ line: 11, column: 11 },
1663+
{ line: 13, column: 13 },
16641664
],
16651665
},
16661666
]);

src/validation/rules/OverlappingFieldsCanBeMergedHelpers/ComplexityBudget.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { GraphQLError } from '../../../error/GraphQLError.ts';
33
const COMPLEXITY_ERROR = new GraphQLError(
44
'This document is too complex to validate overlapping fields.',
55
);
6-
const MAX_WORK = 5_000;
6+
const MAX_WORK = 2_000;
77

88
/**
99
* Bounds total field collection, graph expansion, and proof work without

src/validation/rules/OverlappingFieldsCanBeMergedHelpers/ConflictDetector.ts

Lines changed: 85 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type {
1414
FieldSelection,
1515
FieldSet,
1616
FieldsForResponseName,
17+
FragmentSpread,
1718
} from './FieldSetGraph.ts';
1819
import { FieldSetGraph } from './FieldSetGraph.ts';
1920
import { FieldSetProofCache } from './FieldSetProofCache.ts';
@@ -39,6 +40,7 @@ export class ConflictDetector {
3940
private _complexityBudget: ComplexityBudget;
4041
private _fieldSetGraph: FieldSetGraph;
4142
private _coveredFieldSets = new WeakSet<FieldSet>();
43+
private _fragmentArgumentChecks?: WeakMap<ReadonlyArray<FieldSet>, boolean>;
4244
private _reporter?: ConflictReporter;
4345
private _responseProofs?: FieldSetProofCache;
4446
private _commonParentProofs?: FieldSetProofCache;
@@ -73,10 +75,13 @@ export class ConflictDetector {
7375
// A checked closure also proves each fragment closure contained in it.
7476
// This prevents the visitor from rebuilding every suffix of a fragment
7577
// chain when it later reaches the fragment definitions themselves.
76-
if (this._coveredFieldSets.has(root)) {
78+
if (this._coveredFieldSets.has(root.template ?? root)) {
7779
return;
7880
}
7981
this._fieldSetGraph.collectFields(root);
82+
if (!this._rootFragmentArgumentsCanMerge([root])) {
83+
return;
84+
}
8085
if (
8186
!root.hasLocalOverlaps &&
8287
(root.fragmentSpreadsByName.size === 0 ||
@@ -89,8 +94,57 @@ export class ConflictDetector {
8994
this._checkResponseLevels(fieldSet);
9095
this._checkFieldsForCommonParents(fieldSet);
9196
for (const chunk of fieldSet.fieldSets) {
92-
this._coveredFieldSets.add(chunk);
97+
// Bindings change variable identities, not the proof shape of the
98+
// fragment body, so reaching one also covers its source template.
99+
this._coveredFieldSets.add(chunk.template ?? chunk);
100+
}
101+
}
102+
103+
private _rootFragmentArgumentsCanMerge(
104+
fieldSets: ReadonlyArray<FieldSet>,
105+
): boolean {
106+
if (!this._fieldSetGraph.usesFragmentArguments) {
107+
return true;
108+
}
109+
const checks = (this._fragmentArgumentChecks ??= new WeakMap());
110+
const cached = checks.get(fieldSets);
111+
if (cached !== undefined) {
112+
return cached;
93113
}
114+
const spreadsByName = new Map<string, Array<FragmentSpread>>();
115+
for (const fieldSet of fieldSets) {
116+
this._fieldSetGraph.collectFields(fieldSet);
117+
for (const [fragmentName, spreads] of fieldSet.fragmentSpreadsByName) {
118+
const combined = spreadsByName.get(fragmentName);
119+
if (combined === undefined) {
120+
spreadsByName.set(fragmentName, spreads.slice());
121+
} else {
122+
combined.push(...spreads);
123+
}
124+
}
125+
}
126+
127+
let canMerge = true;
128+
for (const [fragmentName, spreads] of spreadsByName) {
129+
if (
130+
this._getReporter().reportFragmentArgumentConflict(
131+
fragmentName,
132+
spreads,
133+
)
134+
) {
135+
canMerge = false;
136+
}
137+
}
138+
checks.set(fieldSets, canMerge);
139+
return canMerge;
140+
}
141+
142+
private _getReporter(): ConflictReporter {
143+
return (this._reporter ??= new ConflictReporter(this._context));
144+
}
145+
146+
private _fragmentArgumentsCanMerge(fieldSet: EffectiveFieldSet): boolean {
147+
return this._rootFragmentArgumentsCanMerge(fieldSet.fieldSets);
94148
}
95149

96150
private _checkResponseLevels(root: EffectiveFieldSet): void {
@@ -108,6 +162,9 @@ export class ConflictDetector {
108162
if (!proofs.shouldCheck(check.fieldSet)) {
109163
continue;
110164
}
165+
if (!this._fragmentArgumentsCanMerge(check.fieldSet)) {
166+
continue;
167+
}
111168
for (const [
112169
responseName,
113170
fieldGroups,
@@ -122,6 +179,10 @@ export class ConflictDetector {
122179
if (fields === undefined) {
123180
continue;
124181
}
182+
const childFieldSet = this._getSubfieldSet(fields);
183+
if (childFieldSet === undefined) {
184+
continue;
185+
}
125186
const conflictPath = reporter.getConflictPath(
126187
check.conflictPath,
127188
responseName,
@@ -130,12 +191,7 @@ export class ConflictDetector {
130191
if (conflictPath !== undefined && conflictPath.parent === undefined) {
131192
rootPaths.push(conflictPath);
132193
}
133-
pending.push({
134-
fieldSet: this._fieldSetGraph.getEffectiveFieldSet(
135-
this._fieldSetGraph.getSubfieldFieldSets(fields),
136-
),
137-
conflictPath,
138-
});
194+
pending.push({ fieldSet: childFieldSet, conflictPath });
139195
}
140196
}
141197
for (const conflictPath of rootPaths) {
@@ -147,10 +203,6 @@ export class ConflictDetector {
147203
return new FieldSetProofCache(this._complexityBudget);
148204
}
149205

150-
private _getReporter(): ConflictReporter {
151-
return (this._reporter ??= new ConflictReporter(this._context));
152-
}
153-
154206
private _fieldsWithSameResponseShape(
155207
responseName: string,
156208
fieldGroups: ReadonlyArray<FieldsForResponseName>,
@@ -189,6 +241,19 @@ export class ConflictDetector {
189241
return allFields;
190242
}
191243

244+
private _getSubfieldSet(
245+
fields: ReadonlyArray<FieldSelection>,
246+
): EffectiveFieldSet | undefined {
247+
const roots = this._fieldSetGraph.getSubfieldFieldSets(fields);
248+
if (roots.length === 0) {
249+
return;
250+
}
251+
if (!this._rootFragmentArgumentsCanMerge(roots)) {
252+
return;
253+
}
254+
return this._fieldSetGraph.getEffectiveFieldSet(roots);
255+
}
256+
192257
private _checkFieldsForCommonParents(root: EffectiveFieldSet): void {
193258
const proofs = (this._commonParentProofs ??= this._newProofCache());
194259
const pending: Array<FieldSetCheck> = [
@@ -204,6 +269,9 @@ export class ConflictDetector {
204269
if (!proofs.shouldCheck(check.fieldSet)) {
205270
continue;
206271
}
272+
if (!this._fragmentArgumentsCanMerge(check.fieldSet)) {
273+
continue;
274+
}
207275
for (const [
208276
responseName,
209277
fieldsForName,
@@ -242,6 +310,10 @@ export class ConflictDetector {
242310
) {
243311
return;
244312
}
313+
const childFieldSet = this._getSubfieldSet(fields);
314+
if (childFieldSet === undefined) {
315+
return;
316+
}
245317
const conflictPath = reporter.getConflictPath(
246318
parentPath,
247319
responseName,
@@ -250,11 +322,6 @@ export class ConflictDetector {
250322
if (conflictPath !== undefined && conflictPath.parent === undefined) {
251323
rootPaths.push(conflictPath);
252324
}
253-
pending.push({
254-
fieldSet: this._fieldSetGraph.getEffectiveFieldSet(
255-
this._fieldSetGraph.getSubfieldFieldSets(fields),
256-
),
257-
conflictPath,
258-
});
325+
pending.push({ fieldSet: childFieldSet, conflictPath });
259326
}
260327
}

0 commit comments

Comments
 (0)