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

Expected type "Upload". Upload value invalid. #246

Closed chereseeriepa closed 3 years ago

chereseeriepa commented 3 years ago

Issue

When uploading a file:

// what the file looks like when it is given to the mutation on the client side (copied and pasted this from the console)
{
  lastModified: 1621800013047,
  lastModifiedDate: Mon May 24 2021 08:00:13 GMT+1200 (New Zealand Standard Time) {},
  name: "avatar",
  size: 1083999,
  type: "image/png",
  webkitRelativePath: "",
  __proto__: File
}

using graphql-upload, I get back an error:

Variable "$file" got invalid value
{
  resolve: [function],
  reject: [function],
  promise: {},
  file: {
    filename: "avatar",
    mimetype: "image/png",
    encoding: "7bit",
    createReadStream: [function createReadStream]
  }
};
Expected type "Upload". Upload value invalid.

Stack:

NOTE: this isnt the exact code, ive just simplified these down a bit to include the important bits

Client side

import { ApolloClient } from '@apollo/client' // v3.3.19
import { createUploadLink } from 'apollo-upload-client' // v15.0.0

const PORT = '18087'
const httpEndpoint = 'http://localhost:${PORT}/graphql'

export function createProvider () {
  const apolloClient = new ApolloClient({
    // ....
    link: createUploadLink({ uri: httpEndpoint })
  })

  const apolloProvider = new VueApollo({
    defaultClient: apolloClient
  })

  return apolloProvider
}

Server side

// typeDefs

scalar Upload

type Mutation {
  uploadFile(file: Upload!): Blob
}

// resolvers
const { Upload } = require('graphql-upload')

{
  Mutation: {
    uploadFile: (_, { file }) => postUploadFile(file)
  },
  Upload
}
const express = require('express') // v4.17.1
const { graphqlHTTP } = require('express-graphql') // v0.12.0
const { graphqlUploadExpress } = require('graphql-upload') // v12.0.0, graphql=v15.5.0
const cors = require('cors') // v2.8.5
const { buildFederatedSchema } = require('@apollo/federation') // v0.19.1

const app = express()
    app.use(cors())

const schema = buildFederatedSchema([
  // different typeDef + resolvers, one including graphql-upload
])

const MAX_FILE_SIZE = 5 * 1024 * 1024 // 5MB
app.use(
  '/graphql',
  graphqlUploadExpress({ maxFileSize: MAX_FILE_SIZE, maxFiles: 10 }),
  graphqlHTTP({
    schema,
    // context,
    graphiql: true
  }
)

const httpServer = http.createServer(app)

// ... starts server

We found that the issue came from the parseValue part of the Upload scalar:

https://github.com/jaydenseric/graphql-upload/blob/bdb5f808e0d514c28c0f59c1abd71680aba29bae/public/GraphQLUpload.js#L82

We just added a temporary fix that doesn't check: if (value instanceof Upload) return value.promise; and does this instead: return value.promise

Tried looking at the tests in this module and many other ones, but couldnt find a good example of a test that shows what value should look like.

Apologies if this isnt the right module to post this!

Other details

mixmix commented 3 years ago

worked on this with @chereseeriepa - as she said we've got that hack in place, and uploads are working happily now. We're not sure why it's not an instanceof Upload, had trouble following where that instantiating is happening

jaydenseric commented 3 years ago

It looks like the Upload scalar JS is not setup correctly; it appears you're using:

const { Upload } = require('graphql-upload');

When it should be:

const { GraphQLUpload } = require('graphql-upload');

See:

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

As a side note, it's a good idea to always use deep imports when they are available:

const GraphQLUpload = require('graphql-upload/public/GraphQLUpload.js');

This way only exactly the JS you need in a given context loads into memory, and if you ever bundle that code you won't rely on slow and unreliable "tree-shaking".

chereseeriepa commented 3 years ago

It looks like the Upload scalar JS is not setup correctly; it appears you're using:

const { Upload } = require('graphql-upload');

When it should be:

const { GraphQLUpload } = require('graphql-upload');

See:

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

As a side note, it's a good idea to always use deep imports when they are available:

const GraphQLUpload = require('graphql-upload/public/GraphQLUpload.js');

This way only exactly the JS you need in a given context loads into memory, and if you ever bundle that code you won't rely on slow and unreliable "tree-shaking".

Thanks for the response, good spotting on that one, but i just checked the old code and it looks like it was using GraphQLUpload where it was broken (sorry i copied a commented section for that particular line, without actually checking if it was the right import from graphql-upload)

jaydenseric commented 3 years ago

What's happening is the GraphQLUpload scalar is receiving a value that was not created using graphqlUploadExpress middleware from the same graphql-upload installation.

You could try running npm ls graphql-upload to check if there are multiple installations in node_modules, and if there are, either dedupe the installation so there is one (update dependencies, etc.), or make sure that at the very least your middleware and scalar are importing from the same one.

If that is not the problem, what might be happening is something relating to the Apollo federation stuff is replacing the GraphQLUpload scalar with their own substitute that can deal with forwarding the upload values across schemas. I'm not sure the right way to set that up, but maybe you need to import their special GraphQLUpload scalar in your codebase instead of the official graphql-upload one, or maybe they just have a bug you need to investigate.

mixmix commented 3 years ago

Yeah we read all the related issues on this repo and checked npm ls graohql-upload but can't see any duplicates.

We even changed our apollo server to match your examples (to remove any magical graphql upload auto configuration happening).

Good point about checking if @apollo/federation is interfering. Not sure where to start on that but we'll have a go looking into that and report back.

Thanks for being so responsive and helpful <3

mixmix commented 3 years ago

This could be useful for people running federation with a gateway

https://medium.com/profusion-engineering/file-uploads-graphql-and-apollo-federation-c5a878707f4c

jaydenseric commented 3 years ago

Closing because I don't think anything is actionable here, but feel free to continue the conversation or share tips for anyone else in the same boat.

stomvi commented 3 years ago

If this can help, https://github.com/jaydenseric/graphql-upload/issues/216#issuecomment-836591589