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

Separate validation of operation and coercion of variables into separate steps #3658

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Separate validation of operation and coercion of variables into separate steps #3658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dbd1118
subscribe: fix missing path on unknown field error
IvanGoncharov Jun 17, 2022
a1b86ac
step 1
IvanGoncharov Jun 17, 2022
974d92d
step 2
IvanGoncharov Jun 17, 2022
f5c9d66
step3
IvanGoncharov Jun 18, 2022
086a4f7
temp
IvanGoncharov Jun 21, 2022
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
3 changes: 0 additions & 3 deletions src/execution/__tests__/executor-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,6 @@ describe('Execute: Handles basic execution tasks', () => {
expectJSON(
executeSync({ schema, document, operationName: 'Q' }),
).toDeepEqual({
data: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

this change fits your proposed simplified criteria here: graphql/graphql-spec#894 (comment) but I am not sure about it

errors: [
{
message: 'Schema is not configured to execute query operation.',
Expand All @@ -883,7 +882,6 @@ describe('Execute: Handles basic execution tasks', () => {
expectJSON(
executeSync({ schema, document, operationName: 'M' }),
).toDeepEqual({
data: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

errors: [
{
message: 'Schema is not configured to execute mutation operation.',
Expand All @@ -895,7 +893,6 @@ describe('Execute: Handles basic execution tasks', () => {
expectJSON(
executeSync({ schema, document, operationName: 'S' }),
).toDeepEqual({
data: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

errors: [
{
message:
Expand Down
1 change: 1 addition & 0 deletions src/execution/__tests__/subscribe-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ describe('Subscription Initialization Phase', () => {
{
message: 'The subscription field "unknownField" is not defined.',
locations: [{ line: 1, column: 16 }],
path: ['unknownField'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this one. There is not an exact corollary to queries/mutations, because unknown fields there are silently dropped rather than raising an error.

The spec at https://spec.graphql.org/draft/#sel-HAPHRPJABABmE-1Y says:

This field should be a list of path segments starting at the root of the response and ending with the field associated with the error. Path segments that represent fields should be strings, and path segments that represent list indices should be 0-indexed integers. If the error happens in an aliased field, the path to the error should use the aliased name, since it represents a path in the response, not in the request.

The last sentence seems odd to me as the paths in the response and the request are the same -- perhaps it should be interpreted that the path is the path in the response, not in the schema. That might argue as you have here that a field not in the schema can have a path. Although, I am not sure about this, as perforce if the field is not in the schema, it cannot be in the response. For subscriptions, of course, there is no path in the response of CreateSourceEventStream, as the response for that section of the algorithm is not a map, it's an AsyncIterable. So, overall, I think we are better off without the path, although it seems to me that it could go either way.

},
],
});
Expand Down
Loading