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.

Sign up for GitHub

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

Adds @tag definition automatically in buildSubgraphSchema #1600

Merged
merged 3 commits into from
Mar 15, 2022
Merged

Adds @tag definition automatically in buildSubgraphSchema #1600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9872dfe
Adds @tag definition automatically in `buildSubgraphSchema`
Mar 15, 2022
35bafbe
Fix placement of tests and use
Mar 15, 2022
982ff6b
Changelog updates
Mar 15, 2022
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
2 changes: 2 additions & 0 deletions gateway-js/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.

- Automatically add the `@tag` directive definition in `buildSubgraphSchema` (but still support it if the definition is present in the input document) [PR #1600](https://github.com/apollographql/federation/pull/1600).

## v2.0.0-preview.5

- Fix propagation of `@tag` to the supergraph and allows @tag to be repeated. Additionally, merged directives (only `@tag` and `@deprecated` currently) are not allowed on external fields anymore [PR #1592](https://github.com/apollographql/federation/pull/1592).
Expand Down
2 changes: 1 addition & 1 deletion subgraph-js/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.

- _Nothing yet! Stay tuned._
- Automatically add the `@tag` directive definition in `buildSubgraphSchema` (but still support it if the definition is present in the input document) [PR #1600](https://github.com/apollographql/federation/pull/1600).

## v2.0.0-preview.5

Expand Down
42 changes: 42 additions & 0 deletions subgraph-js/src/__tests__/buildSubgraphSchema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,48 @@ extend type User @key(fields: "email") {
`);
});
});

describe('@tag directive', () => {
const query = `query GetServiceDetails {
_service {
sdl
}
}`;

const validateTag = async (header: string) => {
const schema = buildSubgraphSchema(gql`${header}
type User @key(fields: "email") @tag(name: "tagOnType") {
email: String @tag(name: "tagOnField")
}

interface Thing @tag(name: "tagOnInterface") {
name: String
}

union UserButAUnion @tag(name: "tagOnUnion") = User
`);

const { data, errors } = await graphql({ schema, source: query });
expect(errors).toBeUndefined();
expect((data?._service as any).sdl).toEqual(`${header}type User @key(fields: "email") @tag(name: "tagOnType") {
email: String @tag(name: "tagOnField")
}

interface Thing @tag(name: "tagOnInterface") {
name: String
}

union UserButAUnion @tag(name: "tagOnUnion") = User
`);
};

it.each([
{name: 'fed1', header: ''},
{name: 'fed2', header: 'extend schema @link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@tag"])\n\n' }
])('adds it for $name schema', async ({header}) => {
await validateTag(header);
});
});
});

describe('legacy interface', () => {
Expand Down
4 changes: 0 additions & 4 deletions subgraph-js/src/__tests__/printSubgraphSchema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ describe('printSubgraphSchema', () => {

directive @transform(from: String!) on FIELD

directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION

directive @cacheControl(maxAge: Int, scope: CacheControlScope, inheritMaxAge: Boolean) on FIELD_DEFINITION | OBJECT | INTERFACE | UNION

enum CacheControlScope {
Expand Down Expand Up @@ -103,8 +101,6 @@ describe('printSubgraphSchema', () => {

directive @transform(from: String!) on FIELD

directive @tag(name: String!) repeatable on INTERFACE | FIELD_DEFINITION | OBJECT | UNION

type Query {
_entities(representations: [_Any!]!): [_Entity]!
_service: _Service!
Expand Down
21 changes: 20 additions & 1 deletion subgraph-js/src/buildSubgraphSchema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import {
GraphQLUnionType,
GraphQLObjectType,
specifiedDirectives,
visit,
GraphQLDirective,
} from 'graphql';
import {
GraphQLSchemaModule,
Expand All @@ -30,6 +32,23 @@ type LegacySchemaModule = {

export { GraphQLSchemaModule };

function missingFederationDirectives(modules: GraphQLSchemaModule[]): GraphQLDirective[] {
// Copying the array as we're going to modify it.
const missingDirectives = federationDirectives.concat();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May not matter too much, but it would probably be better to use a set to avoid the O(n) lookup in a loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's one those rare case where the array size is not dynamic, we know exactly how many elements the array, it's a very small number and we kind of know it's never going to grow to a large number (relatively). So complexity argument kind of don't apply, and I'd be surprise if the array version is not as fast if not faster (but I agree it doesn't matter either way in practice).

for (const mod of modules) {
visit(mod.typeDefs, {
DirectiveDefinition: (def) => {
const matchingFedDirectiveIdx = missingDirectives.findIndex((d) => d.name === def.name.value);
if (matchingFedDirectiveIdx >= 0) {
missingDirectives.splice(matchingFedDirectiveIdx, 1);
}
return def;
}
});
}
return missingDirectives;
}

export function buildSubgraphSchema(
modulesOrSDL:
| (GraphQLSchemaModule | DocumentNode)[]
Expand Down Expand Up @@ -67,7 +86,7 @@ export function buildSubgraphSchema(
modules,
new GraphQLSchema({
query: undefined,
directives: [...specifiedDirectives, ...federationDirectives],
directives: [...specifiedDirectives, ...missingFederationDirectives(modules)],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand correctly, we are going through the AST to figure out which federation directives are not present and then passing them in a list? Can you explain why it isn't the federation directives that are present that we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we visit the AST, we only look at directive definitions, not application. And what we're adding is the directives definition that don't already have a definition in the user provided schema.

We're not looking at applications at all. Which does mean we may be adding @provides definition even though it's never used by that particular schema, and we could well avoid it, but buildSubgraphSchema has always done that, adding all federation directive definition regardless of what is being used (which is harmless), and so keeping the code change focused.

The problem we're solving with this is that we want the @tag definition to be added automatically, like all other federation directive definitions, but the difference is that we historically have asked fed1 user to provide the definition themselves. So to be backward compatible, we just need to ensure we don't try to auto-add definitions that are already user defined, which wasn't something buildSubgraphSchema was previously bothering with.

}),
);

Expand Down
12 changes: 1 addition & 11 deletions subgraph-js/src/directives.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,23 +110,13 @@ export const federationDirectives = [
ProvidesDirective,
ShareableDirective,
LinkDirective,
TagDirective,
];

export function isFederationDirective(directive: GraphQLDirective): boolean {
return federationDirectives.some(({ name }) => name === directive.name);
}

export const otherKnownDirectives = [TagDirective];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note for reviewers: an option could have been to keep that notion of "known but not officially federation" directives but have it empty for now. But if anything, we're trying to move away from hard-coded directives and toward @link-defined directives, so I'm almost certain this would never be used again, so removed it (in fact, #1554 is probably going to change all of this before GA anyway).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that it's better to delete code that is unused rather than having it hand around!


export const knownSubgraphDirectives = [
...federationDirectives,
...otherKnownDirectives,
];

export function isKnownSubgraphDirective(directive: GraphQLDirective): boolean {
return knownSubgraphDirectives.some(({ name }) => name === directive.name);
}

export type ASTNodeWithDirectives =
| FieldDefinitionNode
| InputValueDefinitionNode
Expand Down
29 changes: 3 additions & 26 deletions subgraph-js/src/printSubgraphSchema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import { isFederationType, Maybe } from './types';
import {
gatherDirectives,
federationDirectives,
otherKnownDirectives,
isFederationDirective,
} from './directives';

Expand Down Expand Up @@ -213,8 +212,6 @@ function printObject(type: GraphQLObjectType): string {
printImplementedInterfaces(type) +
// Apollo addition: print @key usages
printFederationDirectives(type) +
// Apollo addition: print @tag usages (or other known directives)
printKnownDirectiveUsagesOnTypeOrField(type) +
printFields(type)
);
}
Expand All @@ -233,8 +230,8 @@ function printInterface(type: GraphQLInterfaceType): string {
(isExtension ? 'extend ' : '') +
`interface ${type.name}` +
printImplementedInterfaces(type) +
// Apollo addition: print @key usages
printFederationDirectives(type) +
printKnownDirectiveUsagesOnTypeOrField(type) +
printFields(type)
);
}
Expand All @@ -247,7 +244,7 @@ function printUnion(type: GraphQLUnionType): string {
'union ' +
type.name +
// Apollo addition: print @tag usages
printKnownDirectiveUsagesOnTypeOrField(type) +
printFederationDirectives(type) +
possibleTypes
);
}
Expand Down Expand Up @@ -284,8 +281,7 @@ function printFields(type: GraphQLObjectType | GraphQLInterfaceType) {
String(f.type) +
printDeprecated(f.deprecationReason) +
// Apollo addition: print Apollo directives on fields
printFederationDirectives(f) +
printKnownDirectiveUsagesOnTypeOrField(f),
printFederationDirectives(f),
);
return printBlock(fields);
}
Expand All @@ -307,25 +303,6 @@ function printFederationDirectives(
return dedupedDirectives.length > 0 ? ' ' + dedupedDirectives.join(' ') : '';
}

// Apollo addition: print `@tag` directive usages (and possibly other future known
// directive usages) found in subgraph SDL.
function printKnownDirectiveUsagesOnTypeOrField(
typeOrField: GraphQLNamedType | GraphQLField<any, any>,
): string {
if (!typeOrField.astNode) return '';
if (isInputObjectType(typeOrField)) return '';

const knownSubgraphDirectivesOnTypeOrField = gatherDirectives(typeOrField)
.filter((n) =>
otherKnownDirectives.some((directive) => directive.name === n.name.value),
)
.map(print);

return knownSubgraphDirectivesOnTypeOrField.length > 0
? ' ' + knownSubgraphDirectivesOnTypeOrField.join(' ')
: '';
}

function printBlock(items: string[]) {
return items.length !== 0 ? ' {\n' + items.join('\n') + '\n}' : '';
}
Expand Down