-
Notifications
You must be signed in to change notification settings - Fork 254
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
trevor-scheer
merged 4 commits into
release-federation-0.26.0
from
trevor/print-tag-inaccessible-directives-conditionally
Jul 7, 2021
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
11e695b
Conditionally print applied directive definitions + core declarations
trevor-scheer 9dfe41a
Update tests and query-planner restrictions
trevor-scheer fca58c0
Reuse existing constants in special-case test fixtures
trevor-scheer 1460a60
Cleanup
trevor-scheer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
58 changes: 58 additions & 0 deletions
58
...n-integration-testsuite-js/src/fixtures/special-cases/accountsWithoutTagOrInaccessible.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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") | ||
} | ||
`; |
2 changes: 1 addition & 1 deletion
2
...uite-js/src/fixtures/reviewsWithUpdate.ts → ...xtures/special-cases/reviewsWithUpdate.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
90 changes: 90 additions & 0 deletions
90
...on-integration-testsuite-js/src/fixtures/special-cases/reviewsWithoutTagOrInaccessible.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
`; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ import { | |
DirectiveNode, | ||
} from 'graphql'; | ||
import { transformSchema } from 'apollo-graphql'; | ||
import apolloTypeSystemDirectives from '../directives'; | ||
import apolloTypeSystemDirectives, { appliedDirectives, federationDirectives } from '../directives'; | ||
import { | ||
findDirectivesOnNode, | ||
isStringValueNode, | ||
|
@@ -35,7 +35,7 @@ import { | |
getFederationMetadata, | ||
CompositionResult, | ||
isDirectiveDefinitionNode, | ||
isApolloTypeSystemDirective | ||
isFederationDirective | ||
} from './utils'; | ||
import { | ||
ServiceDefinition, | ||
|
@@ -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>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
|
@@ -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]); | ||
} | ||
} | ||
} | ||
|
@@ -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, | ||
], | ||
}); | ||
|
||
|
@@ -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; | ||
|
@@ -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], | ||
), | ||
], | ||
}; | ||
|
@@ -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)), | ||
], | ||
}); | ||
|
||
|
@@ -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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 likeotherKnownDirectives
could work here?