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

RestController - Use GET requests when possible #629

Closed
wants to merge 3 commits into from

Conversation

ridem
Copy link

@ridem ridem commented Aug 9, 2018

Note: I'll be happy to edit the tests impacted if it's considered for merging

This PR intends to use GET requests whenever possible instead of forcing POST requests.

While this probably makes very little difference for server-to-server requests, using GET requests allows for browser-caching and "CDN caching" when used in a web browser.
Right now, parse-server sends back ETag for POST requests, which is of no use for the web browser. Putting the API behind a service like Cloudfront isn't useful if all the requests made are POST requests, as they cannot be cached.

Incidentally, this makes Parse JS use its own REST API as per its specifications with regards to using the HTTP Headers (https://docs.parseplatform.org/rest/guide/)

We might want to actually properly map all the other HTTP methods properly as well. (DELETE, PUT)

@ridem ridem changed the title Restcontroller use get RestController - Use GET requests when possible Aug 9, 2018
@flovilmart
Copy link
Contributor

While I understand the motivation, I recall that originally, the SDK was written with POST request in order to avoid potential issues around passing headers to the server using GET requests.

@andrewimm may have more informations about this.

I won't consider it for merging now, as:

  1. There is no tests at all, integration or unit that ensures everything works fine.
  2. The SDK has been working forever with the current strategy, I am not sure why it would make sense to change it now

Please open an issue first so we ca try to get ahold of the original SDK authors so they can answer the underlying question.

@flovilmart flovilmart closed this Aug 9, 2018
@flovilmart
Copy link
Contributor

cc @drew-gross

@ridem
Copy link
Author

ridem commented Aug 9, 2018

Ok, thanks for your feedback! I opened an Issue as you suggested

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.

2 participants