jaydenseric / graphql-upload

Middleware and an Upload scalar to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
https://npm.im/graphql-upload
MIT License
1.43k stars 131 forks source link

Improve documentation for handling results of uploads in express/koa #252

Closed viveleroi closed 3 years ago

viveleroi commented 3 years ago

I didn't want to file an issue but am not sure where to get help.

I've followed the steps to get the express middleware working, I've followed tutorials for sending in the query from my client side, but I don't understand what to do in my express controller handling the actual file.

In my arguments, all I get is an object of { file: { path: 'filename.png' } }

The file doesn't exist in the os.tmpdir, I can't use createReadStream on it, etc.

It's possible I'm not sending in the data right, but I've structured my api query just like other tutorials did:

query: `
    mutation($file: Upload!) {
        server:createServer(
            input: {
                bannerFile: $file,
                name: "${values.serverName}",
                address: "${values.serverAddress}",
                description: "${values.serverDescription}"
            }) { id }
        }`,
variables: {
    file: values.bannerFile // this comes directly from the files from a file input. 
}
jaydenseric commented 3 years ago

If your GraphQL API is setup correctly with graphql-upload, you would get some sort of an error on the server side if the client was sending the wrong thing.

Double check you have followed every part of the setup instructions. If you are using Apollo Server, be aware you have to disable their outdated built-in file uploads implementation when you construct it (see numerous past issues and Apollo Server docs).

viveleroi commented 3 years ago

No errors, the only errors I get are when I try to do stuff with the args.input.file that aren't valid.

I've followed the setup instructions, I don't see what I might have missed. I setup my schema, adding scalar Upload and then Upload: GraphQLUpload to my root, added the middleware, etc.

viveleroi commented 3 years ago

Is this step required? It seems like it's implying if I used makeExecutableSchema I then need to do a different step but maybe this is what I misunderstand.

A schema built with separate SDL and resolvers (e.g. using makeExecutableSchema from @graphql-tools/schema) requires the Upload scalar to be setup

viveleroi commented 3 years ago

Here's the important bits of my api/gql stuff:

const { graphqlHTTP } = require('express-graphql');
const { buildSchema } = require('graphql');
const { createServer } = require('./api/servers');
const { GraphQLUpload, graphqlUploadExpress } = require('graphql-upload');

const schema = buildSchema(`
    scalar Upload
    type Server {
        id: ID!,
        // stuff...
    }
    input ServerInput {
        bannerFile: Upload,
        name: String!,
        address: String!,
        description: String!
    }
    type Mutation {
        createServer(input: ServerInput!): Server
    }
`);

// The root provides a resolver function for each API endpoint
const root = {
    createServer,
    Upload: GraphQLUpload
};

module.exports = function(app) {
    app.use('/api', graphqlUploadExpress({ maxFileSize: 1000000, maxFiles: 1 }), graphqlHTTP({
        schema: schema,
        rootValue: root,
        graphiql: true,
        customFormatErrorFn: err => ({ message: err.message })
    }));
};
jaydenseric commented 3 years ago

You're building your schema with a separate GraphQL SDL string and resolver code, then bringing the two together using buildSchema, so yes, you need to setup the Upload scalar (which at a glance looks correct the way you've done it). If you skipped that weird way of defining the schema and just made the schema directly using the graphql JS API you wouldn't need to setup the Upload scalar; you would just import and use it wherever you want that type in your schema like this:

https://github.com/jaydenseric/apollo-upload-examples/blob/4b8ab99a11bebcc03526299daa51886e5c4c23f4/api/schema/MutationType.mjs#L2

It's very unfortunate that Apollo and then a bunch of tutorials normalized defining a schema using a separate GraphQL SDL string and resolver code; it's slower, buggier, and way more abstract to learn. I highly recommend doing it using regular JS.

I think your root is incorrect (off the top of my head I think this is the way to do it, but please check):

  const root = {
-   createServer,
+   Mutation: {
+     createServer
+   },
    Upload: GraphQLUpload
  };
viveleroi commented 3 years ago

Assuming all that was done correctly what would I actually do in my createServer method to process the file? I think that’s where my biggest confusion is.

jaydenseric commented 3 years ago

Here are the docs for what the Upload scalar value is in resolvers, with a simple code example:

https://github.com/jaydenseric/graphql-upload#class-graphqlupload

Here is a detailed code example (linked in the graphql-upload readme BTW) of promisifying the file upload stream and storing the file:

https://github.com/jaydenseric/apollo-upload-examples/blob/4b8ab99a11bebcc03526299daa51886e5c4c23f4/api/storeUpload.mjs#L11

There are infinite ways you can use a file upload stream, so it's not really possible for me to list code examples for every conceivable way a file could be analyzed or stored in a mutation resolver.

viveleroi commented 3 years ago

I’ll need to keep trying I guess, because awaiting the file never works, destructuring doesn’t give anything, it’s just that file/path object and nothing more. I’m fairly sure your untested mutation change isn’t right, I have zero problem with my mutations being used/found and that’s a syntax I’m not familiar with.

viveleroi commented 3 years ago

I'm not having much luck. I re-read all of the few tutorials I could find, re-read our conversation, and am not sure what I've missed. I am not using apollo so I send requests manually from the client side, so maybe that's related? I can't find clear examples of how the requests are actually being sent because they're either using apollo or they've left that part out entirely and just show their payload.

I am getting no errors from graphql-upload. My apis have been working fine outside of this file upload issue.

Some tutorials use makeExecutableSchema over buildSchema but that doesn't appear to be required here, because some use buildSchema like I have been.

All three of those console.logs print undefined.

jaydenseric commented 3 years ago

First thing to check, is that the client is sending a valid GraphQL multipart request when it contains file uploads. Looking at the client side code you just shared, I can't see that happening.

A good reference for making appropriate fetch options based on the GraphQL operation is the function fetchOptionsGraphQL from graphql-react:

You can verify the client is sending the right type of request using the browser dev tools network inspector, paying attention to the request body.

I'm not sure why the GraphQL server is not producing a relevant error about the Upload scalar value, but lets solve one problem at a time.

viveleroi commented 3 years ago

Ok you're probably right. I'll look into that and report back. Appreciate your help and time.

viveleroi commented 3 years ago

Ok, I believe I've adapted my client-side logic to send the format as explained in the graphql-multipart-request-spec, but the graphql-http server responds with the json error {"errors":[{"message":"Must provide query string."}]}

img

const payload = {
    query: `
        mutation($file: Upload!) {
            server:createServer(
                input: {
                    bannerFile: $file,
                    name: "${values.serverName}",
                    address: "${values.serverAddress}",
                    description: "${values.serverDescription}"
                }) { id }
            }`,
    variables: {
        file: values.bannerFile
    }
};

superagent
    .post('/api')
    .type('form')
    .field('operations', JSON.stringify(payload))
    .field('map', JSON.stringify({ 0: ['variables.file'] }))
    .attach(0, values.bannerFile)
    .set('accept', 'json')
    .then(result => {
        console.log(result.body);
    });

Edit 1: type('form') sets the content-type as application/x-www-form-urlencoded, so removing that means content-type becomes multipart/form-data; which solves the "must provide query" error, but it did nothing to solve my original issue. The await args.input.file is still not returning anything.

img

Edit 2: Since the spec also define operations as "A JSON encoded operations object with files replaced with null" I also replaced the files variable values with null.

Edit 3: Looking deeper at the arguments to my api method now, I see that bannerFile is actually an Upload now... but my await code still gets undefined. The examples always use const { filename, mimetype, createReadStream } = await args.file so I was trying what that would be given my inputs (args.input.bannerFile) but it seems like that's actually nested for me, I'd have to use args.input.bannerFile.file. I don't see how the examples can use args.file not args.file.file... but this is progress

Edit 4: I do have to use const { filename, mimetype, createReadStream } = await args.input.bannerFile.file; but it works for me now. I don't know why I needed to go one level deeper but maybe the tutorials I've seen are out of date. It appears to be working, as I can finally get the values and the stream.

Final Edit: Yup it all works now. I'll leave all of this here in case it helps some future developer.

Here is my final client-side superagent code:

const payload = {
    query: `
        mutation($file: Upload!) {
            server:createServer(
                input: {
                    bannerFile: $file,
                    name: "${values.serverName}",
                    address: "${values.serverAddress}",
                    description: "${values.serverDescription}"
                }) { id }
            }`,
    variables: {
        file: null
    }
};

superagent
    .post('/api')
    .field('operations', JSON.stringify(payload))
    .field('map', JSON.stringify({ 0: ['variables.file'] }))
    .attach(0, values.bannerFile)
    .set('accept', 'json')
    .then(result => {
        console.log(result.body);
    });
jaydenseric commented 3 years ago

Final Edit: Yup it all works now.

Sorry to ruin the victory mood, but it's not working the way it should which is a serious problem.

What's happening now is the GraphQLUpload scalar implementation isn't being used at all. I had forgotten that amongst other limitations, buildSchema from graphql is not capable of building a schema that contains custom scalars. I think it sees the scalar Upload in the string bit, and therefore allows Upload in query validation, but it must have some sort of default scalar behavior that basically just passes the raw value through to the resolvers.

You either can switch to using pure JS modules to compose your schema like I recommended earlier, or follow the GraphQLUpload code example for a schema built using makeExecutableSchema from @graphql-tools/schema:

const { makeExecutableSchema } = require('@graphql-tools/schema');
const GraphQLUpload = require('graphql-upload/public/GraphQLUpload.js');

const schema = makeExecutableSchema({
  typeDefs: /* GraphQL */ `
    scalar Upload
  `,
  resolvers: {
    Upload: GraphQLUpload,
  },
});
viveleroi commented 3 years ago

Ok, I went through and moved my schema creation over to makeExecutableSchema. I had to tweak the signatures of all my methods but it appears to be working better now. I no longer see a nested .file but can use const { mimetype, createReadStream } = await input.bannerFile; directly.

jaydenseric commented 3 years ago

Glad you got it going :)

Now that the Upload scalar is setup correctly in the GraphQL schema it should show a more relevant error if the client was to send the wrong thing.