-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Validate against using keys that haven't been declared #3227
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
4e06455
Validate against using keys that havent been declared
7c2d9af
Exit early on internal consistancy error
0b92c63
Remove uneeded assertion. Continue to check in face of strange intern…
26d4c3e
Update comment
8c02be6
Respond to PR
37b36f0
Fixup snapshots
6a29cae
Add note about why not graphql-print
571726e
Merge branch 'master' into key-not-declared-validation
abernix 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
212 changes: 212 additions & 0 deletions
212
...ation/src/composition/validate/postComposition/__tests__/keySelectionSetsDeclared.test.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,212 @@ | ||
import gql from 'graphql-tag'; | ||
import { composeServices } from '../../../compose'; | ||
import { keySelectionSetsDeclared } from '..'; | ||
import { graphqlErrorSerializer } from '../../../../snapshotSerializers'; | ||
|
||
expect.addSnapshotSerializer(graphqlErrorSerializer); | ||
|
||
describe('keySelectionSetsDeclared', () => { | ||
it('returns no warnings with proper @key usage', () => { | ||
const serviceA = { | ||
typeDefs: gql` | ||
type Product @key(fields: "sku") { | ||
sku: String! | ||
} | ||
`, | ||
name: 'serviceA', | ||
}; | ||
|
||
const serviceB = { | ||
typeDefs: gql` | ||
extend type Product @key(fields: "sku") { | ||
sku: String! @external | ||
price: Int! | ||
} | ||
`, | ||
name: 'serviceB', | ||
}; | ||
|
||
const { schema, errors } = composeServices([serviceA, serviceB]); | ||
expect(errors).toHaveLength(0); | ||
|
||
const warnings = keySelectionSetsDeclared(schema); | ||
expect(warnings).toHaveLength(0); | ||
}); | ||
|
||
it("warns if @key is used that isn't declared on the base service", () => { | ||
const serviceA = { | ||
typeDefs: gql` | ||
type Product @key(fields: "sku") { | ||
sku: String! | ||
upc: String! | ||
} | ||
`, | ||
name: 'serviceA', | ||
}; | ||
|
||
const serviceB = { | ||
typeDefs: gql` | ||
extend type Product @key(fields: "upc") { | ||
sku: String! @external | ||
upc: String! @external | ||
price: Int! | ||
} | ||
`, | ||
name: 'serviceB', | ||
}; | ||
|
||
const { schema, errors } = composeServices([serviceA, serviceB]); | ||
expect(errors).toHaveLength(0); | ||
|
||
const warnings = keySelectionSetsDeclared(schema); | ||
expect(warnings).toMatchInlineSnapshot(` | ||
Array [ | ||
Object { | ||
"code": "KEY_NOT_DECLARED", | ||
"message": "[serviceB] Product -> uses the key \`upc\`, which is not declared on the base service. All keys must be declared on the base service.", | ||
}, | ||
] | ||
`); | ||
}); | ||
|
||
it('does not warn if the only difference is ordering of fields', () => { | ||
const serviceA = { | ||
typeDefs: gql` | ||
type Product | ||
@key( | ||
fields: "upc sku thing { a b product { upc sku thing { a b } } }" | ||
) { | ||
sku: String! | ||
upc: String! | ||
thing: Thing! | ||
} | ||
|
||
type Thing { | ||
product: Product! | ||
a: String | ||
b: String | ||
} | ||
`, | ||
name: 'serviceA', | ||
}; | ||
|
||
const serviceB = { | ||
typeDefs: gql` | ||
extend type Product | ||
@key( | ||
fields: "upc thing { a product { thing { b a } sku upc } b } sku" | ||
) { | ||
sku: String! @external | ||
upc: String! @external | ||
price: Int! | ||
} | ||
|
||
type Thing { | ||
product: Product! | ||
a: String | ||
b: String | ||
} | ||
`, | ||
name: 'serviceB', | ||
}; | ||
|
||
const { schema, errors } = composeServices([serviceA, serviceB]); | ||
expect(errors).toHaveLength(0); | ||
|
||
const warnings = keySelectionSetsDeclared(schema); | ||
expect(warnings).toHaveLength(0); | ||
}); | ||
|
||
it('warns if compound keys do not have the same fields', () => { | ||
const serviceA = { | ||
typeDefs: gql` | ||
type Product @key(fields: "upc sku thing { a b }") { | ||
sku: String! | ||
upc: String! | ||
thing: Thing! | ||
} | ||
|
||
type Thing { | ||
product: Product! | ||
a: String | ||
b: String | ||
} | ||
`, | ||
name: 'serviceA', | ||
}; | ||
|
||
const serviceB = { | ||
typeDefs: gql` | ||
extend type Product @key(fields: "upc sku thing { a }") { | ||
sku: String! @external | ||
upc: String! @external | ||
price: Int! | ||
} | ||
|
||
type Thing { | ||
product: Product! | ||
a: String | ||
b: String | ||
} | ||
`, | ||
name: 'serviceB', | ||
}; | ||
|
||
const { schema, errors } = composeServices([serviceA, serviceB]); | ||
expect(errors).toHaveLength(0); | ||
|
||
const warnings = keySelectionSetsDeclared(schema); | ||
expect(warnings).toMatchInlineSnapshot(` | ||
Array [ | ||
Object { | ||
"code": "KEY_NOT_DECLARED", | ||
"message": "[serviceB] Product -> uses the key \`upc sku thing { a }\`, which is not declared on the base service. All keys must be declared on the base service.", | ||
}, | ||
] | ||
`); | ||
}); | ||
|
||
it('does not warn if one of the multiple primary keys matches', () => { | ||
const serviceA = { | ||
typeDefs: gql` | ||
type Product | ||
@key(fields: "upc sku thing { a b }") | ||
@key(fields: "upc sku") { | ||
sku: String! | ||
upc: String! | ||
thing: Thing! | ||
} | ||
|
||
type Thing { | ||
product: Product! | ||
a: String | ||
b: String | ||
} | ||
`, | ||
name: 'serviceA', | ||
}; | ||
|
||
const serviceB = { | ||
typeDefs: gql` | ||
extend type Product @key(fields: "upc sku") { | ||
sku: String! @external | ||
upc: String! @external | ||
price: Int! | ||
} | ||
|
||
type Thing { | ||
product: Product! | ||
a: String | ||
b: String | ||
} | ||
`, | ||
name: 'serviceB', | ||
}; | ||
|
||
const { schema, errors } = composeServices([serviceA, serviceB]); | ||
expect(errors).toHaveLength(0); | ||
|
||
const warnings = keySelectionSetsDeclared(schema); | ||
expect(warnings).toHaveLength(0); | ||
}); | ||
}); |
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
91 changes: 91 additions & 0 deletions
91
...es/apollo-federation/src/composition/validate/postComposition/keySelectionSetsDeclared.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,91 @@ | ||
import { GraphQLSchema, isObjectType, FieldNode, GraphQLError } from 'graphql'; | ||
|
||
import { logServiceAndType, errorWithCode } from '../../utils'; | ||
|
||
/** | ||
* - Each key's selection set must be present on the base service | ||
*/ | ||
export const keySelectionSetsDeclared = (schema: GraphQLSchema) => { | ||
const errors: GraphQLError[] = []; | ||
|
||
const types = schema.getTypeMap(); | ||
for (const [typeName, namedType] of Object.entries(types)) { | ||
if (!isObjectType(namedType)) continue; | ||
|
||
if (namedType.federation && namedType.federation.keys) { | ||
const baseService = namedType.federation.serviceName; | ||
if (!baseService) { | ||
// This is an invalid case which is handled by the UniqueTypeNamesWithFields validator, throwing error VALUE_TYPE_NO_ENTITY | ||
continue; | ||
} | ||
|
||
const baseKeys = namedType.federation.keys[baseService] as FieldNode[][]; | ||
|
||
for (const [serviceName, selectionSets] of Object.entries( | ||
namedType.federation.keys, | ||
)) { | ||
for (const selectionSet of selectionSets as FieldNode[][]) { | ||
if (!baseKeys.some(key => selectionSetsEqual(key, selectionSet))) { | ||
errors.push( | ||
errorWithCode( | ||
'KEY_NOT_DECLARED', | ||
logServiceAndType(serviceName, typeName) + | ||
`uses the key \`${selectionSetToString( | ||
selectionSet, | ||
)}\`, which is not declared on the base service. All keys must be declared on the base service.`, | ||
), | ||
); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
return errors; | ||
}; | ||
|
||
// graphql.print(...) gives non-ideal formatting. | ||
const selectionSetToString = (a: FieldNode[]): string => | ||
a | ||
.map( | ||
node => | ||
node.name.value + | ||
(node.selectionSet | ||
? ` { ${selectionSetToString(node.selectionSet | ||
.selections as FieldNode[])} }` | ||
: ''), | ||
) | ||
.join(' '); | ||
|
||
const selectionSetsEqual = (a: FieldNode[], b: FieldNode[]) => { | ||
if (a.length !== b.length) return false; | ||
a = a.slice(); | ||
b = b.slice(); | ||
|
||
a.sort((_a, _b) => _a.name.value.localeCompare(_b.name.value)); | ||
b.sort((_a, _b) => _a.name.value.localeCompare(_b.name.value)); | ||
|
||
for (let i = 0; i < a.length; i++) { | ||
const nodeA = a[i]; | ||
const nodeB = b[i]; | ||
|
||
if (nodeA.name.value !== nodeB.name.value) return false; | ||
|
||
const nodeASelections = nodeA.selectionSet; | ||
const nodeBSelections = nodeB.selectionSet; | ||
|
||
if (nodeASelections && nodeBSelections) { | ||
if ( | ||
!selectionSetsEqual( | ||
nodeASelections.selections as FieldNode[], | ||
nodeBSelections.selections as FieldNode[], | ||
) | ||
) { | ||
return false; | ||
} | ||
} else if (nodeASelections || nodeBSelections) { | ||
return false; | ||
} | ||
} | ||
return true; | ||
}; |
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.
Can we use
graphql.print()
for this or is the outcome different from what I'm imagining?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.
It gave weird formatting in my experience, also it doesn't operate that smoothly with the
FieldNode
array, and we don't have aSelectionSet
to pass it, so idk. Left a comment.