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

Jump to bottom

Signature normalization, ordering fragments #2282

Closed
wants to merge 10 commits into from
Closed

Signature normalization, ordering fragments #2282

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6f3313e
Commit failing test
trevor-scheer Feb 7, 2019
a494e94
Add one more passing test case to show that order of fragments matters
trevor-scheer Feb 7, 2019
ec695b9
Add top-level definition sorting to sortAST
trevor-scheer Feb 8, 2019
24dcd99
Add tests and fix broken example
trevor-scheer Feb 8, 2019
1b70cd3
Create utilities file, move / remove utilities.
trevor-scheer Feb 8, 2019
d1aeb3a
Update test to use defaultEngineSignature function instead of a slightly
trevor-scheer Feb 8, 2019
2ad998b
Rename sortAST -> lexicographicSortOperations
trevor-scheer Feb 9, 2019
602eb66
Update and create new test cases
trevor-scheer Feb 9, 2019
077e2c7
Add no-focused-test tslint rule, added tslint config for apollo-graphql
trevor-scheer Feb 9, 2019
d341e08
Leverage the visitor for sorting top level nodes as well
trevor-scheer Feb 9, 2019
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
117 changes: 116 additions & 1 deletion packages/apollo-graphql/src/__tests__/signature.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
// breaks if you turn it off in tests.
disableFragmentWarnings();

describe('aggressive signature', () => {
describe.only('aggressive signature', () => {
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
function aggressive(ast: DocumentNode, operationName: string): string {
return printWithReducedWhitespace(
removeAliases(
Expand Down Expand Up @@ -106,6 +106,121 @@ describe('aggressive signature', () => {
`,
output: '{user{name...Bar}}fragment Bar on User{asd}',
},
{
name: 'fragments out of order',
operationName: '',
input: gql`
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
fragment Baz on User {
jkl
}

{
user {
name
...Bar
...Baz
}
}

fragment Bar on User {
asd
}
`,
output:
'{user{name...Bar...Baz}}fragment Bar on User{asd}fragment Baz on User{jkl}',
},
{
name: 'fragments in order',
operationName: '',
input: gql`
{
user {
name
...Bar
...Baz
}
}

fragment Bar on User {
asd
}

fragment Baz on User {
jkl
}
`,
output:
'{user{name...Bar...Baz}}fragment Bar on User{asd}fragment Baz on User{jkl}',
},
{
name: 'fragments in order',
operationName: '',
input: gql`
{
user {
name
...Bar
...Baz
}
}

fragment Bar on User {
asd
}

fragment Baz on User {
jkl
}
`,
output:
'{user{name...Bar...Baz}}fragment Bar on User{asd}fragment Baz on User{jkl}',
},
{
name: 'with a subscription',
operationName: '',
input: gql`
fragment Bar on User {
asd
}

fragment Baz on User {
jkl
}

subscription A {
user {
name
...Bar
...Baz
}
}
`,
output:
'subscription A{user{name...Bar...Baz}}' +
'fragment Bar on User{asd}fragment Baz on User{jkl}',
},
{
name: 'with a mutation',
operationName: '',
input: gql`
mutation A {
user {
name
...Bar
...Baz
}
}
fragment Baz on User {
jkl
}
fragment Bar on User {
asd
}
`,
output:
'mutation A{user{name...Bar...Baz}}' +
'fragment Bar on User{asd}fragment Baz on User{jkl}',
},
Copy link
Member

Choose a reason for hiding this comment

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

These tests are invaluable. Thank you so much for adding these!

I suspect we would see additional value (now or in the future) in a test which had multiple named operations in it. The spec states that if there is a single operation in a document, it need not be named, however if there are 2 or more, each must be named. I think it would be smart of us to try to incorporate tests which test that sorting behavior. Would that be something you'd entertain adding?

Currently, if a document contains multiple operations, the name of the operation for which single execution is desired should be passed in. The graphql/graphql-spec#375 RFC proposes another plan for the future, if you're interested.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is very helpful articulation of some behavior I was seeing during further implementation today.

I'll gladly add some tests specific to what you've mentioned (if I haven't already - will go back and take a look). I went a little crazy adding tests since I found TDD to be the fastest and best way to implement these changes :woo:!

{
name: 'full test',
operationName: 'Foo',
Expand Down
27 changes: 26 additions & 1 deletion packages/apollo-graphql/src/transforms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
FragmentDefinitionNode,
ObjectValueNode,
ListValueNode,
ASTNode,
} from 'graphql/language/ast';
import { print } from 'graphql/language/printer';
import { separateOperations } from 'graphql/utilities';
Expand Down Expand Up @@ -94,13 +95,37 @@ function sorted<T>(
return undefined;
}

// Predicate for filtering and type checking operation definitions (query, mutation, subscription)
// Similar to graphql's isExecutableDefinitionNode
function isOperationDefinitionNode(
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
node: ASTNode,
): node is OperationDefinitionNode {
return node.kind === 'OperationDefinition';
}

// Predicate for filtering and type checking fragment definitions
// Similar to graphql's isExecutableDefinitionNode
function isFragmentDefinitionNode(
node: ASTNode,
): node is FragmentDefinitionNode {
return node.kind === 'FragmentDefinition';
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
}

// sortAST sorts most multi-child nodes alphabetically. Using this as part of
// your signature calculation function may make it easier to tell the difference
// between queries that are similar to each other, and if for some reason your
// GraphQL client generates query strings with elements in nondeterministic
// order, it can make sure the queries are treated as identical.
export function sortAST(ast: DocumentNode): DocumentNode {
return visit(ast, {
const [operation] = ast.definitions.filter(isOperationDefinitionNode);
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think we should avoid doing direct filtering passes on the AST to account for Nodes (like FragmentDefinitionNodes) which may not exist.

Also, while the typing of this sortAST function currently accepts a DocumentNode (right now), it should theoretically be able to handle sub-ASTs as well (GraphQL's visit can actually handle more than just DocumentNode), if we ever desired to do so.

I think remaining aligned with the original behavior of this sortAST to be a direct implementation of a visitor may be still possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, totally agree! Are you recommending changes for the input type (DocumentNode) or just pointing out that we may be able to make this more flexible in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Visitor comment resolved in d341e08.
Leaving this thread open for question above.

const fragments = ast.definitions.filter(isFragmentDefinitionNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

I fear that this is overly specific. Given the definition of DefinitionNode and ExecutableDefinitionNode:

export type DefinitionNode =
  | ExecutableDefinitionNode
  | TypeSystemDefinitionNode
  | TypeSystemExtensionNode;

export type ExecutableDefinitionNode =
  | OperationDefinitionNode
  | FragmentDefinitionNode;

I don't know what TypeSystemDefinitionNode or TypeSystemExtensionNode are, but they are legal values in a DefinitionNode. This also would be susceptible to be other new things added in the future. With that lack of knowledge, I think it would make sense to favor using "fragments and everything else" versus "fragments and operations". Thoughts?

const nonFragments = ast.definitions.filter((node) => ! isFragmentDefinitionNode(node))
const fragments = ast.definitions.filter(isFragmentDefinitionNode);

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm operating under the assumption that the document will only have ExecutableDefinitionNodes else it wouldn't be a valid operation signature. I wouldn't expect the other two TypeSystemXNodes to turn up in an operation (they're used in SDL) but that may be an incorrect assumption.

Regardless of what I find, I'll add some documentation here to explain.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is totally true for our use case. I don't think you can be positive that it covers all use cases for both us and consumers of apollo-graphql.

The function is called sortAST and accepts a DocumentNode; I think you need to account for all legal values that might exist in a DocumentNode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Left a piece off the above comment: this function is called sortAST; so it might be used to sort any kid of AST. Restricting this to DocumentNodes that only include ExecutableDefinitionNodes doesn't feel right.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think giving this a more specific name, like lexicographicSortOperations (by analogy with lexicographicSortSchema might be more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filtering out non-ExecutableDefinitionNodes (as you currently do) might be ok, that is also what execute does. Alternatively, maybe we could put them all at the end?

Copy link
Member

Choose a reason for hiding this comment

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

I support a different name! But I'm pretty okay with not exposing sortAST as a public API, instead only exposing abstractions that implement it (like defaultEngineReportingSignature in the adjacent index.ts)

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved in 2ad998b

Following @martijnwalraven 's suggestion of parity with the graphql function, I've opted to rename (and effectively repurpose) this: lexicographicSortOperations.

This is worth discussing - it's a breaking change but this package is new and may not be in use. Regardless, this requires a major version bump.

Copy link
Member Author

Choose a reason for hiding this comment

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

@abernix I'm just now seeing your comments as I had a stale page up while responding to comments. I'm open to not exporting this function publicly.

trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved

const astWithSortedDefinitions = {
...ast,
trevor-scheer marked this conversation as resolved.
Show resolved Hide resolved
definitions: [operation, ...sortBy(fragments, 'name.value')],
Copy link
Member

Choose a reason for hiding this comment

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

I believe that in order to achieve a more reliable normalized signature, we should first sort the definitions on a DocumentNode by their kind property, in a similar way to how we sort SelectionSet selections: first by kind, then name.value. This would place FragmentNodes above DocumentNodes consistently.

I believe the only Node without a name at that level should be an OperationNode in which it's optional, except for when there are multiple OperationNodes.

I tried to quickly piece together what I thought the sorting would be like in the most complex situation, and I think it'd be:

DirectiveDefinition
EnumTypeDefinitionNode
EnumTypeExtensionNode
ExecutableDefinitionNode
FragmentDefinitionNode
InputObjectTypeDefinitionNode
InputObjectTypeExtensionNode
InterfaceTypeDefinitionNode
InterfaceTypeExtensionNode
ObjectTypeDefinitionNode
ObjectTypeExtensionNode
OperationDefinitionNode
ScalarTypeDefinitionNode
ScalarTypeExtensionNode
SchemaDefinitionNode
SchemaExtensionNode
UnionTypeDefinitionNode
UnionTypeExtensionNode

It's worth noting that since we're only concerned with executable operations, based on current server implementations in which the presence of most of those Node types would cause a validation error, I think the only sorting we'll end up with that matters is a fraction of those Nodes.

};

return visit(astWithSortedDefinitions, {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return visit(astWithSortedDefinitions, {
return visit(ast, {
Document(
node: DocumentNode
): DocumentNode {
return {
...node,
// Use sortBy because 'definitions' is not optional.
definitions: sortBy(node.definitions, 'kind', 'name.value'),
};
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is excellent. See: d341e08

I'd like your take on the iteratees that I've added.

  • operation will give a nice grouping of query mutation subscription. I don't see this adding any value to the normalization itself, but I imagine it will read much more nicely. Do we expect human eyes on these at any point? I don't know the answer to that.
  • The stringification for anonymous operations - after reading your explanation above about single anonymous operations only, considering this edge case may be overkill.

OperationDefinition(
node: OperationDefinitionNode,
): OperationDefinitionNode {
Expand Down