keystonejs / keystone

The superpowered headless CMS for Node.js — built with GraphQL and React
https://keystonejs.com
MIT License
9.12k stars 1.14k forks source link

File upload does not work in 'keystone-in-nextjs' example #8176

Closed marekryb closed 1 year ago

marekryb commented 1 year ago

Thank you for providing new example (https://github.com/keystonejs/keystone/pull/8148) and other changes leading into it. I tried embedded-nextjs example in the past, but abandoned it after realising that uploading files does not work and there is no easy way to replace generated 'withKeystone' with something else. This new way looks much brighter, but file upload functionality also does not work out-of-the-box.

Steps to Reproduce

  1. Add some image/file field to the schema. Eg. https://github.com/marekryb/keystone/commit/2aca0fedb5e085f67c9d3c078b8ac4c13a1e0a5e

  2. Run the keystone server in standalone mode (port 3000)

    yarn keystone:dev

  3. Run mutation that uploads a file NOTE: Standard playground does not support file uploads, so I am using curl. You can also use Altair GraphQL client or do it programatically.

    curl localhost:3000/api/graphql \ -F operations='{ "query": "mutation Test($id: ID!, $file: Upload!) {updateUser(where:{id:$id},data:{avatar:{upload:$file}}) { id } }", "variables": { "id": "{{USER_ID}}", "file": null } }' \ -F map='{ "0": ["variables.file"] }' \ -F 0=@{{FILE_NAME}}

Need to replace USER_ID and FILE_NAME in above. In my case final query is eg.

curl localhost:3000/api/graphql \ -F operations='{ "query": "mutation Test($id: ID!, $file: Upload!) {updateUser(where:{id:$id},data:{avatar:{upload:$file}}) { id } }", "variables": { "id": "clbafdgom00135y3upyadgilw", "file": null } }' \ -F map='{ "0": ["variables.file"] }' \ -F 0=@test.jpg

Result is as expected

{"data":{"updateUser":{"id":"clbafdgom00135y3upyadgilw"}}}

  1. Run nextjs with embedded keystone (port 4000)

    yarn next:dev

  2. Run same mutation as in point 3, but for port 4000

    curl localhost:4000/api/graphql \ -F operations='{ "query": "mutation Test($id: ID!, $file: Upload!) {updateUser(where:{id:$id},data:{avatar:{upload:$file}}) { id } }", "variables": { "id": "clbafdgom00135y3upyadgilw", "file": null } }' \ -F map='{ "0": ["variables.file"] }' \ -F 0=@test.jpg

  3. Observe error

    {"errors":[{"message":"Variable \"$file\" got invalid value {}; Upload value invalid.","locations":[{"line":1,"column":25}]}]}

Expected Behavior

File upload should also work in embedded mode.

Possible Solution

You can fix this problem by manually pre-processing requests with graphql-upload, before passing them to Yoga. See: https://github.com/marekryb/keystone/commit/bae9841622d237a2c9b40b248c34a66fd0a49447

junaid33 commented 1 year ago

I was about to open this same issue. The issue seems to be createUploadLink used in Apollo Client: https://github.com/keystonejs/keystone/blob/628f58872ab8b71249307639922e5a4796fc42d2/packages/core/src/admin-ui/context.tsx

GraphQL-Yoga has docs for Apollo Client: https://the-guild.dev/graphql/yoga-server/docs/features/subscriptions#client-usage-with-apollo

That says to use a new package:

import { YogaLink } from '@graphql-yoga/apollo-link'

const client = new ApolloClient({
  link: new YogaLink({
    endpoint: 'http://localhost:4000/graphql'
  }),
  cache: new InMemoryCache()
})

Instead of

link: createUploadLink({ uri: props.apiPath }),
flexdinesh commented 1 year ago

If I'm understanding this correctly the root cause is not with the nextjs-keystone example or keystone core. For file upload to work in GraphQL, both the GraphQL client and the GraphQL server need to have a compatible setup.

The next.js-keystone example doesn't have anything setup for file upload so adding adding the changes to the server setup @marekryb has suggested will make file upload work when uploading a file via the GraphQL API (from GraphiQL playground or from frontend built in the next.js app).

However Admin UI is built with apollo client. But starting Keystone in standalone mode will start Keystone's GraphQL server which is just an Apollo GraphQL server and file uploads should work just the same since the Admin UI uses Keystone's GraphQL API. What am I missing @junaid33? How does createUploadLink in Admin UI affect file uploads via the custom GraphQL server setup in a Next.js route?

flexdinesh commented 1 year ago

Would you like my changelist as PR to keystone-in-nextjs example?

@marekryb Absolutely. Please create a PR and tag me in that and we'll take it from there. One ask is to add enough comments in the code to make it obvious why the extra lines of code are needed.

junaid33 commented 1 year ago

@flexdinesh For openfront, I've managed to get the Admin UI working with Next.js. If you clone that repo and run locally, on localhost:3000 you'll see the Admin UI using the graphQL-yoga server. On localhost:8000 is the actual Keystone app. If you try uploading an image on localhost:3000, you'll get the error mentioned here. On localhost:8000, uploading an image works.

In short, if you want the Admin UI to work with graphql servers outside of just Apollo Server 2 out-of-the-box, I think we have to use YogaLink instead of createUploadLink. @marekryb fix with graphql-upload works for yoga, but people may have to configure multipart spec for different servers.

flexdinesh commented 1 year ago

Gotcha. Admin UI is built with apollo client and expects the GraphQL server to be apollo server. We can't change that now without breaking standalone keysone. However I think patch-package would be handy for your case @junaid33. You could replace createUploadLink in the built files and replace it with YogaLink. If you need code inspiration, we already use patch-package in the keystone monorepo for a few things.

marekryb commented 1 year ago

I dug a little deeper into this and this is what I found:

@junaid33 Could you test your setup with my fix? I do not think there is any problem with Admin UI using apollo-client. It is POSTing queries and mutations to GraphQL API same way as you would do in your code. 'Over the wire' data structures are same regardless of client used.

@flexdinesh PR at https://github.com/keystonejs/keystone/pull/8179

flexdinesh commented 1 year ago

Thanks @marekryb. I will take a look at it once I am back from holidays.

junaid33 commented 1 year ago

@marekryb Your fix worked perfectly, but is there a downside to checking the content-type on each server call? Also, the latest version of graphql-upload doesn't export processRequest. Will that be an issue going forward?

marekryb commented 1 year ago

is there a downside to checking the content-type on each server call?

No. It is done same way in standalone keystone and yoga. https://github.com/keystonejs/keystone/blob/main/packages/core/src/lib/server/createExpressServer.ts#L45 (using graphqlUploadExpress) https://github.com/jaydenseric/graphql-upload/blob/master/graphqlUploadExpress.mjs#L48

Also, the latest version of graphql-upload doesn't export processRequest. Will that be an issue going forward?

No. graphql-upload does not export anything by default, which create some drama (see Issues) in this repo ;-) https://github.com/jaydenseric/graphql-upload#exports