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 multiple file upload directive #1033

Merged
merged 5 commits into from Sep 19, 2017

Conversation

Projects
None yet
8 participants
@adlawson
Contributor

adlawson commented Apr 14, 2017

Adds support for multiple file uploads with the same field name.
I've followed the same implementation and testing approach as fileUpload. I did think about changing the fileUpload implementation to take the head of fileUploadAll but it would remove any benefit Sink.headOption gives you over Sink.seq.

// scaladsl
def fileUploadAll(fieldName: String): Directive1[immutable.Seq[(FileInfo, Source[ByteString, Any])]]

// javadsl
def fileUploadAll(fieldName: String, inner: JFunction[JList[JMap.Entry[FileInfo, Source[ByteString, Any]]], Route]): Route

Tasks

Following review at #1033 (review) there are a few outstanding pieces of work that need to be done before merge

  • Rename directives
  • Separate destination file creation
  • Refactor Source(files).map to just files.map
  • Java Documentation
  • Scala Documentation
  • Deprecate uploadedFile directive(?) Todo in a following PR
@lightbend-cla-validator

This comment has been minimized.

Show comment
Hide comment
@lightbend-cla-validator

lightbend-cla-validator Apr 14, 2017

Hi @adlawson,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

lightbend-cla-validator commented Apr 14, 2017

Hi @adlawson,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Apr 14, 2017

Collaborator

Can one of the repo owners verify this patch?

Collaborator

akka-ci commented Apr 14, 2017

Can one of the repo owners verify this patch?

@jlprat

This comment has been minimized.

Show comment
Hide comment
@jlprat

jlprat Apr 15, 2017

Member

OK TO TEST

Member

jlprat commented Apr 15, 2017

OK TO TEST

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Apr 15, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Apr 15, 2017

Test FAILed.

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Apr 18, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Apr 18, 2017

Test PASSed.

@adlawson adlawson changed the title from [WIP] Add fileMultiUpload directive to Add fileMultiUpload directive Apr 18, 2017

@jrudolph

Needs some solution to the deadlocking problem, commented above. (Just adding the "Changes Requested" marker for administrative reasons)

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Apr 28, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Apr 28, 2017

Test PASSed.

@adlawson

This comment has been minimized.

Show comment
Hide comment
@adlawson

adlawson Apr 28, 2017

Contributor

I've updated the implementation to buffer to disk and squashed the commits into one. The changes now include 2 directives, where the first depends on the second.

def fileMultiUpload(fieldName: String): Directive1[immutable.Seq[(FileInfo, Source[ByteString, Any])]]

def uploadedFiles(fieldName: String): Directive1[immutable.Seq[(FileInfo, File)]]

I had started to go down the route of a custom GraphStage to buffer to disk, just like Alpakka's akka.stream.alpakka.s3.impl.DiskBuffer but it proved harder to implement and more complex than necessary. I also couldn't figure out a way to guarantee reading from a written-to file. Maybe chunking writes may have helped but I still don't see any guarantees.

Another solution I tried was Flow.fromSinkAndSource but it appears the Source doesn't wait for the Sink to fully complete, so this wouldn't work.

The final solution looks too simple to me, but I've tested it with multiple large files and they stream over perfectly fine. I suppose the only downside is that we effectively have to wait for all ByteString sinks to complete before we can begin sourcing again from disk.

Contributor

adlawson commented Apr 28, 2017

I've updated the implementation to buffer to disk and squashed the commits into one. The changes now include 2 directives, where the first depends on the second.

def fileMultiUpload(fieldName: String): Directive1[immutable.Seq[(FileInfo, Source[ByteString, Any])]]

def uploadedFiles(fieldName: String): Directive1[immutable.Seq[(FileInfo, File)]]

I had started to go down the route of a custom GraphStage to buffer to disk, just like Alpakka's akka.stream.alpakka.s3.impl.DiskBuffer but it proved harder to implement and more complex than necessary. I also couldn't figure out a way to guarantee reading from a written-to file. Maybe chunking writes may have helped but I still don't see any guarantees.

Another solution I tried was Flow.fromSinkAndSource but it appears the Source doesn't wait for the Sink to fully complete, so this wouldn't work.

The final solution looks too simple to me, but I've tested it with multiple large files and they stream over perfectly fine. I suppose the only downside is that we effectively have to wait for all ByteString sinks to complete before we can begin sourcing again from disk.

@patrox

This comment has been minimized.

Show comment
Hide comment
@patrox

patrox Jul 10, 2017

hi @adlawson - I'm quite interested in the functionality from this PR, so I would like to ask if you are going to finish it ?

@jrudolph Can you please give me some overview of what has to be done in order to get this PR to a mergable state ?

patrox commented Jul 10, 2017

hi @adlawson - I'm quite interested in the functionality from this PR, so I would like to ask if you are going to finish it ?

@jrudolph Can you please give me some overview of what has to be done in order to get this PR to a mergable state ?

@jrudolph

This is looking quite good (even it seems to simple, streams should make things simple ;) ).

Apart from the few comments, it also needs documentation before it can be merged.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Jul 17, 2017

Member

(And sorry for keeping it around for so long. It's not because the PR wasn't good, we were just busy.)

Member

jrudolph commented Jul 17, 2017

(And sorry for keeping it around for so long. It's not because the PR wasn't good, we were just busy.)

@adlawson

This comment has been minimized.

Show comment
Hide comment
@adlawson

adlawson Jul 18, 2017

Contributor

Thanks for the feedback. I stuck with similar names to the single file uploads, just for consistency, but I'm happy to change them to be more descriptive. I'll also get onto your other points like separating temp file construction etc. too.

Don't worry about the delay - I've been buried under a mountain of work too. I'll try to get this updated within the next week.

Contributor

adlawson commented Jul 18, 2017

Thanks for the feedback. I stuck with similar names to the single file uploads, just for consistency, but I'm happy to change them to be more descriptive. I'll also get onto your other points like separating temp file construction etc. too.

Don't worry about the delay - I've been buried under a mountain of work too. I'll try to get this updated within the next week.

@akka-ci akka-ci added validating and removed tested labels Jul 30, 2017

@adlawson

This comment has been minimized.

Show comment
Hide comment
@adlawson

adlawson Jul 30, 2017

Contributor

Hey @jrudolph, I've made some updates since your last review

  • Renamed uploadedFiles to storeUploadedFiles to better describe what it does
  • Separated out the temporary file creation
  • Added @ApiMayChange where necessary, and removed the mima-filter exclusions

I've also taken a couple of further steps

  • Renamed fileMultiUpload to fileUploadAll so "multi" isn't confused with "multipart"
  • Added a storeUploadedFile (singular) to have a consistent API with the new directive. This should replace uploadedFile (which still exists and behaves the same as before, but could be deprecated in favour of this new function)

Let me know what you think, and whether you agree with storeUploadedFile or not.

Contributor

adlawson commented Jul 30, 2017

Hey @jrudolph, I've made some updates since your last review

  • Renamed uploadedFiles to storeUploadedFiles to better describe what it does
  • Separated out the temporary file creation
  • Added @ApiMayChange where necessary, and removed the mima-filter exclusions

I've also taken a couple of further steps

  • Renamed fileMultiUpload to fileUploadAll so "multi" isn't confused with "multipart"
  • Added a storeUploadedFile (singular) to have a consistent API with the new directive. This should replace uploadedFile (which still exists and behaves the same as before, but could be deprecated in favour of this new function)

Let me know what you think, and whether you agree with storeUploadedFile or not.

@akka-ci akka-ci added validating and removed tested labels Sep 10, 2017

adlawson added some commits Apr 28, 2017

Add fileUploadAll directive
- Add fileUploadAll directive, buffering multiple files on disk
- Add storeUploadedFiles directive to stream multiple files to disk
- Add mima-filter exclusions for new directives
- Add destFn argument to directives to support custom destinations
- Replace uploadedFile directive with storeUploadedFile
Add documentation for new file upload directives
- Document fileUploadAll directive
- Document storeUploadedFile directive
- Document storeUploadedFiles directive
@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Sep 10, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Sep 10, 2017

Test FAILed.

@akka-ci akka-ci added tested and removed needs-attention labels Sep 10, 2017

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Sep 10, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Sep 10, 2017

Test PASSed.

@adlawson

This comment has been minimized.

Show comment
Hide comment
@adlawson

adlawson Sep 10, 2017

Contributor

I've added the discardBytes call you asked for, but I haven't been able to come up with a working test for it. The closest I had was observing completing promises when the streams are consumed, just like the tests in akka/http/scaladsl/model/EntityDiscardingSpec.scala#L44-L53, but the futures never complete. I also tried simply consuming streams again to trigger and catch an IllegalStateException but I was able to consume them all again fine, even the ones consumed to files. If you have any pointers I'll keep trying, but I'm at a loss right now.

In addition to the change, I've squashed the commits into more reasonable, safe, chunks of work.

Contributor

adlawson commented Sep 10, 2017

I've added the discardBytes call you asked for, but I haven't been able to come up with a working test for it. The closest I had was observing completing promises when the streams are consumed, just like the tests in akka/http/scaladsl/model/EntityDiscardingSpec.scala#L44-L53, but the futures never complete. I also tried simply consuming streams again to trigger and catch an IllegalStateException but I was able to consume them all again fine, even the ones consumed to files. If you have any pointers I'll keep trying, but I'm at a loss right now.

In addition to the change, I've squashed the commits into more reasonable, safe, chunks of work.

jrudolph added some commits Sep 18, 2017

!htp deprecate uploadedFile directive in favor of new storeUploadedFile
storeUploadedFile has a more descriptive name and also adds the possibility
to select the target file.
=htp slight improvement of `storeUploadedFile` signature
Directive1[T] = Directive[Tuple1[T]], so don't nest another Tuple2 inside.
This also allows to get rid of the `case` syntax when using the directive.

Slightly improved implementation as well .
=htp add additional streaming tests for new FileUploadDirectives
Before only Strict BodyParts/HttpEntity were tested which are not
susceptible to deadlocks. The new version should test the more common
streaming uploads more comprehensively.
@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Sep 18, 2017

Member

@adlawson looking very good now. I added a few commits with smallish cleanups and the deprecation of uploadedFile. Could you review and merge https://github.com/adlawson/akka-http/pull/1 into this PR (couldn't push to your branch)?

Member

jrudolph commented Sep 18, 2017

@adlawson looking very good now. I added a few commits with smallish cleanups and the deprecation of uploadedFile. Could you review and merge https://github.com/adlawson/akka-http/pull/1 into this PR (couldn't push to your branch)?

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Sep 19, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Sep 19, 2017

Test PASSed.

@adlawson

This comment has been minimized.

Show comment
Hide comment
@adlawson

adlawson Sep 19, 2017

Contributor

Changes look good, @jrudolph. Thanks for the updates.
All merged and ready to go 👍

Contributor

adlawson commented Sep 19, 2017

Changes look good, @jrudolph. Thanks for the updates.
All merged and ready to go 👍

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 19, 2017

Member

Great, will give it a final read then :)

Member

ktoso commented Sep 19, 2017

Great, will give it a final read then :)

@ktoso

ktoso approved these changes Sep 19, 2017

LGTM, great effort, thanks a lot 👍

* @group fileupload
*/
@ApiMayChange
def storeUploadedFile(fieldName: String, destFn: FileInfo File): Directive[(FileInfo, File)] =

This comment has been minimized.

@ktoso

ktoso Sep 19, 2017

Member

nice name btw 👍

@ktoso

ktoso Sep 19, 2017

Member

nice name btw 👍

Docs added and Johanness's PR also merged, everything looks good

@ktoso ktoso merged commit a1a9c27 into akka:master Sep 19, 2017

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 19, 2017

Member

Resolved #1273 Thanks!

Member

ktoso commented Sep 19, 2017

Resolved #1273 Thanks!

@adlawson adlawson deleted the adlawson:adlawson/directive-file-multiupload branch Sep 19, 2017

/**
* Streams the bytes of the file submitted using multipart with the given field name into designated files on disk.
* If there is an error writing to disk the request will be failed with the thrown exception, if there is no such
* field the request will be rejected. Stored files are cleaned up on exit but not on failure.

This comment has been minimized.

@mary-element

mary-element Oct 6, 2017

Does storeUploadedFiles Directive reject request if field is empty?
It seems there is no implementation of it

@mary-element

mary-element Oct 6, 2017

Does storeUploadedFiles Directive reject request if field is empty?
It seems there is no implementation of it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment