Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to reference elements further up the call chain as named parameters #2988

Merged
merged 85 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
85 commits
Select commit Hold shift + click to select a range
884befc
Some work on setContext. Mostly defining the directives and changes t…
clenfest Jan 31, 2024
4561844
First pass at context validation. This will become obsolete since we …
clenfest Mar 12, 2024
b85d78a
Finished validation of contexts and making sure extractSubgraphsFromS…
clenfest Mar 25, 2024
346fe3d
Merge branch 'next' into clenfest/setContext
clenfest Mar 25, 2024
d5117f3
Bump join spec and federation spec
clenfest Mar 26, 2024
672b705
checkpoint
clenfest Apr 12, 2024
1f495a6
fix a couple of tests
clenfest Apr 22, 2024
4ed6fdc
Change from uuid for generation to something that is more gql friendl…
clenfest Apr 24, 2024
dffd92d
Add inputRewrites for contextual values
clenfest Apr 25, 2024
b2bf2b6
Make context variable definition deterministic rather than random
clenfest Apr 26, 2024
1bc21d1
fixed up tests with a todo
clenfest Apr 26, 2024
ed116b1
Fix up spelling and formatting
clenfest Apr 26, 2024
e6503b5
Treat argument to contextual fields as a variable rather than a string
clenfest Apr 29, 2024
7af5952
typo
clenfest Apr 29, 2024
86b9120
test
clenfest Apr 29, 2024
0ba353b
testing in codesandbox, ignore this commit
clenfest Apr 29, 2024
b8e9085
trying to use a key renamer instead of a value setter
clenfest Apr 29, 2024
411875a
Realized that although I do want behavior very much like a inputRewri…
clenfest Apr 29, 2024
84c53ef
Use generated context id rather than paramter name for context_rewrites
clenfest Apr 29, 2024
ba7b85d
push an empty commit because package query-graphs wasnt published in …
o0Ignition0o Apr 30, 2024
cff5f8d
Use argument type in validation rather than hardcoded String
clenfest Apr 30, 2024
e692a81
Add typename to query plan
clenfest Apr 30, 2024
509674d
fix bug in copyInputsOf
clenfest Apr 30, 2024
5aef7f3
Merge branch 'main' into clenfest/setContext-qp
o0Ignition0o May 5, 2024
e30ff81
address comment
clenfest May 6, 2024
3d35225
more changes
clenfest May 6, 2024
d370f20
updating regex
clenfest May 6, 2024
52c043f
get rid of canFulfillType to make more standard
clenfest May 6, 2024
6514d0b
update implementation
clenfest May 6, 2024
9b45fe8
Add tests for rejecting selection sets with aliases or directives in …
clenfest May 6, 2024
881ae1a
Add test for ensuring that no @interfaceObject type may be referenced…
clenfest May 6, 2024
2890817
multiple paths
clenfest May 7, 2024
9477fac
fix prettier and spelling
clenfest May 7, 2024
00fedbf
Add ContextFieldValue scalar to subgraph schema.
clenfest May 7, 2024
647a5a4
delete dead code
clenfest May 7, 2024
f355e8b
change logic of valid non-contextual arguments
clenfest May 7, 2024
124760a
add more validation tests
clenfest May 7, 2024
96b428e
extractSubgraphFromSupergraph code review comments
clenfest May 8, 2024
34b2fff
code review updates
clenfest May 9, 2024
dcef412
fixing up union types
clenfest May 9, 2024
9298a56
Add a union test into buildPlan. Also fixed up the logic that tests f…
clenfest May 10, 2024
87abc6f
prettier
clenfest May 10, 2024
9bdfd1f
relative path should ignore FragmentElements
clenfest May 10, 2024
6976448
get rid of hack
clenfest May 10, 2024
7271c8b
checkpoint
clenfest May 10, 2024
ea94a7e
add optional validation
clenfest May 10, 2024
ec73f62
union types broken
clenfest May 10, 2024
1f82cf3
Deal with @requires and @fromContext in the same conditions
clenfest May 10, 2024
3b8f641
fix FragmentSelection case (union)
clenfest May 11, 2024
a6c9a03
delete dead code
clenfest May 12, 2024
7d561f6
code review updates
clenfest May 12, 2024
b210b3b
updates
clenfest May 12, 2024
903436b
subgraph schema in toPlanNode()
clenfest May 12, 2024
c21a63a
Add contextsToConditionsGroups as a dedicated variable in the compute…
clenfest May 13, 2024
5ebddec
fix subgraph name transform
clenfest May 13, 2024
3ba1b5d
Add test for and fixes the case where the fetchGroup where data is us…
clenfest May 13, 2024
0a592ff
Add hint when removing an argument from the supergraph
clenfest May 13, 2024
eee34b5
remove some dead code
clenfest May 13, 2024
b000806
move contextToSelection to Child from OpPathTree
clenfest May 13, 2024
3c44f7a
Move parameterToContext onto child
clenfest May 13, 2024
d90b818
fix spelling
clenfest May 13, 2024
fcff867
update snapshots
clenfest May 13, 2024
6eadd46
Update loop detection in satisfiability checks
sachindshinde May 13, 2024
fa91a78
Update advancePathWithDirectTransition() error messaging to account f…
sachindshinde May 13, 2024
dcd870b
Use camelCase instead of snake_case
sachindshinde May 13, 2024
5cd6477
Update validate() for arguments to check @fromContext on subgraphs in…
sachindshinde May 13, 2024
8f0544e
Fix argument types in context spec
sachindshinde May 13, 2024
4830903
Update names, and remove dead code from specs files
sachindshinde May 13, 2024
f7a35c0
Remove context spec from gateway supported features, and add list of …
sachindshinde May 13, 2024
cc55f8a
Add function Supergraph.buildForTests() that will build using router …
clenfest May 13, 2024
c14a873
Update subgraph validation to account for object type conditions
sachindshinde May 13, 2024
cc6d499
Translate subgraph names to GraphQL enum values using existing mapping
sachindshinde May 13, 2024
3ccd70a
Update QueryGraph to have schema, and update option generation to par…
sachindshinde May 14, 2024
ac64973
Fix bug in parameter to context comparison
sachindshinde May 14, 2024
8acad3e
Updated computeGroupsForTree() for context handling
sachindshinde May 14, 2024
92bb1e8
update tests to use ofType and remove nonNullable from contextual arg…
clenfest May 14, 2024
1c0ab3d
Remove unused field in query plan
sachindshinde May 14, 2024
0f09e7e
Fix bug where context additions happened at wrong location in compute…
sachindshinde May 14, 2024
2aa8f76
Update composition validation to handle bad subgraph names
sachindshinde May 14, 2024
c420486
fixing up broken tests
clenfest May 14, 2024
f0f0ce7
fix spelling
clenfest May 14, 2024
cf64c4a
add changeset
clenfest May 14, 2024
7ff75ab
Merge branch 'next' into clenfest/setContext-qp
clenfest May 14, 2024
d6f99b5
spelling
clenfest May 14, 2024
dd72c8e
prettier fix
clenfest May 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
928 changes: 928 additions & 0 deletions composition-js/src/__tests__/compose.setContext.test.ts

Large diffs are not rendered by default.

26 changes: 22 additions & 4 deletions composition-js/src/__tests__/compose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('composition', () => {
expect(result.supergraphSdl).toMatchString(`
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/join/v0.4", for: EXECUTION)
@link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION)
{
query: Query
}
Expand All @@ -79,7 +79,7 @@ describe('composition', () => {

directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE

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
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__graph(name: String!, url: String!) on ENUM_VALUE

Expand All @@ -98,10 +98,19 @@ describe('composition', () => {
V2 @join__enumValue(graph: SUBGRAPH2)
}

input join__ContextArgument {
name: String!
type: String!
context: String!
selection: join__FieldValue!
}

scalar join__DirectiveArguments

scalar join__FieldSet

scalar join__FieldValue

enum join__Graph {
SUBGRAPH1 @join__graph(name: "Subgraph1", url: "https://Subgraph1")
SUBGRAPH2 @join__graph(name: "Subgraph2", url: "https://Subgraph2")
Expand Down Expand Up @@ -222,7 +231,7 @@ describe('composition', () => {
expect(result.supergraphSdl).toMatchString(`
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/join/v0.4", for: EXECUTION)
@link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION)
{
query: Query
}
Expand All @@ -231,7 +240,7 @@ describe('composition', () => {

directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE

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
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__graph(name: String!, url: String!) on ENUM_VALUE

Expand All @@ -250,10 +259,19 @@ describe('composition', () => {
V2 @join__enumValue(graph: SUBGRAPH2)
}

input join__ContextArgument {
context: String!
name: String!
selection: join__FieldValue!
type: String!
}

scalar join__DirectiveArguments

scalar join__FieldSet

scalar join__FieldValue

enum join__Graph {
SUBGRAPH1 @join__graph(name: "Subgraph1", url: "https://Subgraph1")
SUBGRAPH2 @join__graph(name: "Subgraph2", url: "https://Subgraph2")
Expand Down
13 changes: 11 additions & 2 deletions composition-js/src/__tests__/override.compose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,7 @@ describe("composition involving @override directive", () => {
expect(result.supergraphSdl).toMatchInlineSnapshot(`
"schema
@link(url: \\"https://specs.apollo.dev/link/v1.0\\")
@link(url: \\"https://specs.apollo.dev/join/v0.4\\", for: EXECUTION)
@link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
{
query: Query
}
Expand All @@ -983,7 +983,7 @@ describe("composition involving @override directive", () => {

directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE

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
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__graph(name: String!, url: String!) on ENUM_VALUE

Expand All @@ -995,10 +995,19 @@ describe("composition involving @override directive", () => {

directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA

input join__ContextArgument {
name: String!
type: String!
context: String!
selection: join__FieldValue!
}

scalar join__DirectiveArguments

scalar join__FieldSet

scalar join__FieldValue

enum join__Graph {
SUBGRAPH1 @join__graph(name: \\"Subgraph1\\", url: \\"https://Subgraph1\\")
SUBGRAPH2 @join__graph(name: \\"Subgraph2\\", url: \\"https://Subgraph2\\")
Expand Down
1 change: 1 addition & 0 deletions composition-js/src/composeDirectiveManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ export class ComposeDirectiveManager {
sg.metadata().authenticatedDirective(),
sg.metadata().requiresScopesDirective(),
sg.metadata().policyDirective(),
sg.metadata().contextDirective(),
clenfest marked this conversation as resolved.
Show resolved Hide resolved
].map(d => d.name);
if (directivesComposedByDefault.includes(directive.name)) {
this.pushHint(new CompositionHint(
Expand Down
108 changes: 105 additions & 3 deletions composition-js/src/merging/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,12 @@ import {
LinkDirectiveArgs,
sourceIdentity,
FeatureUrl,
isFederationDirectiveDefinedInSchema,
parseSelectionSet,
parseContext,
CoreFeature,
Subgraph,
StaticArgumentsTransform,
} from "@apollo/federation-internals";
import { ASTNode, GraphQLError, DirectiveLocation } from "graphql";
import {
Expand Down Expand Up @@ -291,7 +295,7 @@ class Merger {
readonly merged: Schema = new Schema();
readonly subgraphNamesToJoinSpecName: Map<string, string>;
readonly mergedFederationDirectiveNames = new Set<string>();
readonly mergedFederationDirectiveInSupergraph = new Map<string, { definition: DirectiveDefinition, argumentsMerger?: ArgumentMerger }>();
readonly mergedFederationDirectiveInSupergraph = new Map<string, { definition: DirectiveDefinition, argumentsMerger?: ArgumentMerger, staticArgumentTransform?: StaticArgumentsTransform }>();
readonly enumUsages = new Map<string, EnumTypeUsage>();
private composeDirectiveManager: ComposeDirectiveManager;
private mismatchReporter: MismatchReporter;
Expand All @@ -306,6 +310,7 @@ class Merger {
private latestFedVersionUsed: FeatureVersion;
private joinDirectiveIdentityURLs = new Set<string>();
private schemaToImportNameToFeatureUrl = new Map<Schema, Map<string, FeatureUrl>>();
private contextToTypeMap = new Map<string, { types: Set<CompositeType>, usages: { usage: string, argumentDefinition: ArgumentDefinition<FieldDefinition<ObjectType | InterfaceType>> }[] }>();
clenfest marked this conversation as resolved.
Show resolved Hide resolved

constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) {
this.latestFedVersionUsed = this.getLatestFederationVersionUsed();
Expand Down Expand Up @@ -455,6 +460,7 @@ class Merger {
this.mergedFederationDirectiveInSupergraph.set(specInSupergraph.url.name, {
definition: this.merged.directive(nameInSupergraph)!,
argumentsMerger,
staticArgumentTransform: compositionSpec.staticArgumentTransform,
});
}
}
Expand Down Expand Up @@ -872,6 +878,7 @@ class Merger {
const isInterfaceObject = sourceMetadata.isInterfaceObjectType(source) ? true : undefined;
const keys = source.appliedDirectivesOf(sourceMetadata.keyDirective());
const name = this.joinSpecName(idx);

if (!keys.length) {
dest.applyDirective(joinTypeDirective, { graph: name, isInterfaceObject });
} else {
Expand Down Expand Up @@ -1709,6 +1716,37 @@ class Merger {
continue;
}

const fromContextDirective = this.subgraphs.values()[idx].metadata().fromContextDirective();
clenfest marked this conversation as resolved.
Show resolved Hide resolved

const contextArguments = (source.kind === 'FieldDefinition' ? source.arguments() : [])
.map((arg): {
context: string,
name: string,
type: string,
selection: string,
} | undefined => {
if (!isFederationDirectiveDefinedInSchema(fromContextDirective)) {
return undefined;
}
const appliedDirectives = arg.appliedDirectivesOf(fromContextDirective);
if (appliedDirectives.length === 0) {
return undefined;
}
assert(appliedDirectives.length === 1, 'There should be at most one @fromContext directive applied to an argument');
const directive = appliedDirectives[0];
const { context, selection } = parseContext(directive.arguments().field);
// these are validated in the subgraph validation phase
assert(context, 'Context should be defined');
assert(selection, 'Selection should be defined');
return {
context: `${this.subgraphs.values()[idx].name}__${context}`,
clenfest marked this conversation as resolved.
Show resolved Hide resolved
name: arg.name,
type: arg.type!.toString(),
selection,
};
})
.filter(isDefined);

const external = this.isExternal(idx, source);
const sourceMeta = this.subgraphs.values()[idx].metadata();
const name = this.joinSpecName(idx);
Expand All @@ -1721,10 +1759,10 @@ class Merger {
external: external ? true : undefined,
usedOverridden: usedOverridden ? true : undefined,
overrideLabel: mergeContext.overrideLabel(idx),
contextArguments: contextArguments.length > 0 ? contextArguments : undefined,
});
}
}

private getFieldSet(element: SchemaElement<any, any>, directive: DirectiveDefinition<{fields: string}>): string | undefined {
const applications = element.appliedDirectivesOf(directive);
assert(applications.length <= 1, () => `Found more than one application of ${directive} on ${element}`);
Expand Down Expand Up @@ -1847,6 +1885,29 @@ class Merger {
// some path. Done because this helps reusing our "reportMismatchHint" method
// in those cases.
const arg = dest.addArgument(argName);

const isContextualArg = (index: number, arg: ArgumentDefinition<DirectiveDefinition<any>> | ArgumentDefinition<FieldDefinition<any>>) => {
const fromContextDirective = this.metadata(index).fromContextDirective();
return fromContextDirective && isFederationDirectiveDefinedInSchema(fromContextDirective) && arg.appliedDirectivesOf(fromContextDirective).length >= 1;
}
const hasContextual = sources.map((s, idx) => {
const arg = s?.argument(argName);
return arg && isContextualArg(idx, arg);
});

if (hasContextual.some((c) => c === true)) {
clenfest marked this conversation as resolved.
Show resolved Hide resolved
// If any of the sources has a contextual argument, then we need to remove it from the supergraph
// and ensure that all the sources have it.
if (hasContextual.some((c) => c === false)) {
this.errors.push(ERRORS.CONTEXTUAL_ARGUMENT_NOT_CONTEXTUAL_IN_ALL_SUBGRAPHS.err(
`Argument "${arg.coordinate}" is contextual in some subgraphs but not in all subgraphs: it is contextual in ${printSubgraphNames(hasContextual.map((c, i) => c ? this.names[i] : undefined).filter(isDefined))} but not in ${printSubgraphNames(hasContextual.map((c, i) => c ? undefined : this.names[i]).filter(isDefined))}`,
{ nodes: sourceASTs(...sources.map((s) => s?.argument(argName))) },
));
}
arg.remove();
continue;
}

clenfest marked this conversation as resolved.
Show resolved Hide resolved
// If all the sources that have the field have the argument, we do merge it
// and we're good, but otherwise ...
if (sources.some((s) => s && !s.argument(argName))) {
Expand Down Expand Up @@ -2590,11 +2651,15 @@ class Merger {
return;
}

const directiveInSupergraph = this.mergedFederationDirectiveInSupergraph.get(name);

if (dest.schema().directive(name)?.repeatable) {
// For repeatable directives, we simply include each application found but with exact duplicates removed
while (perSource.length > 0) {
const directive = this.pickNextDirective(perSource);
dest.applyDirective(directive.name, directive.arguments(false));

const transformedArgs = directiveInSupergraph && directiveInSupergraph.staticArgumentTransform && directiveInSupergraph.staticArgumentTransform(this.subgraphs.values()[0], directive.arguments(false));
dest.applyDirective(directive.name, transformedArgs ?? directive.arguments(false));
// We remove every instances of this particular application. That is we remove any other applicaiton with
// the same arguments. Note that when doing so, we include default values. This allows "merging" 2 applications
// when one rely on the default value while another don't but explicitely uses that exact default value.
Expand Down Expand Up @@ -2980,7 +3045,44 @@ class Merger {
}
}
}
this.validateContextUsages();
}

// private traverseSelectionSetForType(
// selection: string,
// type: ObjectType,
// ) {
// const selectionSet = new SelectionSet(type, )
// }

private validateContextUsages() {
clenfest marked this conversation as resolved.
Show resolved Hide resolved
// For each usage of a context, we need to validate that all set contexts could fulfill the selection of the context
this.contextToTypeMap.forEach(({ usages, types }, context) => {
for (const { usage, argumentDefinition } of usages) {
if (types.size === 0) {
this.errors.push(ERRORS.CONTEXT_NOT_SET.err(
`Context "${context}" is used in "${argumentDefinition.coordinate}" but is never set in any subgraph.`,
{ nodes: sourceASTs(argumentDefinition) }
));
}
// const resolvedTypes = [];
for (const type of types) {
// now ensure that for each type, the selection is satisfiable and collect the resolved type
try {
parseSelectionSet({ parentType: type, source: usage });
} catch (error) {
if (error instanceof GraphQLError) {
this.errors.push(ERRORS.CONTEXT_INVALID_SELECTION.err(
`Context "${context}" is used in "${argumentDefinition.coordinate}" but the selection is invalid: ${error.message}`,
{ nodes: sourceASTs(argumentDefinition) }
));
} else {
throw error;
}
}
}
}
});
}

private updateInaccessibleErrorsWithLinkToSubgraphs(
Expand Down
2 changes: 1 addition & 1 deletion gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ describe('lifecycle hooks', () => {
// the supergraph (even just formatting differences), this ID will change
// and this test will have to updated.
expect(secondCall[0]!.compositionId).toMatchInlineSnapshot(
`"208492fefbbc8f62458b3f698b047a7c119f5169d4ccaef4c24b81a1ba82f87c"`,
`"6dc1bde2b9818fabec62208c5d8825abaa1bae89635fa6f3a5ffea7b78fc6d82"`,
);
// second call should have previous info in the second arg
expect(secondCall[1]!.compositionId).toEqual(expectedFirstId);
Expand Down
Loading