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

feat(UserInputError): handle unhandled errors. #5508

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

5achinJani
Copy link
Contributor

It handles the unhandled user input errors mentioned in #5353 and closes #5353

@glasser glasser added this to the MM-2021-07 milestone Jul 20, 2021
@glasser
Copy link
Member

glasser commented Jul 21, 2021

Oops, I did tell you to work on release-3.0 but we merged it since then! I think we keep old release branches open in order to make cherry-picks on them, though maybe that's not helpful. Let me see if I can rebase your branch on top of main.

required type & non-null errors handled.
@glasser glasser force-pushed the feat/user-input-errors branch from 2110b3f to 652e14a Compare July 21, 2021 23:00
@glasser glasser changed the base branch from release-3.0 to main July 21, 2021 23:00
@glasser
Copy link
Member

glasser commented Jul 21, 2021

OK, the rebasing worked.

FWIW there's a related issue in graphql-js (graphql/graphql-js#3169) and this PR graphql/graphql-js#3193 would let us implement this in a less hacky way. But we'd need this code anyway for back-compat with current graphql-js releases.

- Update comment to mention new checks and link to graphql-js issue
- Reduce repetition in the logic (hopefully this increases rather than
  decreases clarity?)
- prettier (required on main after rebasing)
- CHANGELOG
@glasser
Copy link
Member

glasser commented Jul 21, 2021

I tweaked it a bit (eliminating some repeated code, which hopefully improves readability rather than making it worse, and some comments etc). Thanks!

@glasser glasser enabled auto-merge (squash) July 21, 2021 23:13
@glasser glasser added the size/small Estimated to take LESS THAN A DAY label Jul 21, 2021
@glasser glasser disabled auto-merge July 21, 2021 23:27
@glasser glasser merged commit 12f46fc into apollographql:main Jul 21, 2021
@hwillson hwillson removed this from the MM-2021-07 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/small Estimated to take LESS THAN A DAY
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some variable-related errors should be BAD_USER_INPUT rather than INTERNAL_SERVER_ERROR
3 participants