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

Ensure POST request when sending full query with persisted queries link. #7456

Merged
merged 5 commits into from Feb 9, 2021

Conversation

rieset
Copy link
Contributor

@rieset rieset commented Dec 11, 2020

The second request after error with full graphql queries should be a POST request. Because there may be more than the limit for a GET request

If using automatic persisted queries, when first request did not find cache (return "PersistedQueryNotFound"). Then second request goes through GET method and if request is big, it falls on limit. The second request must use the POST method

@apollo-cla
Copy link

@rieset: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@benjamn benjamn self-assigned this Dec 11, 2020
@benjamn
Copy link
Member

benjamn commented Dec 11, 2020

@rieset I think your analysis is right, but in the process of checking whether operation.setContext({ method: "POST" }) will really work (instead of, say, operation.setContext({ options: { method: "POST" }})), I'm realizing that the TypeScript types for these objects are embarrassingly under-specified (way too many anys). I would ask you to write a quick regression test, but I think I need to improve the types first, before you/I write any more code that interacts with them.

@rieset
Copy link
Contributor Author

rieset commented Dec 13, 2020

Before changed:

Dev Tools:
https://drive.google.com/file/d/11_0K-FWVGTih3ViqU-KHKV1-WP8C8f4J/view?usp=sharing
Server log:
https://drive.google.com/file/d/1OTR6kSpX4rH_sj1uErrVgT_pVEUVdqP-/view?usp=sharing


Сhange:
https://drive.google.com/file/d/1NnPccVGKTi1MbcrOBiCsuHo46AQNVMxm/view?usp=sharing


After:

Dev tools:
https://drive.google.com/file/d/1AiOJAsvDZOvBdrgISt_kIuv6bTHzcRBa/view?usp=sharing

Server log:
https://drive.google.com/file/d/1XpJEeNHZqr_g3q8Mqx0rA4V9PrfgWZrQ/view?usp=sharing


I have tested this change and it seems to me to be the most appropriate and simplest. But perhaps there are other options.

Unfortunately, I don't have time to write tests for this case at the moment. Is this necessary?

@benjamn benjamn changed the base branch from main to release-3.4 February 9, 2021 20:02
@benjamn benjamn added this to the Release 3.4 milestone Feb 9, 2021
@benjamn benjamn changed the title Bug with second request on automatic persisted queries Ensure POST request when sending full query with persisted queries link. Feb 9, 2021
@benjamn benjamn merged commit 7bcc9d2 into apollographql:release-3.4 Feb 9, 2021
@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants