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

[useResponseCache] In Multipart Response queries (using defer) only the last part sent is cached #2228

Closed
4 tasks
santino opened this issue May 7, 2024 · 9 comments
Labels
kind/docs Improvements or additions to documentation

Comments

@santino
Copy link
Contributor

santino commented May 7, 2024

Issue workflow progress

Progress of the issue based on the
Contributor Workflow

  • 1. The issue provides a
    minimal reproduction available on
    Stackblitz.
    • Please install the latest @envelop/* packages that you are using.
    • Please make sure the reproduction is as small as possible.
  • 2. A failing test has been provided
  • 3. A local solution has been provided
  • 4. A pull request is pending review

Describe the bug

Hello folks, been some time, hope you're all well.
I reporting an issue I discovered with Envelop response cache plugin.

When I have defer queries, using multipart, it caches the last part sent over the entire query.
This is quite problematic because it will serve only that part to anyone making the same queries.
Since the response if very incomplete, it causes application errors.

Would be great to cache the combined response in these cases. Or the minimum requirement to bypass this issue is disabling cache for these specific queries.
I looked into shouldCacheResult but it doesn't have any useful param to disable caching for multipart or defer queries.

Do you have any advice?

Environment:

  • OS: macOS Sonoma 14.3.1
  • NodeJS: v20.12.2
  • @envelop/* versions:
    • @envelop/core: 5.0.0
    • @envelop/response-cache: 6.1.2
@EmrysMyrddin
Copy link
Collaborator

Hi! Thank you for reaching out!
Can you provide a reproduction case ? Because we actually have a some unit tests verifying that cache is working well if async results (using @defer and @stream):

@EmrysMyrddin EmrysMyrddin added kind/bug Something isn't working stage/0-issue-prerequisites Needs more information before we can start working on it labels May 7, 2024
@santino
Copy link
Contributor Author

santino commented May 7, 2024

Thanks for the quick response, very much appreciated.
I had a look at the test you have in place for @defer
My gut feeling is that the difference might be due to executor behaviour and server request/response lifecycle.
In this case it would not be trivial to provide a reproduction case as I would need to setup an entire server and some example queries involving a few reused fragments to make things more realistic.

The pattern of requesting retrieving data is very simple.
This is the request I send (makes use of persisted documents):

curl 'http://localhost:3045/graphql' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: application/json' \
  --data-raw '{"variables":null,"docId":"4009b69c7fc6361e47245db6f572ca0d"}'

In my server on the first request (not cached) I am returning a multipart response that looks like this

---
Content-Type: application/json; charset=utf-8
Content-Length: 1003

{"data":{"__responseCacheTypeName":"Query","user":{"__responseCacheTypeName":"user","__responseCacheId":"dXNlcjpjMDMyMGQzNy1hYWNlLTRlMzItYjU1NC0xY2I3MGYzNDdkNzc=","id":"dXNlcjpjMDMyMGQzNy1hYWNlLTRlMzItYjU1NC0xY2I3MGYzNDdkNzc=","fullName":"Mickey Mouse","username":"mickey","country":{"__responseCacheTypeName":"country","__responseCacheId":"USA","code":"USA","localName":"United States"}}},"hasNext":true}

---
---
Content-Type: application/json; charset=utf-8
Content-Length: 16935

{"data":{"countries":[{"code":"ITA","defaultName":"Italy","localName":"Italia"},{"code":"USA","defaultName":"United States","localName":"United States"}]},"extensions":{"responseCache":{"hit":false,"didCache":true,"ttl":120000}}}
---

Then I make a second request identical to the previous one, and I get the following cached response

{
  "data": {
    "countries": [
      { "code": "ITA", "defaultName": "Italy", "localName": "Italia" },
      { "code": "USA", "defaultName": "United States", "localName": "United States" }
    ]
  },
  "extensions": {
    "responseCache": {
      "hit": true
    }
  }
}

As you can see the cached response takes into account only the countries field and not the user field that was sent in the first part of the response.

For completion, this is the prettified document stored within persisted documents

query AccountSettingsPage_Query {
  ...AccountSettingsFormOrganism_User_Query
  }
  
  fragment AccountSettingsFormOrganism_Countries_Query on Query {
    countries {
      code
      defaultName
      localName
    }
  }
  
  fragment AccountSettingsFormOrganism_User_Query on Query {
    user {
      id
      fullName
      username
      country {
        code
        localName
      }
    }
    ...AccountSettingsFormOrganism_Countries_Query @defer(label: "AccountSettingsFormOrganism_User_Query$defer$AccountSettingsFormOrganism_Countries_Defer")
  }

@santino
Copy link
Contributor Author

santino commented May 9, 2024

I think the source of the problem is here
https://github.com/n1ru4l/envelop/blob/main/packages/plugins/response-cache/src/plugin.ts#L607

My server does never attach the incremental property to the payload result.
Is this a standard that I am somehow missing with the GraphQL executors (Helix) I am using?

I appreciate the effort in making all the requests cacheable, but maybe you can consider adding a flag to skip async/iterable responses from the cache.
At least that would get me out of the application errors I am facing in no time.

As a long term solution, I will probably need to invest some time to replace my Helix implementation with Yoga; but this is dangerous and requires time, carefulness and tests so is not something I can consider in urgency.

@ardatan
Copy link
Collaborator

ardatan commented May 9, 2024

Are you using GraphQL Helix with graphql-js's defer stream version?

@santino
Copy link
Contributor Author

santino commented May 9, 2024

Currently, I am using Helix with graphql-executor.

I have started also looking into Yoga but having issues with incremental as this is not expected by my Relay fetcher function which uses fetch-multipart-graphql.
None of these seems to be aware of / understand incremental
Also facing some issues with subscriptions over my Yoga implementation, which I am looking at.

@EmrysMyrddin
Copy link
Collaborator

Hi! can you share more about your setup ? Which version of helix are you using ? With which clients ?
If you can share your integration code between envelop and Helix, or provide a reproduction code ?

@santino
Copy link
Contributor Author

santino commented May 20, 2024

Rather than spending more time and diggining more into the source of the issue I decided to spend the time to migrate to Yoga.
In Yoga the incremental property is handled directly and returned within the response.
Then I just had to update my Relay Network Layer to loop through this property and push it to the sink.

This is the function I use with Relay, in case in can be useful to others

const fetchOrSubscribe = (document, variables) =>
  Observable.create(sink =>
    subscriptionsClient.subscribe(
      {
        variables,
        operationName: document.name,
        query: document.text
      },
      {
        next: data =>
          data?.incremental
            ? data?.incremental.forEach(part => sink.next(part))
            : sink.next(data),
        error: error => sink.error(error),
        complete: () => sink.complete()
      }
    )
  )

I attach it to Relay Environment like this:

const relayEnvironment = new Environment({
  network: Network.create(fetchOrSubscribe, fetchOrSubscribe),
  store: new Store(new RecordSource(), {})
})

Thanks for your support. I believe the issue can be closed.

@EmrysMyrddin
Copy link
Collaborator

Great news! I hope the migration was not too dificult :-) If you have some insight about it, perhaps we can even add a migration guide to the documentation.

Thank you for the Relay boilerplate snippet! Very useful!
I will keep this open so that we add this to the documentation :-)

@EmrysMyrddin EmrysMyrddin added kind/docs Improvements or additions to documentation and removed kind/bug Something isn't working stage/0-issue-prerequisites Needs more information before we can start working on it labels May 28, 2024
@EmrysMyrddin
Copy link
Collaborator

Closing this in favor of #2244 :-) Since this documentation update is actually not related that much to the initial issue.

Thank you again for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/docs Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants