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.

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

Integrate missing required fields into errorResponseFields #4807

Closed
wants to merge 3 commits into from

Conversation

captbaritone
Copy link
Contributor

This represents the last major step in consolidating how field errors are modeled on Snapshot. Here we move missing required fields into the existing errorResponseFields (which should probably get renamed at some point) and are now directly stored in their event shape, rather than needing an intermediate shape.

This comes with a few small changes:

  1. The decision of if we should throw or not is now guided by the actual event rather than just the presence of @throwOnFieldError on the fragment being read. This is important because we may inherit events from resolvers that we read.
  2. We now throw individual errors based on the first field error we encounter rather than throwing a composite error with lots of metadata attached. In practice I think this is probably better. Many errors are likely uncommon, and having an informative error message is likely to result in a more actionable error report (error reporting infra won't easily pick up the metadata). Advanced users are free to use the FieldLogger to build more advanced errors if they wish
  3. We dropped the feature where cascading missing required field errors were not logged. We now log all required field errors (even if some are the result of previous errors) but only the first error will throw.

@facebook-github-bot
Copy link
Contributor

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@captbaritone merged this pull request in 535f35c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants