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

fix: Update GraphQL to return correct HTTP response code for file size limit upload errors #8082

Open
wants to merge 3 commits into
base: alpha
Choose a base branch
from

Conversation

ardatan
Copy link

@ardatan ardatan commented Jun 28, 2022

New Pull Request Checklist

  • [X ] I am not disclosing a vulnerability.
  • I am creating this PR in reference to an issue.

Issue Description

Previously GraphQL Yoga was returning 500 but in the latest version it returns 400 which also not 100% correct.
It should be 413 which indicates that user sends a request over the specified limit per HTTP spec.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/413

dotansimha/graphql-yoga#1270
With the PR above on GraphQL Yoga repo, the behavior is fixed per spec. But Parse Server tests need to be updated since it was expecting the wrong status code (500) which indicates a misleading internal server error that isn't related to the user.

Related issue: #8043 (comment)

Approach

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Jun 28, 2022

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

@Moumouls
Copy link
Member

Hi @ardatan, parse-server still use npm lockfile v1. So to install the project use nvm use 14 && npm i to avoid a lockfile migration. Then you should have less diffs.

@ardatan
Copy link
Author

ardatan commented Jun 29, 2022

@Moumouls Sure I will try :)
Is there anything I can do so we can get CI running for this PR?

@ardatan ardatan marked this pull request as ready for review June 29, 2022 09:22
@ardatan ardatan changed the title fix: update Yoga and fix tests to expect correct HTTP status code for upload errors (DO NOT MERGE YET) fix: update Yoga and fix tests to expect correct HTTP status code for upload errors Jun 29, 2022
@ardatan ardatan changed the title (DO NOT MERGE YET) fix: update Yoga and fix tests to expect correct HTTP status code for upload errors fix: update Yoga and fix tests to expect correct HTTP status code for upload errors (DO NOT MERGE YET) Jun 29, 2022
@mtrezza mtrezza marked this pull request as draft June 30, 2022 09:35
@mtrezza mtrezza marked this pull request as draft June 30, 2022 09:35
@mtrezza mtrezza changed the title fix: update Yoga and fix tests to expect correct HTTP status code for upload errors (DO NOT MERGE YET) fix: update Yoga and fix tests to expect correct HTTP status code for upload errors Jun 30, 2022
@mtrezza
Copy link
Member

mtrezza commented Jun 30, 2022

@ardatan if a PR is in the works and not ready for review yet you can change its status to "draft"; I've started the CI

@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Patch coverage has no change and project coverage change: -6.91 ⚠️

Comparison is base (2ea4e37) 94.18% compared to head (f9049cf) 87.27%.

❗ Current head f9049cf differs from pull request most recent head c85de55. Consider uploading reports for the commit c85de55 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8082      +/-   ##
==========================================
- Coverage   94.18%   87.27%   -6.91%     
==========================================
  Files         182      182              
  Lines       13712    13712              
==========================================
- Hits        12914    11967     -947     
- Misses        798     1745     +947     

see 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ardatan ardatan force-pushed the update-yoga-fix-status-code branch 2 times, most recently from 7a03ef6 to b4ea255 Compare June 30, 2022 10:38
@ardatan
Copy link
Author

ardatan commented Jun 30, 2022

Could you run it again @mtrezza ?
Thanks!

@mtrezza
Copy link
Member

mtrezza commented Jun 30, 2022

Do you want to test anything specific in this CI that you cannot test locally by running npm test?

@ardatan ardatan force-pushed the update-yoga-fix-status-code branch from b4ea255 to e44efad Compare June 30, 2022 11:14
@ardatan ardatan marked this pull request as ready for review June 30, 2022 11:14
@ardatan
Copy link
Author

ardatan commented Jun 30, 2022

We released a new version now, I updated the PR and ran tests locally it passes.
Hopefully it will pass on CI as well. @mtrezza

@ardatan ardatan force-pushed the update-yoga-fix-status-code branch from 59dc218 to 051b7e6 Compare June 30, 2022 16:08
@ardatan
Copy link
Author

ardatan commented Jun 30, 2022

@mtrezza There was an issue with Node 12 support. So I updated the PR again.

@mtrezza
Copy link
Member

mtrezza commented Jun 30, 2022

Restarted CI

@Moumouls
Copy link
Member

Moumouls commented Jul 3, 2022

Hi @ardatan thanks, i think we have merge conflicts here, related to the package.lock.

Could you rebase your PR on alpha and then push again (or fix package.lock conflicts) then we should be okay 🚀

failing CI on node 17 is related to a flacky test on apple auth game center

@mtrezza
Copy link
Member

mtrezza commented Jul 25, 2022

I've resolved the conflicts, waiting for CI to pass

@mtrezza
Copy link
Member

mtrezza commented Jul 25, 2022

Not sure how to handle this change of which is essentially a 3rd party-managed API. This is a clear example of why #7979 is so important to move GraphQL into an adapter, out of the core of Parse Server.

On the other hand if the file size limit is an undocumented GraphQL feature so far (or "experimental"), there may not even be a formal issue merging this as a "fix". As @n1ru4l said: "From our perspective, the file upload limit was undocumented and treated as unstable."

Suggested changelog entry for this PR:

fix: update GraphQL to return correct HTTP response code for file size limit upload errors; the HTTP response code changes from 500 to 413; the file size limit mechanism in GraphQL is an undocumented feature, if you are using this feature and parsing the response code make sure to adapt your app code accordingly before upgrading Parse Server.

@n1ru4l please correct me if the "is an undocumented feature" does not accurately describe it

@mtrezza mtrezza changed the title fix: update Yoga and fix tests to expect correct HTTP status code for upload errors fix: update GraphQL to return correct HTTP response code for file size limit upload errors Jul 25, 2022
@mtrezza
Copy link
Member

mtrezza commented Oct 10, 2022

What is the state of this PR?

@mtrezza mtrezza closed this May 21, 2023
@mtrezza mtrezza reopened this May 21, 2023
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: update GraphQL to return correct HTTP response code for file size limit upload errors fix: Update GraphQL to return correct HTTP response code for file size limit upload errors May 21, 2023
@mtrezza
Copy link
Member

mtrezza commented May 21, 2023

@n1ru4l, @ardatan, @Moumouls Is this PR still needed? Or can we close it?

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

Successfully merging this pull request may close these issues.

None yet

3 participants