Add locations to composition errors #686

mayakoneval · 2021-04-22T22:25:56Z

This PR

adds location information to composition errors that are specific to federation. This includes pre, post and composition errors.
There are a couple things that needed to be in place for location information to be exposed properly. The gql function strips loc information from the type definitions that it returns, so all tests that I edited use parse instead of gql.
Composition errors use a function errorWithCode which calls the GraphQLError constructor. If node is provided to the GraphQLError constructor with a loc field on node with a source on loc, the location information added to an error is the same as the location information on node. This took me a while to get :)
This PR is organized by error. Each commit is a composition error code as defined in the federation docs.
The first commit moves a couple tests from composeAndValidate into the describe('error because that seemed more fitting 🤷 I would be fine deleting this one if anyone disagrees.
I updated all the error code specific tests that I could find, as well as adding locations to composeAndValidate.test.ts and compose.test.ts

I recommend reviewing by commit

Please let me know of any concerns switching gql to parse.

Questions

Should I change all gql calls to parse calls in the test files I edited? I only changed the tests that checked the snapshot of the whole error, rather than the length of errors, since those are the ones that would check for location.

2. make sure all test's use parse() instead of gql() so that the loc fields are populated on each type definition

…ocation & update tests

…cation & update tests

… location & update tests

…ations & update test

…hCode to get location & update test

…& update test

…t locations & add test

…ons & update test

…date tests

…ate tests

…get locations & update tests

…s & update test

…o get locations & update test

…ations & update tests

…de to get locations & update tests

…call to get locations & update tests

… get locations & update test

…s & update test

…Code to get locations & update test

…o get locations & update test

…e to get locations & update test

…ode to get locations & update test

…ode to get locations & update tests

…Code to get the location & update tests need to figure out why the tests don't print the errors as I expect

…& update tests

… errorWithCode to get locations & update tests

…ph nodes = locations for both subgraphs in location. updated tests

…hCodes = locations correspond to both services. updated tests

…an the enum

…ldDefinitionNodes

federation-js/src/composition/validate/sdl/__tests__/uniqueTypeNamesWithFields.test.ts

federation-js/src/composition/validate/postComposition/externalUnused.ts

federation-js/src/composition/validate/postComposition/keyFieldsSelectInvalidType.ts

trevor-scheer · 2021-05-06T18:52:21Z

federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts

+                  extendingServiceTypeNode && 'directives' in extendingServiceTypeNode ?
+                    extendingServiceTypeNode.directives?.find(directive =>
+                      directive.name.value === 'key')?.arguments?.find
+                        (argument => argument.name.value === 'fields') : undefined,


I think the nodes we want in this case are each of the @key directives on the extended type?

For this one, since we can't select the general "directive area" I am going to select the type node, so the error can be located to the whole error where there are too many @keys.

federation-js/src/composition/validate/postComposition/__tests__/providesNotOnEntity.test.ts

…tch, externalUsedOnBase

…pecific in the case where the error is about the number of key directives

mayakoneval · 2021-05-07T02:14:50Z

Okay I spent time today doing a full search through all the codes to make sure the locations make sense, which meant a few more changes than were requested. I'm hoping this looks pretty good 😋 . Excited about this! @trevor-scheer

federation-js/src/composition/validate/postComposition/__tests__/externalUnused.test.ts

federation-js/src/composition/validate/postComposition/externalMissingOnBase.ts

trevor-scheer · 2021-05-07T06:35:19Z

federation-js/src/composition/utils.ts

    | FieldDefinitionNode
-    | SchemaDefinitionNode
+    | FieldNode
+    | DefinitionNode


I like the direction you're taking this. I will suggest, I think we should either go full bore Maybe<ASTNode> as the type or only permit types which at least have a directives property on them. I like the latter, since the typings will warn users of the fn when they're using it inappropriately. In your case, the only inappropriate usage which is now included in this union is the DirectiveDefinitonNode, so I'll suggest this (+ a nice little TS revamp since we're here)

export function findDirectivesOnNode( node: Maybe< Exclude< | FieldDefinitionNode | InputValueDefinitionNode | FieldNode | DefinitionNode, DirectiveDefinitionNode > >, directiveName: string, ) { return ( node?.directives?.filter( (directive) => directive.name.value === directiveName, ) ?? [] ); }

Got it, I think I'll go with enumerating all the AST type with directives if that sounds good, rather than excluding from the ASTNode, since there aren't actually too many Types with directives.

trevor-scheer · 2021-05-07T06:36:48Z

federation-js/src/composition/utils.ts

+export function findSelectionSetOnNode(
+  node: Maybe<
+    | FieldDefinitionNode
+    | DefinitionNode


The same Exclude<> type I suggested above would give you a similar ergonomic win here.

trevor-scheer · 2021-05-07T06:46:59Z

federation-js/src/composition/utils.ts

    ? node.directives.filter(
        directive => directive.name.value === directiveName,
      )
    : [];
 }

+export function findSelectionSetOnNode(


This fn is a bit tough to grok. Can you please add a good description to what this fn looks for and how?

trevor-scheer · 2021-05-07T06:51:19Z

federation-js/src/composition/utils.ts

+  return node && 'directives' in node && node.directives
+    ? node.directives.find(
+        directive =>
+          directive.name.value === directiveName && directive.arguments?.some(
+            argument => argument.value.kind === "StringValue" &&
+              argument.value.value.includes(elementInSelectionSet)
+          ))?.arguments?.find(
+            argument => argument.name.value === 'fields')?.value
+    : undefined;


we should use Kind.STRING for "StringValue" (or use the isStringValueNode predicate fn!)

I'm not totally sure I understand this correctly but I think I do. Is the includes here possibly error-prone?

I'm imagining 2 keys:
@key(fields: "id")
@key(fields: "sku { id }")

Could this scenario break this implementation? I might be mistaken and just need to understand this a bit better.

In the scenario you described:

type SomeType @key(fields: "id") @key(fields: "sku { id }") { and we are looking for the field with id on it, yeah this would break. If this were the case, and we wanted to find a top level field, would it be sufficient to make sure that string is not wrapped in curly braces? I'll write a test for this

The resolution here was to search for the entire selection set, so we know we are selecting the correct key.

…single field. Update test name to be plural & write definition for selection set search

trevor-scheer · 2021-05-10T17:41:03Z

federation-js/src/composition/utils.ts

-            argument => argument.value.kind === "StringValue" &&
-              argument.value.value.includes(elementInSelectionSet)
+            argument => isStringValueNode(argument.value) &&
+              argument.value.value.includes(printedSelectionSet)


Can this be === now instead of includes?

trevor-scheer

Ship it!

Can we also replace the printFieldSet fn in printSupergraphSdl with the one we've moved into the utils file?

mayakoneval · 2021-05-10T17:45:34Z

@trevor-scheer just pushed those last changes, I think you have to merge :) Woohoo!

…SelectionSetOnNode

mayakoneval · 2021-05-10T17:54:49Z

cargo test on the rust windows test has failed twice, I force pushed again to try.

trevor-scheer · 2021-05-10T17:59:18Z

@mayakoneval this is probably not a you problem - I'll take a look at the errors in a bit. You might also try merging in main to make sure your branch is fully up to date.

trevor-scheer · 2021-05-10T19:58:36Z

@mayakoneval I merged main which must've introduced some good TS changes since I then had to remove a couple unused imports. Now everything is passing, so I'm going to land this now 🎉

mayakoneval added 24 commits April 22, 2021 10:50

1. move all error tests into describe('error'...,

da811fd

2. make sure all test's use parse() instead of gql() so that the loc fields are populated on each type definition

EXTENSION_OF_WRONG_KIND pass extension node to errorWithCode to get l…

0e9cb22

…ocation & update tests

EXTENSION_WITH_NO_BASE pass extension node to errorWithCode to get lo…

5b68ef1

…cation & update tests

KEY_FIELDS_SELECT_INVALID_TYPE add field node to errorWithCode to get…

cbb8296

… location & update tests

KEY_FIELDS_MISSING_ON_BASE add field node on errorWithCode to get loc…

cd4971d

…ations & update test

KEY_FIELDS_MISSING_EXTERNAL add field node or parent node on errorWit…

965b73a

…hCode to get location & update test

KEY_MISSING_ON_BASE add parent node to errorWithCode to get location …

321e8ca

…& update test

MULTIPLE_KEYS_ON_EXTENSION add parentType node to errorWithCode to ge…

594baa6

…t locations & add test

KEY_NOT_SPECIFIED add parent type node to errorWithCode to get locati…

5542820

…ons & update test

EXTERNAL_UNUSED add field node to errorWithCode to get locations & up…

c820bfe

…date tests

EXTERNAL_TYPE_MISMATCH add external field node to get locations & upd…

d702a35

…ate tests

EXTERNAL_MISSING_ON_BASE add external field node on errorWithCode to …

e7d11d9

…get locations & update tests

EXTERNAL_USED_ON_BASE add field node to errorWithCode to get location…

ed1d413

…s & update test

PROVIDES_FIELDS_MISSING_EXTERNAL add field astnode to errorWithCode t…

6cdff46

…o get locations & update test

PROVIDES_NOT_ON_ENTITY add field ast node on errorWithCode to get loc…

2003802

…ations & update tests

PROVIDES_FIELDS_SELECT_INVALID_TYPE add field ast node on errorWithCo…

a88b5e8

…de to get locations & update tests

REQUIRES_FIELDS_MISSING_EXTERNAL add field ast node to errorWithCode …

24cf587

…call to get locations & update tests

REQUIRES_FIELDS_MISSING_ON_BASE add field astnode to errorWithCode to…

f9ab2ca

… get locations & update test

REQUIRES_USED_ON_BASE add field node to errorWithCode to get location…

951dc97

…s & update test

EXECUTABLE_DIRECTIVES_IN_ALL_SERVICES add directive node to errorWith…

ae74390

…Code to get locations & update test

EXECUTABLE_DIRECTIVES_IDENTICAL add directive node to errorWithCode t…

0858b2f

…o get locations & update test

DUPLICATE_SCALAR_DEFINITION add duplicate scalar node to errorWithCod…

c77733f

…e to get locations & update test

DUPLICATE_ENUM_DEFINITION add duplicate enum definition to errorWithC…

47a4495

…ode to get locations & update test

DUPLICATE_ENUM_VALUE add enum type def / extension node to errorWithC…

4d4c862

…ode to get locations & update tests

mayakoneval requested a review from trevor-scheer April 22, 2021 22:26

mayakoneval added 5 commits April 22, 2021 16:05

ENUM_MISMATCH & ENUM_MISMATCH_TYPE - add enum type node to errorsWith…

45740f8

…Code to get the location & update tests need to figure out why the tests don't print the errors as I expect

RESERVED_FIELD_USED add field node to errorWithCode to get locations …

0b79f14

…& update tests

ROOT_QUERY_USED ROOT_MUTATION_USED ROOT_SUBSCRIPTION_USED add node to…

1a6b1d7

… errorWithCode to get locations & update tests

VALUE_TYPE_FIELD_TYPE_MISMATCH nodes were already passed, both subgra…

b23a9f7

…ph nodes = locations for both subgraphs in location. updated tests

VALUE_TYPE_INPUT_VALUE_MISMATCH nodes were already passed to errorWit…

1cbfa5f

…hCodes = locations correspond to both services. updated tests

mayakoneval added 4 commits May 5, 2021 14:32

requiresUsedOnBase - more specific -> requires directive

c640914

duplicateEnumValue error location is on the duplicate value rather th…

8b638e8

…an the enum

uniqueTypeNamesWithFields points to return types

ae3e2ca

another comment

fcb15c4

mayakoneval force-pushed the maya/composition-errors-locations branch from bc8e27a to fcb15c4 Compare May 5, 2021 21:33

pass accurate nodes & update the error message to match for uniqueFie…

4a94b78

…ldDefinitionNodes

mayakoneval force-pushed the maya/composition-errors-locations branch from b512dbc to 4a94b78 Compare May 5, 2021 22:01

trevor-scheer reviewed May 6, 2021

View reviewed changes

mayakoneval added 4 commits May 6, 2021 17:40

point to only the external directive on externalUnused, externalMisma…

8eda3db

…tch, externalUsedOnBase

point to the selection set on the @key errors

a79ae16

make the provides errors more specfic to the selection set, or less s…

0ac045d

…pecific in the case where the error is about the number of key directives

make requires errors more specific to the selection set

0e4302e

mayakoneval requested a review from trevor-scheer May 7, 2021 02:14

trevor-scheer reviewed May 7, 2021

View reviewed changes

search for selection set by whole printed selection set, rather than …

0b5a188

…single field. Update test name to be plural & write definition for selection set search

mayakoneval force-pushed the maya/composition-errors-locations branch from 0fc8d54 to 0b5a188 Compare May 7, 2021 19:19

mayakoneval requested a review from trevor-scheer May 7, 2021 21:26

trevor-scheer reviewed May 10, 2021

View reviewed changes

trevor-scheer approved these changes May 10, 2021

View reviewed changes

mayakoneval force-pushed the maya/composition-errors-locations branch from 01642f6 to 46bd00c Compare May 10, 2021 17:49

use printFieldSet from utils all over, change includes to === in find…

606a341

…SelectionSetOnNode

mayakoneval force-pushed the maya/composition-errors-locations branch from 46bd00c to 606a341 Compare May 10, 2021 17:54

trevor-scheer added 2 commits May 10, 2021 12:21

Merge branch 'main' into maya/composition-errors-locations

463c8f9

Remove unused imports

d34729e

trevor-scheer merged commit 3e225b5 into apollographql:main May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add locations to composition errors #686

Add locations to composition errors #686

mayakoneval commented Apr 22, 2021 •

edited

Loading

trevor-scheer May 6, 2021

mayakoneval May 7, 2021 •

edited

Loading

mayakoneval commented May 7, 2021 •

edited

Loading

trevor-scheer May 7, 2021

mayakoneval May 7, 2021 •

edited

Loading

trevor-scheer May 7, 2021

trevor-scheer May 7, 2021

trevor-scheer May 7, 2021 •

edited

Loading

mayakoneval May 7, 2021

mayakoneval May 7, 2021

trevor-scheer May 10, 2021

mayakoneval May 10, 2021

trevor-scheer left a comment

mayakoneval commented May 10, 2021

mayakoneval commented May 10, 2021

trevor-scheer commented May 10, 2021

trevor-scheer commented May 10, 2021

Add locations to composition errors #686

Add locations to composition errors #686

Conversation

mayakoneval commented Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

mayakoneval May 7, 2021 • edited Loading

Choose a reason for hiding this comment

mayakoneval commented May 7, 2021 • edited Loading

Choose a reason for hiding this comment

mayakoneval May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-scheer May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-scheer left a comment

Choose a reason for hiding this comment

mayakoneval commented May 10, 2021

mayakoneval commented May 10, 2021

trevor-scheer commented May 10, 2021

trevor-scheer commented May 10, 2021

mayakoneval commented Apr 22, 2021 •

edited

Loading

mayakoneval May 7, 2021 •

edited

Loading

mayakoneval commented May 7, 2021 •

edited

Loading

mayakoneval May 7, 2021 •

edited

Loading

trevor-scheer May 7, 2021 •

edited

Loading