n1ru4l / envelop

Envelop is a lightweight library allowing developers to easily develop, share, collaborate and extend their GraphQL execution layer. Envelop is the missing GraphQL plugin system.
https://envelop.dev
MIT License
792 stars 129 forks source link

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

Closed santino closed 6 months ago

santino commented 6 months ago

Issue workflow progress

Progress of the issue based on the Contributor Workflow


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:

EmrysMyrddin commented 6 months ago

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):

santino commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

santino commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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!