Skip to content

Commit

Permalink
- only put external on overridden field if it is used (i.e. reference…
Browse files Browse the repository at this point in the history 
…d by @provides, @requires) or referenced by an @key

- if overridden directive does not have external, we want to filter it from consideration rather than allowing it to be used in the join
  • Loading branch information
@clenfest
clenfest committed Mar 28, 2022
1 parent 03ee140 commit daee9ee
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 42 deletions.
4 changes: 2 additions & 2 deletions composition-js/src/__tests__/override.compose.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ describe("composition involving @override directive", () => {
@join__type(graph: SUBGRAPH2, key: \\"k\\")
{
k: ID
a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, external: true)
a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\")
b: Int @join__field(graph: SUBGRAPH2)
}"
`);
Expand Down Expand Up @@ -314,7 +314,7 @@ describe("composition involving @override directive", () => {
@join__type(graph: SUBGRAPH2, key: \\"k\\")
{
k: ID
a: Int @join__field(graph: SUBGRAPH1)
a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\")
}"
`);

Expand Down
17 changes: 12 additions & 5 deletions composition-js/src/merging/merge.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -710,10 +710,10 @@ class Merger {
this.hintOnInconsistentValueTypeField(sources, dest, destField);
}
const subgraphFields = sources.map(t => t?.field(destField.name));
this.validateOverride(subgraphFields, destField);
const filteredFields = this.validateOverride(subgraphFields, destField);

this.mergeField(subgraphFields, destField);
this.validateFieldSharing(subgraphFields, destField);
this.mergeField(filteredFields, destField);
this.validateFieldSharing(filteredFields, destField);
}
}
}
Expand Down Expand Up @@ -918,6 +918,7 @@ class Merger {
return acc;
}, { subgraphsWithOverride: [], subgraphMap: {} });

const overriddenSubgraphs: number[] = [];
// for each subgraph that has an @override directive, check to see if any errors or hints should be surfaced
subgraphsWithOverride.forEach((subgraphName) => {
const { overrideDirective } = subgraphMap[subgraphName];
Expand Down Expand Up @@ -965,9 +966,13 @@ class Merger {
}));
} else {
// if we get here, then the @override directive is valid
// if the field being overridden is a key, then we need to add an @external directive
// if the field being overridden is used, then we need to add an @external directive
assert(fromField, 'fromField should not be undefined');
fromField.applyDirective(this.metadata(fromIdx).externalDirective());
if (this.metadata(fromIdx).isFieldUsed(fromField) || this.metadata(fromIdx).isKeyField(fromField)) {
fromField.applyDirective(this.metadata(fromIdx).externalDirective());
} else {
overriddenSubgraphs.push(fromIdx);
}
this.hints.push(new CompositionHint(
hintOverriddenFieldCanBeRemoved,
`Field "${coordinate}" on subgraph "${sourceSubgraphName}" is overridden. Consider removing it.`,
Expand All @@ -976,6 +981,8 @@ class Merger {
}
}
});

return sources.map((source, idx) => overriddenSubgraphs.includes(idx) ? undefined : source);
}

private mergeField(sources: FieldOrUndefinedArray, dest: FieldDefinition<any>) {
Expand Down
2 changes: 1 addition & 1 deletion gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,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).toEqual(
'4718616f8164d2ffeb33c08278051dc144718392a9c9e5470b1922a0cb4bb22a',
'7544614133e6f2b5f995badc562c803785b1ccd5e0a52be47a59d8cc95df0285',
);
// second call should have previous info in the second arg
expect(secondCall[1]!.compositionId).toEqual(expectedFirstId);
Expand Down
7 changes: 0 additions & 7 deletions internals-js/src/extractSubgraphsFromSupergraph.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,6 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs {
if (args.external) {
subgraphField.applyDirective(subgraph.metadata().externalDirective());
}
if (args.override) {
subgraphField.applyDirective(subgraph.metadata().overrideDirective(), {'from': args.override});
const overriddenSubgraph = subgraphs.get(args.override);
if (overriddenSubgraph) {
overriddenSubgraph.metadata().addSyntheticExternal(subgraphField.coordinate);
}
}
}
}
}
Expand Down
44 changes: 24 additions & 20 deletions internals-js/src/federation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,21 +258,21 @@ function validateAllFieldSet<TParent extends SchemaElement<any, any>>(
}
}

export function collectUsedExternalFieldsCoordinates(metadata: FederationMetadata): Set<string> {
const usedExternalCoordinates = new Set<string>();
export function collectUsedFields(metadata: FederationMetadata): Set<FieldDefinition<CompositeType>> {
const usedFields = new Set<FieldDefinition<CompositeType>>();

// Collects all external fields used by a key, requires or provides
collectUsedExternaFieldsForDirective<CompositeType>(
metadata,
metadata.keyDirective(),
type => type,
usedExternalCoordinates,
usedFields,
);
collectUsedExternaFieldsForDirective<FieldDefinition<CompositeType>>(
metadata,
metadata.requiresDirective(),
field => field.parent!,
usedExternalCoordinates,
usedFields,
);
collectUsedExternaFieldsForDirective<FieldDefinition<CompositeType>>(
metadata,
Expand All @@ -281,7 +281,7 @@ export function collectUsedExternalFieldsCoordinates(metadata: FederationMetadat
const type = baseType(field.type!);
return isCompositeType(type) ? type : undefined;
},
usedExternalCoordinates,
usedFields,
);

// Collects all external fields used to satisfy an interface constraint
Expand All @@ -291,20 +291,20 @@ export function collectUsedExternalFieldsCoordinates(metadata: FederationMetadat
for (const runtimeType of runtimeTypes) {
const implemField = runtimeType.field(field.name);
if (implemField && metadata.isFieldExternal(implemField)) {
usedExternalCoordinates.add(implemField.coordinate);
usedFields.add(implemField);
}
}
}
}

return usedExternalCoordinates;
return usedFields;
}

function collectUsedExternaFieldsForDirective<TParent extends SchemaElement<any, any>>(
metadata: FederationMetadata,
definition: DirectiveDefinition<{fields: any}>,
targetTypeExtractor: (element: TParent) => CompositeType | undefined,
usedExternalCoordinates: Set<string>
usedExternalCoordinates: Set<FieldDefinition<CompositeType>>
) {
for (const application of definition.applications()) {
const type = targetTypeExtractor(application.parent! as TParent);
Expand All @@ -323,7 +323,7 @@ function collectUsedExternaFieldsForDirective<TParent extends SchemaElement<any,
includeInterfaceFieldsImplementations: true,
validate: false,
}).filter((field) => metadata.isFieldExternal(field))
.forEach((field) => usedExternalCoordinates.add(field.coordinate));
.forEach((field) => usedExternalCoordinates.add(field));
}
}

Expand All @@ -332,13 +332,12 @@ function collectUsedExternaFieldsForDirective<TParent extends SchemaElement<any,
* interface implementation. Otherwise, the field declaration is somewhat useless.
*/
function validateAllExternalFieldsUsed(metadata: FederationMetadata, errorCollector: GraphQLError[]): void {
const allUsedExternals = collectUsedExternalFieldsCoordinates(metadata);
for (const type of metadata.schema.types()) {
if (!isObjectType(type) && !isInterfaceType(type)) {
continue;
}
for (const field of type.fields()) {
if (!metadata.isFieldExternal(field) || allUsedExternals.has(field.coordinate) || metadata.isSyntheticExternal(field)) {
if (!metadata.isFieldExternal(field) || metadata.isFieldUsed(field)) {
continue;
}

Expand Down Expand Up @@ -432,7 +431,7 @@ export class FederationMetadata {
private _externalTester?: ExternalTester;
private _sharingPredicate?: (field: FieldDefinition<CompositeType>) => boolean;
private _keysPredicate?: (field: FieldDefinition<CompositeType>) => boolean;
private _syntheticExternals: Set<string> = new Set();
private _fieldUsedPredicate?: (field: FieldDefinition<CompositeType>) => boolean;
private _isFed2Schema?: boolean;

constructor(readonly schema: Schema) {
Expand All @@ -443,6 +442,7 @@ export class FederationMetadata {
this._sharingPredicate = undefined;
this._keysPredicate = undefined;
this._isFed2Schema = undefined;
this._fieldUsedPredicate = undefined;
}

isFed2Schema(): boolean {
Expand Down Expand Up @@ -473,6 +473,18 @@ export class FederationMetadata {
return this._keysPredicate;
}

private fieldUsedPredicate(): (field: FieldDefinition<CompositeType>) => boolean {
if (!this._fieldUsedPredicate) {
const usedFields = Array.from(collectUsedFields(this));
this._fieldUsedPredicate = (field: FieldDefinition<CompositeType>) => !!usedFields.find((f) => f.coordinate === field.coordinate);
}
return this._fieldUsedPredicate;
}

isFieldUsed(field: FieldDefinition<CompositeType>): boolean {
return this.fieldUsedPredicate()(field);
}

isFieldExternal(field: FieldDefinition<any> | InputFieldDefinition) {
return this.externalTester().isExternal(field);
}
Expand Down Expand Up @@ -501,14 +513,6 @@ export class FederationMetadata {
return this.keysPredicate()(field);
}

addSyntheticExternal(coordinate: string) {
this._syntheticExternals.add(coordinate);
}

isSyntheticExternal(field: FieldDefinition<any>): boolean {
return this._syntheticExternals.has(field.coordinate);
}

federationDirectiveNameInSchema(name: string): string {
if (this.isFed2Schema()) {
const coreFeatures = this.schema.coreFeatures;
Expand Down
11 changes: 4 additions & 7 deletions internals-js/src/schemaUpgrader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
import {
addSubgraphToError,
collectTargetFields,
collectUsedExternalFieldsCoordinates,
federationMetadata,
FederationMetadata,
printSubgraphNames,
Expand All @@ -43,7 +42,7 @@ type UpgradeChanges = MultiMap<UpgradeChangeID, UpgradeChange>;
export type UpgradeSuccess = {
subgraphs: Subgraphs,
changes: Map<string, UpgradeChanges>,
errors?: never,
errors?: never,
}

export type UpgradeFailure = {
Expand Down Expand Up @@ -235,7 +234,7 @@ export function upgradeSubgraphsIfNecessary(inputs: Subgraphs): UpgradeResult {
* 2. do not have a definition for the same type in the same subgraph (this is a GraphQL extension otherwise).
*
* Not that type extensions in federation 1 generally have a @key but in really the code consider something a type extension even without
* it (which I'd argue is a unintended bug of fed1 since this leads to various problems) so we don't check for the presence of @key here.
* it (which I'd argue is a unintended bug of fed1 since this leads to various problems) so we don't check for the presence of @key here.
*/
function isFederationTypeExtension(type: NamedType): boolean {
const metadata = federationMetadata(type.schema());
Expand Down Expand Up @@ -522,7 +521,7 @@ class SchemaUpgrader {
continue;
}
assert(isCompositeType(typeInOther), () => `Type ${type} is of kind ${type.kind} in ${this.subgraph.name} but ${typeInOther.kind} in ${other.name}`);
const keysInOther = typeInOther.appliedDirectivesOf(other.metadata().keyDirective());
const keysInOther = typeInOther.appliedDirectivesOf(other.metadata().keyDirective());
if (keysInOther.length === 0) {
continue;
}
Expand Down Expand Up @@ -564,14 +563,12 @@ class SchemaUpgrader {
}

private removeUnusedExternals() {
const usedExternalFieldsCoordinates = collectUsedExternalFieldsCoordinates(this.metadata);

for (const type of this.schema.types()) {
if (!isObjectType(type) && !isInterfaceType(type)) {
continue;
}
for (const field of type.fields()) {
if (this.metadata.isFieldExternal(field) && !usedExternalFieldsCoordinates.has(field.coordinate)) {
if (this.metadata.isFieldExternal(field) && !this.metadata.isFieldUsed(field)) {
this.addChange(new UnusedExternalRemoval(field.coordinate));
field.remove();
}
Expand Down

0 comments on commit daee9ee

Please sign in to comment.