Skip to content

Add support for Graphql file upload #434

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

Closed
wants to merge 8 commits into from
Closed

Add support for Graphql file upload #434

wants to merge 8 commits into from

Conversation

eokoneyo
Copy link
Contributor

@eokoneyo eokoneyo commented Dec 2, 2021

Hi There,

Really great work with what's been done so far with the implementation to translate OAS to GraphQL, I recently had some reason whilst using this library to support file uploads for the GraphQL server running on the schema generated by the library. I know there's no support for it and it's already been brought up as seen here #220

So I added support for file uploads, Here are the considerations I made;

  • API definitions with content-typeof multipart/form-data are processed as such
  • the implementation for file upload is solely based on the spec proposal defined here https://github.com/jaydenseric/graphql-multipart-request-spec which would imply that users who wish to use this need to make some extra configuration for parsing request body to match the spec's expectations.
  • Uploaded files are streamed directly to the intending REST endpoint, this way the library needs not necessarily concern itself too much about memory management and the problems that could accompany it when multiple users are uploading files at scale.

I would appreciate feedback pertaining to this and like to see this merged in, as I'm using the changes in here as a patch to the library for upload support in my team.

@Acring
Copy link

Acring commented Dec 7, 2021

I need this. hope it can be merged as soon as possible

@Alan-Cha
Copy link
Collaborator

@eokoneyo Sorry for the late reply! This looks really good to me! If you could just clarify one thing, I think we can merge this in an push out a new version!

@eokoneyo
Copy link
Contributor Author

@eokoneyo Sorry for the late reply! This looks really good to me! If you could just clarify one thing, I think we can merge this in an push out a new version!

@Alan-Cha That's great to hear, I wasn't sure if this was a feature that's in line with the library's roadmap. Since you're okay with this, I think there'll be a need to add details about supporting file upload to the README file

@Alan-Cha
Copy link
Collaborator

@eokoneyo Ah, the public roadmap is outdated but we welcome any suggestions and new features.

Do you think you could flesh out the documentation? I think you are more qualified to describe what is going on. If you can do that, then I can merge and push out a new release.

@eokoneyo
Copy link
Contributor Author

@eokoneyo Ah, the public roadmap is outdated but we welcome any suggestions and new features.

Do you think you could flesh out the documentation? I think you are more qualified to describe what is going on. If you can do that, then I can merge and push out a new release.

I actually stumbled on the roadmap today, I'll be sure to add in any new features or suggestions I might have and yes, I'll add in the documentation for this.

@eokoneyo eokoneyo requested a review from Alan-Cha December 15, 2021 15:21
@eokoneyo
Copy link
Contributor Author

eokoneyo commented Dec 15, 2021

@Alan-Cha I added in documentation, also some slight modifications to allow the users specify config options pertaining to how the file upload is processed. I also wanted to point out that my changes make it so this module has to run within a NodeJS context because of the file upload.

@Alan-Cha
Copy link
Collaborator

@eokoneyo Everything looks great! I made some small adjustments and also rebased it but because I do not have push access to your branch, I had to create my own and filed a new PR #435. I will merge everything in through said PR and close this one.

I know you have some other PRs that you submitted a while ago. I plan on looking on them as well as all of the other stale PRs. I'm trying to get back into maintaining this project. In the meantime, feel free to add suggestions to the roadmap and thanks again for this great contribution!

@Alan-Cha Alan-Cha closed this Dec 15, 2021
@Alan-Cha
Copy link
Collaborator

v2.6.0 is out!

@eokoneyo eokoneyo deleted the feat/add-support-for-graphql-file-upload branch March 14, 2022 15:49
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