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

Add support for file uploads #920

Merged
merged 7 commits into from
Mar 11, 2021

Conversation

InsOpDe
Copy link
Contributor

@InsOpDe InsOpDe commented Feb 21, 2021

Addresses issues #782 #93
Took most code from #96 - so props to ezra-quemuel. sadly his code didnt got merged and it was too much of a hassle to merge his PR with the current code. This PR should fix this and add one of the most wanted features to tsoa

I decided not to store files send to the server, instead let the developer decide what to do with the received buffer.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue?

Closing issues

issues are already closed (because a work around existed)

If this is a new feature submission:

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

None

Test plan

Added 2 unit tests for each middleware (hapi, koa and express) one for testing whether the actual issue works, another one for testing multiple text-fields (plus multiple files) being uplaoded at once.

@InsOpDe InsOpDe force-pushed the 93-add-support-for-file-uploads branch from 778a34e to e0af755 Compare February 21, 2021 13:50
@WoH
Copy link
Collaborator

WoH commented Feb 27, 2021

Can you add missing spec tests for OpenAPI 2/3?
One of the reason this currently is tedious is the manual spec writing.

@InsOpDe
Copy link
Contributor Author

InsOpDe commented Feb 27, 2021

Will do

@InsOpDe InsOpDe force-pushed the 93-add-support-for-file-uploads branch from cc56c48 to 00e3ef8 Compare February 28, 2021 12:14
@InsOpDe
Copy link
Contributor Author

InsOpDe commented Feb 28, 2021

  • Removed file[] type as we have array
  • Added negative test cases for checking that an error is thrown if the keynames are wrong in a multipart/form-data upload
  • Added multiple methods which ensure that formData generates a valid requestBody for uploads. Also overwrote return value for dataType which changed from file to string with binary in openapi3
  • Added openApi v2 and v3 tests which verify that a valid swagger file is generated (regarding to file upload)
  • Run tests with https://editor.swagger.io/ to ensure that the GUI is properly rendered

@InsOpDe InsOpDe force-pushed the 93-add-support-for-file-uploads branch from 4e76081 to 43b2dcc Compare March 5, 2021 20:46
@WoH WoH merged commit fd1915f into lukeautry:master Mar 11, 2021
@WoH
Copy link
Collaborator

WoH commented Mar 11, 2021

LGTM, thanks a lot for submitting the PR

@joel1st
Copy link

joel1st commented Aug 16, 2021

Thanks for this work! Have been using TSOA for nearly 3 years and very excited to see this land.

Just some quick questions (I can put up a documentation PR after I've confirmed)

  • Because files are loaded into memory, I guess this should only be used for small/non frequent uploads (eg a large video upload could crash the server from OOM / a lot of smaller assets being uploaded concurrently) https://www.npmjs.com/package/multer#user-content-memorystorage

  • If we did want to get around this limitation is there a way to change the multer configuration? or should we use the old approach?

@WoH
Copy link
Collaborator

WoH commented Aug 16, 2021

You can either use the old approach, adapt the route template, or submit a PR that allows you to hook into/provide the config we pass to multer.
Other approaches are probably less advisable.

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.

None yet

3 participants