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

Transform Uploads to Streams before calling action #71

Merged
merged 10 commits into from
Apr 4, 2020

Conversation

dylanwulf
Copy link
Contributor

@dylanwulf dylanwulf commented Apr 2, 2020

Hello 👋 I am currently working on accepting file uploads with moleculer-apollo-server. In the process, I discovered a couple pain points which I would like to propose solutions for. These changes will result in a minor API change.

Problem 1

In ApolloServer.js, the processRequest function is being imported from v10 of graphql-upload. The output from the processRequest function then ends up passing through the GraphQLUpload scalar implementation, which is re-exported from apollo-server-core in index.js. The problem is that apollo-server-core is using v8 of graphql-upload. Since the output of v10 processRequest does not match the expected input of v8 GraphQLUpload, the action ends up receiving the wrong data.

Proposed solution to Problem 1

In index.js, we could re-export GraphQLUpload from the graphql-upload package instead of from apollo-server-core. This should ensure that both processRequest and GraphQLUpload are coming from the same version of graphql-upload.

Problem 2

When uploading a file, the action receives a parameter in the following format (assuming Problem 1 has been fixed):

{
  filename: string
  encoding: string
  mimetype: string
  createReadStream: function
}

Calling the createReadStream function will return a readable Stream which can be used to read the contents of the uploaded file. Unfortunately since functions are not serializable, file uploads do not work across the transporter.

Proposed solution to Problem 2

Moleculer supports passing streams to actions, so in service.js createActionResolver, we could transform the file upload into a stream before calling the action. This should allow file uploads to actions in separate nodes.
Since we have no way to tell which arguments are uploads, we could allow the consumer to specify the name of the argument which is expected to be a file upload (I named this option fileUploadArg).
Unfortunately this solution creates a couple sub-problems, discussed below.

Subproblem 2.1

When a stream is passed to an action, that action cannot accept any other parameters. So we can get the file contents to the action, but filename, encoding, and mimetype get lost.

Proposed solution to Subproblem 2.1

We can put filename, encoding, and mimetype into ctx.meta so that the action still has access to that data (I used a property called ctx.meta.$fileInfo).

Subproblem 2.2

graphql-upload allows uploading multiple files in a single mutation. But moleculer actions can only receive a single stream.

Proposed solution to Subproblem 2.2

If the fileUploadArg is an array, then we can call the action once per array element, and combine the results into a result array. This would allow multiple file uploads in a single mutation.

Thank you!

src/service.js Outdated Show resolved Hide resolved
src/service.js Outdated Show resolved Hide resolved
@shawnmcknight
Copy link
Member

This proposal looks good to me with the exception of my few notes. Please make sure to update the TS types (I think only the GraphQL upload is affected) also.

@dylanwulf dylanwulf marked this pull request as ready for review April 3, 2020 21:32
index.js Outdated Show resolved Hide resolved
Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really solid. Thank you for including the examples and documentation. I just had one note to relocate the placement of the export.

Copy link
Member

@icebob icebob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your PR! Particularly, you've added example & update docs with example. Great job!

src/service.js Show resolved Hide resolved
@icebob
Copy link
Member

icebob commented Apr 4, 2020

@shawnmcknight I've approved it. If you also approve, please merge it.

Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Merging!

@shawnmcknight shawnmcknight merged commit e89ec85 into moleculerjs:master Apr 4, 2020
@shawnmcknight
Copy link
Member

@icebob When you have an opportunity, can you publish a new version for this?

This is a partial breaking change due to the export of a different version of GraphQL upload which elicits different behavior from the object that is resolved. The new transformation to a stream logic is opt-in, but an existing consumer would see different behavior due to how the graphql-upload version differs from how the version exported from apollo-core worked.

@icebob
Copy link
Member

icebob commented Apr 4, 2020

Released as 0.3.0: https://github.com/moleculerjs/moleculer-apollo-server/releases/tag/v0.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants