Skip to content

Commit 6f7266b

Browse files
trevor-scheertrevor-scheer
committed
Progressive @override (composition only) (#2879)
Add new optional `label` arg to `@override` which is a `String`. Capture label in the supergraph via the new `@join__field` arg `overrideLabel` so these values can be used during query graph creation and query planning.
1 parent 66833fb commit 6f7266b

File tree

10 files changed

+275
-23
lines changed

10 files changed

+275
-23
lines changed

‎composition-js/src/__tests__/compose.test.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ describe('composition', () => {
8080
8181
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
8282
83-
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
83+
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
8484
8585
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
8686
@@ -232,7 +232,7 @@ describe('composition', () => {
232232
233233
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
234234
235-
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
235+
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
236236
237237
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
238238

‎composition-js/src/__tests__/override.compose.test.ts

+181-6
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,7 @@ describe("composition involving @override directive", () => {
502502

503503
const result = composeAsFed2Subgraphs([subgraph1, subgraph2]);
504504
expect(result.errors).toBeDefined();
505-
expect(result.errors?.map(e => e.message)).toMatchStringArray([
505+
expect(result.errors?.map((e) => e.message)).toMatchStringArray([
506506
`
507507
The following supergraph API query:
508508
{
@@ -526,7 +526,7 @@ describe("composition involving @override directive", () => {
526526
- from subgraph "Subgraph2":
527527
- cannot find field "T.a".
528528
- cannot move to subgraph "Subgraph1" using @key(fields: "k") of "T", the key field(s) cannot be resolved from subgraph "Subgraph2" (note that some of those key fields are overridden in "Subgraph2").
529-
`
529+
`,
530530
]);
531531
});
532532

@@ -562,9 +562,9 @@ describe("composition involving @override directive", () => {
562562
expect(result.errors).toBeDefined();
563563
expect(errors(result)).toStrictEqual([
564564
[
565-
'FIELD_TYPE_MISMATCH',
566-
'Type of field "T.a" is incompatible across subgraphs: it has type "Int" in subgraph "Subgraph1" but type "String" in subgraph "Subgraph2"'
567-
]
565+
"FIELD_TYPE_MISMATCH",
566+
'Type of field "T.a" is incompatible across subgraphs: it has type "Int" in subgraph "Subgraph1" but type "String" in subgraph "Subgraph2"',
567+
],
568568
]);
569569
});
570570

@@ -810,7 +810,7 @@ describe("composition involving @override directive", () => {
810810
// At the moment, we've punted on @override support when interacting with @interfaceObject, so the
811811
// following tests mainly cover the various possible use and show that it currently correcly raise
812812
// some validation errors. We may lift some of those limitation in the future.
813-
describe('@interfaceObject', () => {
813+
describe("@interfaceObject", () => {
814814
it("does not allow @override on @interfaceObject fields", () => {
815815
// We currently rejects @override on fields of an @interfaceObject type. We could lift
816816
// that limitation in the future, and that would mean such override overrides the field
@@ -914,4 +914,179 @@ describe("composition involving @override directive", () => {
914914
]);
915915
});
916916
});
917+
918+
describe("progressive override", () => {
919+
it("captures override labels in supergraph", () => {
920+
const subgraph1 = {
921+
name: "Subgraph1",
922+
url: "https://Subgraph1",
923+
typeDefs: gql`
924+
type Query {
925+
t: T
926+
}
927+
928+
type T @key(fields: "k") {
929+
k: ID
930+
a: Int @override(from: "Subgraph2", label: "foo")
931+
}
932+
`,
933+
};
934+
935+
const subgraph2 = {
936+
name: "Subgraph2",
937+
url: "https://Subgraph2",
938+
typeDefs: gql`
939+
type T @key(fields: "k") {
940+
k: ID
941+
a: Int
942+
b: Int
943+
}
944+
`,
945+
};
946+
947+
const result = composeAsFed2Subgraphs([subgraph1, subgraph2]);
948+
assertCompositionSuccess(result);
949+
950+
const typeT = result.schema.type("T");
951+
expect(printType(typeT!)).toMatchInlineSnapshot(`
952+
"type T
953+
@join__type(graph: SUBGRAPH1, key: \\"k\\")
954+
@join__type(graph: SUBGRAPH2, key: \\"k\\")
955+
{
956+
k: ID
957+
a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\", overrideLabel: \\"foo\\")
958+
b: Int @join__field(graph: SUBGRAPH2)
959+
}"
960+
`);
961+
962+
const [_, api] = schemas(result);
963+
expect(printSchema(api)).toMatchString(`
964+
type Query {
965+
t: T
966+
}
967+
968+
type T {
969+
k: ID
970+
a: Int
971+
b: Int
972+
}
973+
`);
974+
});
975+
976+
describe("label validation", () => {
977+
const overridden = {
978+
name: "overridden",
979+
url: "https://overridden",
980+
typeDefs: gql`
981+
type T @key(fields: "k") {
982+
k: ID
983+
a: Int
984+
}
985+
`,
986+
};
987+
988+
it.each(["abc123", "Z_1-2:3/4.5"])(
989+
"allows valid labels starting with alpha and including alphanumerics + `_-:./`",
990+
(value) => {
991+
const withValidLabel = {
992+
name: "validLabel",
993+
url: "https://validLabel",
994+
typeDefs: gql`
995+
type Query {
996+
t: T
997+
}
998+
999+
type T @key(fields: "k") {
1000+
k: ID
1001+
a: Int
1002+
@override(from: "overridden", label: "${value}")
1003+
}
1004+
`,
1005+
};
1006+
1007+
const result = composeAsFed2Subgraphs([withValidLabel, overridden]);
1008+
assertCompositionSuccess(result);
1009+
}
1010+
);
1011+
1012+
it.each(["1_starts-with-non-alpha", "includes!@_invalid_chars"])(
1013+
"disallows invalid labels",
1014+
(value) => {
1015+
const withInvalidLabel = {
1016+
name: "invalidLabel",
1017+
url: "https://invalidLabel",
1018+
typeDefs: gql`
1019+
type Query {
1020+
t: T
1021+
}
1022+
1023+
type T @key(fields: "k") {
1024+
k: ID
1025+
a: Int @override(from: "overridden", label: "${value}")
1026+
}
1027+
`,
1028+
};
1029+
1030+
const result = composeAsFed2Subgraphs([withInvalidLabel, overridden]);
1031+
expect(result.errors).toBeDefined();
1032+
expect(errors(result)).toContainEqual([
1033+
"OVERRIDE_LABEL_INVALID",
1034+
`Invalid @override label "${value}" on field "T.a" on subgraph "invalidLabel": labels must start with a letter and after that may contain alphanumerics, underscores, minuses, colons, periods, or slashes. Alternatively, labels may be of the form "percent(x)" where x is a float between 0-100 inclusive.`,
1035+
]);
1036+
}
1037+
);
1038+
1039+
it.each(["0.5", "1", "1.0", "99.9"])(
1040+
"allows valid percent-based labels",
1041+
(value) => {
1042+
const withPercentLabel = {
1043+
name: "percentLabel",
1044+
url: "https://percentLabel",
1045+
typeDefs: gql`
1046+
type Query {
1047+
t: T
1048+
}
1049+
1050+
type T @key(fields: "k") {
1051+
k: ID
1052+
a: Int @override(from: "overridden", label: "percent(${value})")
1053+
}
1054+
`,
1055+
};
1056+
1057+
const result = composeAsFed2Subgraphs([withPercentLabel, overridden]);
1058+
assertCompositionSuccess(result);
1059+
}
1060+
);
1061+
1062+
it.each([".1", "101", "1.1234567879"])(
1063+
"disallows invalid percent-based labels",
1064+
(value) => {
1065+
const withInvalidPercentLabel = {
1066+
name: "invalidPercentLabel",
1067+
url: "https://invalidPercentLabel",
1068+
typeDefs: gql`
1069+
type Query {
1070+
t: T
1071+
}
1072+
1073+
type T @key(fields: "k") {
1074+
k: ID
1075+
a: Int @override(from: "overridden", label: "percent(${value})")
1076+
}
1077+
`,
1078+
};
1079+
1080+
const result = composeAsFed2Subgraphs([
1081+
withInvalidPercentLabel,
1082+
overridden,
1083+
]);
1084+
expect(errors(result)).toContainEqual([
1085+
"OVERRIDE_LABEL_INVALID",
1086+
`Invalid @override label "percent(${value})" on field "T.a" on subgraph "invalidPercentLabel": labels must start with a letter and after that may contain alphanumerics, underscores, minuses, colons, periods, or slashes. Alternatively, labels may be of the form "percent(x)" where x is a float between 0-100 inclusive.`,
1087+
]);
1088+
}
1089+
);
1090+
});
1091+
});
9171092
});

‎composition-js/src/merging/merge.ts

+56-6
Original file line numberDiff line numberDiff line change
@@ -95,14 +95,21 @@ type FieldMergeContextProperties = {
9595
usedOverridden: boolean,
9696
unusedOverridden: boolean,
9797
overrideWithUnknownTarget: boolean,
98+
overrideLabel: string | undefined,
9899
}
99100

100101
// for each source, specify additional properties that validate functions can set
101102
class FieldMergeContext {
102103
_props: FieldMergeContextProperties[];
103104

104105
constructor(sources: unknown[]) {
105-
this._props = (new Array(sources.length)).fill(true).map(_ => ({ usedOverridden: false, unusedOverridden: false, overrideWithUnknownTarget: false}));
106+
this._props = (
107+
new Array(sources.length)).fill(true).map(_ => ({
108+
usedOverridden: false,
109+
unusedOverridden: false,
110+
overrideWithUnknownTarget: false,
111+
overrideLabel: undefined,
112+
}));
106113
}
107114

108115
isUsedOverridden(idx: number) {
@@ -117,6 +124,10 @@ class FieldMergeContext {
117124
return this._props[idx].overrideWithUnknownTarget;
118125
}
119126

127+
overrideLabel(idx: number) {
128+
return this._props[idx].overrideLabel;
129+
}
130+
120131
setUsedOverridden(idx: number) {
121132
this._props[idx].usedOverridden = true;
122133
}
@@ -129,6 +140,10 @@ class FieldMergeContext {
129140
this._props[idx].overrideWithUnknownTarget = true;
130141
}
131142

143+
setOverrideLabel(idx: number, label: string) {
144+
this._props[idx].overrideLabel = label;
145+
}
146+
132147
some(predicate: (props: FieldMergeContextProperties) => boolean): boolean {
133148
return this._props.some(predicate);
134149
}
@@ -261,6 +276,11 @@ type EnumTypeUsage = {
261276
},
262277
}
263278

279+
interface OverrideArgs {
280+
from: string;
281+
label?: string;
282+
}
283+
264284
class Merger {
265285
readonly names: readonly string[];
266286
readonly subgraphsSchema: readonly Schema[];
@@ -1064,7 +1084,7 @@ class Merger {
10641084
return this.metadata(sourceIdx).isFieldShareable(field);
10651085
}
10661086

1067-
private getOverrideDirective(sourceIdx: number, field: FieldDefinition<any>): Directive<any> | undefined {
1087+
private getOverrideDirective(sourceIdx: number, field: FieldDefinition<any>): Directive<any, OverrideArgs> | undefined {
10681088
// Check the directive on the field, then on the enclosing type.
10691089
const metadata = this.metadata(sourceIdx);
10701090
const overrideDirective = metadata.isFed2Schema() ? metadata.overrideDirective() : undefined;
@@ -1119,7 +1139,7 @@ class Merger {
11191139
isInterfaceField?: boolean,
11201140
isInterfaceObject?: boolean,
11211141
interfaceObjectAbstractingFields?: FieldDefinition<any>[],
1122-
overrideDirective?: Directive<FieldDefinition<any>>,
1142+
overrideDirective?: Directive<FieldDefinition<any>, OverrideArgs>,
11231143
};
11241144

11251145
type ReduceResultType = {
@@ -1164,7 +1184,9 @@ class Merger {
11641184
// for each subgraph that has an @override directive, check to see if any errors or hints should be surfaced
11651185
subgraphsWithOverride.forEach((subgraphName) => {
11661186
const { overrideDirective, idx, isInterfaceObject, isInterfaceField } = subgraphMap[subgraphName];
1167-
const overridingSubgraphASTNode = overrideDirective?.sourceAST ? addSubgraphToASTNode(overrideDirective.sourceAST, subgraphName) : undefined;
1187+
if (!overrideDirective) return;
1188+
1189+
const overridingSubgraphASTNode = overrideDirective.sourceAST ? addSubgraphToASTNode(overrideDirective.sourceAST, subgraphName) : undefined;
11681190
if (isInterfaceField) {
11691191
this.errors.push(ERRORS.OVERRIDE_ON_INTERFACE.err(
11701192
`@override cannot be used on field "${dest.coordinate}" on subgraph "${subgraphName}": @override is not supported on interface type fields.`,
@@ -1181,7 +1203,7 @@ class Merger {
11811203
return;
11821204
}
11831205

1184-
const sourceSubgraphName = overrideDirective?.arguments()?.from;
1206+
const sourceSubgraphName = overrideDirective.arguments().from;
11851207
if (!this.names.includes(sourceSubgraphName)) {
11861208
result.setOverrideWithUnknownTarget(idx);
11871209
const suggestions = suggestionList(sourceSubgraphName, this.names);
@@ -1195,7 +1217,7 @@ class Merger {
11951217
} else if (sourceSubgraphName === subgraphName) {
11961218
this.errors.push(ERRORS.OVERRIDE_FROM_SELF_ERROR.err(
11971219
`Source and destination subgraphs "${sourceSubgraphName}" are the same for overridden field "${dest.coordinate}"`,
1198-
{ nodes: overrideDirective?.sourceAST },
1220+
{ nodes: overrideDirective.sourceAST },
11991221
));
12001222
} else if (subgraphsWithOverride.includes(sourceSubgraphName)) {
12011223
this.errors.push(ERRORS.OVERRIDE_SOURCE_HAS_OVERRIDE.err(
@@ -1269,6 +1291,33 @@ class Merger {
12691291
overriddenSubgraphASTNode,
12701292
));
12711293
}
1294+
1295+
// capture an override label if it exists
1296+
const overrideLabel = overrideDirective.arguments().label;
1297+
if (overrideLabel) {
1298+
const labelRegex = /^[a-zA-Z][a-zA-Z0-9_\-:./]*$/;
1299+
// Enforce that the label matches the following pattern: percent(x)
1300+
// where x is a float between 0 and 100 with no more than 8 decimal places
1301+
const percentRegex = /^percent\((\d{1,2}(\.\d{1,8})?|100)\)$/;
1302+
if (labelRegex.test(overrideLabel)) {
1303+
result.setOverrideLabel(idx, overrideLabel);
1304+
} else if (percentRegex.test(overrideLabel)) {
1305+
const parts = percentRegex.exec(overrideLabel);
1306+
if (parts) {
1307+
const percent = parseFloat(parts[1]);
1308+
if (percent >= 0 && percent <= 100) {
1309+
result.setOverrideLabel(idx, overrideLabel);
1310+
}
1311+
}
1312+
}
1313+
1314+
if (!result.overrideLabel(idx)) {
1315+
this.errors.push(ERRORS.OVERRIDE_LABEL_INVALID.err(
1316+
`Invalid @override label "${overrideLabel}" on field "${dest.coordinate}" on subgraph "${subgraphName}": labels must start with a letter and after that may contain alphanumerics, underscores, minuses, colons, periods, or slashes. Alternatively, labels may be of the form "percent(x)" where x is a float between 0-100 inclusive.`,
1317+
{ nodes: overridingSubgraphASTNode }
1318+
));
1319+
}
1320+
}
12721321
}
12731322
}
12741323
});
@@ -1608,6 +1657,7 @@ class Merger {
16081657
type: allTypesEqual ? undefined : source.type?.toString(),
16091658
external: external ? true : undefined,
16101659
usedOverridden: usedOverridden ? true : undefined,
1660+
overrideLabel: mergeContext.overrideLabel(idx),
16111661
});
16121662
}
16131663
}

‎docs/source/errors.md

+1
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ The following errors might be raised during composition:
6565
| `ONLY_INACCESSIBLE_CHILDREN` | A type visible in the API schema has only @inaccessible children. | 2.0.0 | |
6666
| `OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE` | The @override directive cannot be used on external fields, nor to override fields with either @external, @provides, or @requires. | 2.0.0 | |
6767
| `OVERRIDE_FROM_SELF_ERROR` | Field with `@override` directive has "from" location that references its own subgraph. | 2.0.0 | |
68+
| `OVERRIDE_LABEL_INVALID` | The @override directive `label` argument must match the pattern /^[a-zA-Z][a-zA-Z0-9_-:./]*$/ or /^percent((d{1,2}(.d{1,8})?|100))$/ | 2.7.0 | |
6869
| `OVERRIDE_ON_INTERFACE` | The @override directive cannot be used on the fields of an interface type. | 2.3.0 | |
6970
| `OVERRIDE_SOURCE_HAS_OVERRIDE` | Field which is overridden to another subgraph is also marked @override. | 2.0.0 | |
7071
| `PROVIDES_DIRECTIVE_IN_FIELDS_ARG` | The `fields` argument of a `@provides` directive includes some directive applications. This is not supported | 2.1.0 | |

0 commit comments

Comments
 (0)