useAPQ errors indefinitely if the cache is down

[jdolle]

Describe the bug

If store.get throws an exception, then that error will be raised to clients which can cause clients to retry indefinitely (depending on client retry logic)

Your Example Website or App

https://github.com/dotansimha/graphql-yoga/blob/main/packages/plugins/apq/src/index.ts#L91

Steps to Reproduce the Bug or Issue

call useAPQ with a cache implementation that throws a new Error() on get and set.

Expected behavior

throw a PersistedQueryNotSupported to indicate to the client that this feature is disabled for now
https://www.apollographql.com/docs/react/api/link/persisted-queries/#protocol

This should allow clients to intelligently fallback to sending the full operation body.

Screenshots or Videos

No response

Platform

• OS: [e.g. macOS, Windows, Linux]
• NodeJS: [e.g. 18.1.0]
• @graphql-yoga/* version(s): [e.g. 2.5.1]

Additional context

No response

[EmrysMyrddin]

Hi! Thank you for raising this concern!

While I do agree that bubbling up the error as is to the client is not a good thing, I'm not sure throwing a PersistedQueryNotSupported is the way to go.

I think that what we should to is actually handling errors in place, and throw the appropriate error, which are PersistedQueryNotFound or PersistedQueryMismatch. Because the APQ is not actually disabled, it's potentially just this particular request that have a problem.

@ardatan What do you think ?

[jdolle]

I'm also not 100% sure if PersistedQueryNotSupported is the best solution, but I can provide some context on why that's what I opted to do and why I recommended the same in this issue:

PersistedQueryMismatch behavior isn't well documented. I can't find reference anywhere in Apollo's client implementations and it seems disingenuous because the hash isnt wrong -- it can't be found at all.

PersistedQueryNotFound is a more accurate error code, but it causes clients to "[send] both hash and query string to Server" (Source).
If the cache is down (e.g. redis server cant connect), then that will error again, presumably causing the same PersistedQueryNotFound to be thrown and then it loops indefinitely until (hopefully) clients hit a retry limit that isn't defined anywhere by the "spec".

PersistedQueryNotSupported is referenced in the code for the apollo clients, and would cause clients to give up on sending the APQ format, and fallback to sending the entire operation body, which is the desired behavior if the cache is down.

However, I agree that in cases where the cache has an intermittent error and isnt down, that this isnt the best outcome. Maybe circuitbreaking or having an error handler decide would be better. I don't know if it matters too much overall though. Always sending PersistedQueryNotSupported is much easier to implement and solves 99% of my concern, which again is just that if the cache is down, the app is effectively down.

[ardatan]

I think it can be handled inside the current API. Not sure if this is the responsibility of the plugin.
No need to throw an error since the client should send the original query if it is not found.

useAPQ({
   store: {
      get(key) {
           try{
             return someStorage.get(key);
           } catch(e) {
              console.error(`Error while fetching the operation: ${key}`, e);
              return null;
           }
      },
      set(key) {
           try{
             return someStorage.get(key);
           } catch(e) {
              console.error(`Error while saving the operation: ${key}`, e);
              return null;
           }
      }
   }
})

[jdolle]

returning null from the get will throw a PersistedQueryNotFound error inside the useAPQ onParams method, which will cause this loop I've mentioned.

I think the behavior is confusing enough that if not explicitly handled, it'd be worth including the recommended implementation in the documentation.

[ardatan]

When PersistedQueryNotFound is thrown, the client will send the original query and the hash, then the server attempts to save it. Even if it fails, it will do nothing then the server will return the response. I didn't understand what is the loop here. As a user, you can handle the error by yourself as in this test.
#3207

[jdolle]

Ty. That makes sense.
I forgot an await on the async set in my test, so the error was still being raised.
Capturing the error and returning null or undefined works.

[n1ru4l]

@jdolle Can you send a PR to document this practise in our documentation?

[EmrysMyrddin]

Fixed by #3213