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

fix(federation): Print @tag and @inaccessible directives conditionally #859

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions federation-integration-testsuite-js/src/fixtures/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import * as documents from './documents';
import * as inventory from './inventory';
import * as product from './product';
import * as reviews from './reviews';
import * as reviewsWithUpdate from './reviewsWithUpdate'
import * as reviewsWithUpdate from './special-cases/reviewsWithUpdate';
import * as accountsWithoutTagOrInaccessible from './special-cases/accountsWithoutTagOrInaccessible';
import * as reviewsWithoutTagOrInaccessible from './special-cases/reviewsWithoutTagOrInaccessible';

export {
accounts,
Expand All @@ -13,7 +15,7 @@ export {
inventory,
product,
reviews,
reviewsWithUpdate
reviewsWithUpdate,
};

export const fixtures = [
Expand All @@ -34,6 +36,15 @@ export const fixturesWithUpdate = [
reviewsWithUpdate,
];

export const fixturesWithoutTagOrInaccessible = [
accountsWithoutTagOrInaccessible,
books,
documents,
inventory,
product,
reviewsWithoutTagOrInaccessible,
];

export const fixtureNames = [
accounts.name,
product.name,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import gql from 'graphql-tag';

export { name, url, resolvers } from '../accounts';
export const typeDefs = gql`
directive @stream on FIELD
directive @transform(from: String!) on FIELD

schema {
query: RootQuery
mutation: Mutation
}

extend type RootQuery {
user(id: ID!): User
me: User
}

type PasswordAccount @key(fields: "email") {
email: String!
}

type SMSAccount @key(fields: "number") {
number: String
}

union AccountType = PasswordAccount | SMSAccount

type UserMetadata {
name: String
address: String
description: String
}

type User @key(fields: "id") @key(fields: "username name { first last }") {
id: ID!
name: Name
username: String
birthDate(locale: String): String
account: AccountType
metadata: [UserMetadata]
ssn: String
}

type Name {
first: String
last: String
}

type Mutation {
login(username: String!, password: String!): User
}

extend type Library @key(fields: "id") {
id: ID! @external
name: String @external
userAccount(id: ID! = "1"): User @requires(fields: "name")
}
`;
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import gql from 'graphql-tag';

import * as reviewsService from './reviews';
import * as reviewsService from '../reviews';

export const name = reviewsService.name;
export const url = reviewsService.url;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import gql from 'graphql-tag';

export { name, url, resolvers } from '../reviews';
export const typeDefs = gql`
directive @stream on FIELD
directive @transform(from: String!) on FIELD

extend type Query {
topReviews(first: Int = 5): [Review]
}

type Review @key(fields: "id") {
id: ID!
body(format: Boolean = false): String
author: User @provides(fields: "username")
product: Product
metadata: [MetadataOrError]
}

input UpdateReviewInput {
id: ID!
body: String
}

extend type UserMetadata {
address: String @external
}

extend type User @key(fields: "id") {
id: ID! @external
username: String @external
reviews: [Review]
numberOfReviews: Int!
metadata: [UserMetadata] @external
goodAddress: Boolean @requires(fields: "metadata { address }")
}

extend interface Product {
reviews: [Review]
}

extend type Furniture implements Product @key(fields: "upc") {
upc: String! @external
reviews: [Review]
}

extend type Book implements Product @key(fields: "isbn") {
isbn: String! @external
reviews: [Review]
similarBooks: [Book]! @external
relatedReviews: [Review!]! @requires(fields: "similarBooks { isbn }")
}

extend interface Vehicle {
retailPrice: String
}

extend type Car implements Vehicle @key(fields: "id") {
id: String! @external
price: String @external
retailPrice: String @requires(fields: "price")
}

extend type Van implements Vehicle @key(fields: "id") {
id: String! @external
price: String @external
retailPrice: String @requires(fields: "price")
}

extend type Mutation {
reviewProduct(upc: String!, body: String!): Product
updateReview(review: UpdateReviewInput!): Review
deleteReview(id: ID!): Boolean
}

# Value type
type KeyValue {
key: String!
value: String!
}

# Value type
type Error {
code: Int
message: String
}

# Value type
union MetadataOrError = KeyValue | Error
`;
68 changes: 43 additions & 25 deletions federation-js/src/composition/compose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
DirectiveNode,
} from 'graphql';
import { transformSchema } from 'apollo-graphql';
import apolloTypeSystemDirectives from '../directives';
import apolloTypeSystemDirectives, { appliedDirectives, federationDirectives } from '../directives';
Copy link
Contributor

Choose a reason for hiding this comment

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

I had missed this before, but appliedDirectives doesn't seem like the right name here, because these exports are directive definitions (as opposed to directive applications, which is what we find on AST nodes). Maybe something like otherKnownDirectives could work here?

import {
findDirectivesOnNode,
isStringValueNode,
Expand All @@ -35,7 +35,7 @@ import {
getFederationMetadata,
CompositionResult,
isDirectiveDefinitionNode,
isApolloTypeSystemDirective
isFederationDirective
} from './utils';
import {
ServiceDefinition,
Expand Down Expand Up @@ -129,6 +129,11 @@ type FieldDirectivesMap = Map<string, DirectiveNode[]>;
// TODO: rename?
type TypeNameToFieldDirectivesMap = Map<string, FieldDirectivesMap>;

/**
* A set of directive names that have been used at least once
*/
type AppliedDirectiveUsages = Set<string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a blocker, but the term 'applied directive usages' is a bit weird because both 'applied directives' and 'directive usages' are meant to differentiate between directive definitions and their use.


/**
* Loop over each service and process its typeDefs (`definitions`)
* - build up typeToServiceMap
Expand All @@ -143,6 +148,7 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
const keyDirectivesMap: KeyDirectivesMap = Object.create(null);
const valueTypes: ValueTypes = new Set();
const typeNameToFieldDirectivesMap: TypeNameToFieldDirectivesMap = new Map();
const appliedDirectiveUsages: AppliedDirectiveUsages = new Set();

for (const { typeDefs, name: serviceName } of serviceList) {
// Build a new SDL with @external fields removed, as well as information about
Expand Down Expand Up @@ -190,19 +196,24 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
for (const field of definition.fields ?? []) {
const fieldName = field.name.value;

const tagAndInaccessibleDirectives = [
...findDirectivesOnNode(field, 'tag'),
...findDirectivesOnNode(field, 'inaccessible'),
];
const tagUsages = findDirectivesOnNode(field, 'tag');
const inaccessibleUsages = findDirectivesOnNode(
field,
'inaccessible',
);

if (tagUsages.length > 0) appliedDirectiveUsages.add('tag');
if (inaccessibleUsages.length > 0)
appliedDirectiveUsages.add('inaccessible');

if (tagAndInaccessibleDirectives.length > 0) {
if (tagUsages.length > 0 || inaccessibleUsages.length > 0) {
const fieldToDirectivesMap = mapGetOrSet(
typeNameToFieldDirectivesMap,
typeName,
new Map(),
);
const directives = mapGetOrSet(fieldToDirectivesMap, fieldName, []);
directives.push(...tagAndInaccessibleDirectives);
directives.push(...[...tagUsages, ...inaccessibleUsages]);
}
}
}
Expand Down Expand Up @@ -386,25 +397,35 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
keyDirectivesMap,
valueTypes,
typeNameToFieldDirectivesMap,
appliedDirectiveUsages,
};
}

export function buildSchemaFromDefinitionsAndExtensions({
typeDefinitionsMap,
typeExtensionsMap,
directiveDefinitionsMap,
appliedDirectiveUsages,
}: {
typeDefinitionsMap: TypeDefinitionsMap;
typeExtensionsMap: TypeExtensionsMap;
directiveDefinitionsMap: DirectiveDefinitionsMap;
appliedDirectiveUsages: AppliedDirectiveUsages;
}) {
let errors: GraphQLError[] | undefined = undefined;

// We only want to include the definitions of applied directives (currently
// just @tag and @include) if there are usages.
const appliedDirectivesToInclude = appliedDirectives.filter((directive) =>
appliedDirectiveUsages.has(directive.name),
);

let schema = new GraphQLSchema({
query: undefined,
directives: [
...specifiedDirectives,
...apolloTypeSystemDirectives,
...federationDirectives,
...appliedDirectivesToInclude,
],
});

Expand All @@ -425,23 +446,19 @@ export function buildSchemaFromDefinitionsAndExtensions({
const definitionsDocument: DocumentNode = {
kind: Kind.DOCUMENT,
definitions: [
...Object.values(typeDefinitionsMap).flatMap(typeDefinitions => {
...Object.values(typeDefinitionsMap).flatMap((typeDefinitions) => {
// See if any of our Objects or Interfaces implement any interfaces at all.
// If not, we can return early.
if (!typeDefinitions.some(nodeHasInterfaces)) return typeDefinitions;

const uniqueInterfaces: Map<
string,
NamedTypeNode
> = (typeDefinitions as HasInterfaces[]).reduce(
(map, objectTypeDef) => {
objectTypeDef.interfaces?.forEach((iface) =>
map.set(iface.name.value, iface),
);
return map;
},
new Map(),
);
const uniqueInterfaces: Map<string, NamedTypeNode> = (
typeDefinitions as HasInterfaces[]
).reduce((map, objectTypeDef) => {
objectTypeDef.interfaces?.forEach((iface) =>
map.set(iface.name.value, iface),
);
return map;
}, new Map());

// No interfaces, no aggregation - just return what we got.
if (uniqueInterfaces.size === 0) return typeDefinitions;
Expand All @@ -455,10 +472,9 @@ export function buildSchemaFromDefinitionsAndExtensions({
interfaces: Array.from(uniqueInterfaces.values()),
},
];

}),
...Object.values(directiveDefinitionsMap).map(
definitions => Object.values(definitions)[0],
(definitions) => Object.values(definitions)[0],
),
],
};
Expand Down Expand Up @@ -489,7 +505,7 @@ export function buildSchemaFromDefinitionsAndExtensions({
schema = new GraphQLSchema({
...schema.toConfig(),
directives: [
...schema.getDirectives().filter((x) => !isApolloTypeSystemDirective(x)),
...schema.getDirectives().filter((x) => !isFederationDirective(x)),
],
});

Expand Down Expand Up @@ -705,12 +721,14 @@ export function composeServices(services: ServiceDefinition[]): CompositionResul
keyDirectivesMap,
valueTypes,
typeNameToFieldDirectivesMap,
appliedDirectiveUsages,
} = buildMapsFromServiceList(services);

let { schema, errors } = buildSchemaFromDefinitionsAndExtensions({
typeDefinitionsMap,
typeExtensionsMap,
directiveDefinitionsMap,
appliedDirectiveUsages,
});

// TODO: We should fix this to take non-default operation root types in
Expand Down
5 changes: 5 additions & 0 deletions federation-js/src/composition/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
} from './types';
import apolloTypeSystemDirectives, {
ASTNodeWithDirectives,
federationDirectives,
} from '../directives';
import { assert, isNotNullOrUndefined } from '../utilities';

Expand Down Expand Up @@ -633,6 +634,10 @@ export function isApolloTypeSystemDirective(directive: GraphQLDirective): boolea
return apolloTypeSystemDirectives.some(({ name }) => name === directive.name);
}

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

export const reservedRootFields = ['_service', '_entities'];

// Map of OperationTypeNode to its respective default root operation type name
Expand Down