Editorial: Error Terminology #966

leebyron · 2022-06-21T20:40:29Z

Uses the definition syntax to set definitions for "field error" and "request error", and uses italic references in every formal normative location as well as the first reference in each prose.

This also clarifies when we previously used a plural "field errors" that we actually mean "a list of field error"

Include links to other sections as set up in #957

netlify · 2022-06-21T20:40:35Z

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	`69cd302`
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/62b4f62afabe2e00089a88a8
😎 Deploy Preview	https://deploy-preview-966--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Uses the definition syntax to set definitions for "field error" and "request error", and uses italic references in every formal normative location as well as the first reference in each prose. This also clarifies when we previously used a plural "field errors" that we actually mean "a list of field error" Include links to other sections as set up in #957 Co-authored-by: Roman Ivantsov <[email protected]>

benjie

Generally great improvement, section 7 needs perhaps a little extra attention. Also, see related: #967

spec/Section 6 -- Execution.md

spec/Section 7 -- Response.md

benjie · 2022-06-22T07:37:49Z

spec/Section 7 -- Response.md

-should contain those errors.
+{null}), the `errors` entry in the response may contain a list of all _field
+error_ that were raised during execution. If any _field error_ was raised during
+execution, it should contain the `errors` entry.


Because we've gone from plural to singular, the "it" has become ambiguous (it could be referring to the "field error"); so let's clarify:

Suggested change

execution, it should contain the `errors` entry.

execution, the request should contain the `errors` entry.

I believe the second sentence (If any field error was raised... it SHOULD (?) contain ...) is a repetition of the first sentence (the errors entry in the response MAY(?) contain a list...) - basically saying 'errors' contains errors raised.
At the same time, there is slight contradiction - MAY vs SHOULD

I suggest to remove the second sentence and change MAY -> SHOULD in the first.

The may is because the errors field is not required to be present (e.g. it will not be present in the case of no errors). The should is to indicate that the field should be present if there are errors. But I agree, the existing wording could be improved; something like:

the response may include an errors property containing a list of all field error that were raised during execution.

Lots of confusing interchanging of must/should - let me try to reword this section further to clarify

benjie · 2022-06-22T07:53:02Z

spec/Section 7 -- Response.md

+:: A _request error_ is an error raised during a _request_ but before execution
+begins. This may occur due to a parse grammar or validation error in the
+_Document_, an inability to determine which operation to execute, or invalid
+input values for variables.


Note CreateSourceEventStream() can also raise a request error:

If groupedFieldSet does not have exactly one entry, raise a request error.

-- https://spec.graphql.org/draft/#CreateSourceEventStream()

There's also various places where there are Assert: that could raise an error (though if you've done validation they should not occur, except when we don't handle that well).

The phrase "before execution begins" is quite a fuzzy definition it turns out; here's a comment where I try and pick it apart a bit: graphql/graphql-js#3640 (comment)

My current opinion is that "execution begins"

for queries: when you enter ExecuteQuery()

for mutations: when you enter ExecuteMutation()

for subscriptions: when you enter MapSourceToResponseEvent()

Time boundary isn't the best way to describe this. Maybe prose about this being "typical" could be helpful, but really the difference is that field errors go alongside partial results where request errors do not

That's kind of circular reasoning though; for example if an error is raised from the subscribe() resolver of a field (i.e. in step 10 of CreateSourceEventStream() via the ResolveFieldEventStream() algorithm) then it's not clear from the spec whether that should be a request error (since it prevents setting up the subscription, so the request cannot continue) or a field error (since it originated from a field). And thus, it's not clear if it should resolve to {data: null, errors: [...]} or just {errors: [...]} - and thus not clear if it's a partial result or not.

In my opinion, the boundary is effectively that it's a request error if it happens before the first ExecuteSelectionSet(); the boundaries I defined above give this rule of thumb explicit positions in the algorithm that are passed once only for each request.

In my opinion, the boundary is effectively that it's a request error if it happens before the first ExecuteSelectionSet()

That does seem to be currently true. Feels too in the weeds to be the written definition.

I applied an update, curious if you find it clearer

spec/Section 7 -- Response.md

leebyron · 2022-06-22T17:16:51Z

Thanks for the detailed read! I'll incorporate

benjie

Really good improvements! I have a couple minor suggestions:

spec/Section 7 -- Response.md

Co-authored-by: Benjie Gillam <[email protected]>

leebyron · 2022-06-23T18:01:02Z

Merged your suggestions @benjie

benjie

Really good improvements, thanks @leebyron 🙌

yaacovCR · 2022-06-23T19:51:46Z

spec/Section 6 -- Execution.md

@@ -154,7 +155,8 @@ ExecuteMutation(mutation, schema, variableValues, initialValue):
 - Let {selectionSet} be the top level Selection Set in {mutation}.
 - Let {data} be the result of running {ExecuteSelectionSet(selectionSet,
  mutationType, initialValue, variableValues)} _serially_.
- Let {errors} be any _field errors_ produced while executing the selection set.
+- Let {errors} be the list of all _field error_ raised while executing the


field errors plural?

yaacovCR · 2022-06-23T19:52:33Z

spec/Section 6 -- Execution.md

@@ -302,7 +304,8 @@ ExecuteSubscriptionEvent(subscription, schema, variableValues, initialValue):
 - Let {data} be the result of running {ExecuteSelectionSet(selectionSet,
  subscriptionType, initialValue, variableValues)} _normally_ (allowing
  parallelization).
- Let {errors} be any _field errors_ produced while executing the selection set.
+- Let {errors} be the list of all _field error_ raised while executing the


For this to be a glossary referenced term it need to remain in the singular, hence "list of field error" vs "field errors". It's a little grammatically awkward, but hopefully palatable

spec/Section 7 -- Response.md

leebyron requested a review from benjie June 21, 2022 20:42

leebyron force-pushed the editorial-error-terminology branch from 8323576 to f6dbee8 Compare June 21, 2022 20:45

leebyron force-pushed the editorial-error-terminology branch from f6dbee8 to eee64e9 Compare June 22, 2022 00:16

benjie mentioned this pull request Jun 22, 2022

Fix normative requirement around errors property #967

Closed

benjie requested changes Jun 22, 2022

View reviewed changes

leebyron requested a review from benjie June 22, 2022 20:23

leebyron force-pushed the editorial-error-terminology branch from b6b74c2 to 054e024 Compare June 22, 2022 20:29

Review feedback plus clarification of error definition and should/must

9e70c72

leebyron force-pushed the editorial-error-terminology branch from 054e024 to 9e70c72 Compare June 22, 2022 20:30

benjie requested changes Jun 23, 2022

View reviewed changes

spec/Section 7 -- Response.md Outdated Show resolved Hide resolved

spec/Section 7 -- Response.md Outdated Show resolved Hide resolved

Review feedback

e503ac4

Co-authored-by: Benjie Gillam <[email protected]>

leebyron requested a review from benjie June 23, 2022 17:59

benjie approved these changes Jun 23, 2022

View reviewed changes

yaacovCR reviewed Jun 23, 2022

View reviewed changes

spec/Section 7 -- Response.md Outdated Show resolved Hide resolved

Fix one resolvable pluralization

69cd302

leebyron merged commit 3885a64 into main Jun 23, 2022

leebyron deleted the editorial-error-terminology branch June 23, 2022 23:27

benjie mentioned this pull request Feb 21, 2025

Add "cut a new spec release" to next WG graphql/graphql-wg#1666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: Error Terminology #966

Editorial: Error Terminology #966

leebyron commented Jun 21, 2022 •

edited

Loading

netlify bot commented Jun 21, 2022 •

edited

Loading

benjie left a comment

benjie Jun 22, 2022

rivantsov Jun 22, 2022

rivantsov Jun 22, 2022

benjie Jun 22, 2022

leebyron Jun 22, 2022

benjie Jun 22, 2022

leebyron Jun 22, 2022

benjie Jun 22, 2022

leebyron Jun 22, 2022

leebyron commented Jun 22, 2022

benjie left a comment

leebyron commented Jun 23, 2022

benjie left a comment

yaacovCR Jun 23, 2022

yaacovCR Jun 23, 2022

leebyron Jun 23, 2022 •

edited

Loading

	execution, it should contain the `errors` entry.
	execution, the request should contain the `errors` entry.

Editorial: Error Terminology #966

Editorial: Error Terminology #966

Conversation

leebyron commented Jun 21, 2022 • edited Loading

netlify bot commented Jun 21, 2022 • edited Loading

✅ Deploy Preview for graphql-spec-draft ready!

benjie left a 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

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

Choose a reason for hiding this comment

leebyron commented Jun 22, 2022

benjie left a comment

Choose a reason for hiding this comment

leebyron commented Jun 23, 2022

benjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

leebyron commented Jun 21, 2022 •

edited

Loading

netlify bot commented Jun 21, 2022 •

edited

Loading

leebyron Jun 23, 2022 •

edited

Loading