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

WIP: Multipart form directive #2006

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@daddykotex
Contributor

daddykotex commented May 4, 2018

Given a list of FileField, the directive will look for parts with a name equal to the FileField#fieldName.
These parts will be streamed on disk in the file returned by FileField#fileNameF call. All the other,
non-binary and strict part will be gathered in a Map[String, List[String]] similar to what
FormFieldDirectives#formFieldMultiMap returns.

All other parts will be discarded.

Given Seq("file" -> (_ => new File("/tmp/content.txt"))) and the following request:

POST /submit
Content-Type: multipart/form-data; boundary=------------------------boundary
--------------------------boundary
Content-Disposition: form-data; name="name"
Content-Type: text/plain; charset=UTF-8

name_value
--------------------------boundary
Content-Disposition: form-data; name="version"
Content-Type: text/plain; charset=UTF-8

version_value
--------------------------boundary
Content-Disposition: form-data; name="unannounced"; filename="dicarded.txt"
Content-Type: text/plain; charset=UTF-8

discarded
--------------------------boundary
Content-Disposition: form-data; name="file"; filename="content.txt"
Content-Type: text/plain; charset=UTF-8

file content
--------------------------boundary--

The map would be: Map("name" -> List("name_value"), "version" -> List("version_value"))
The list would be List(FileInfo("file", "content.txt", Content.Type.text/plain) -> File("/tmp/content.txt"))

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented May 4, 2018

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@daddykotex

This comment has been minimized.

Contributor

daddykotex commented May 4, 2018

Travis gave:

[error]  * abstract method formAndFiles(scala.collection.immutable.Seq)akka.http.scaladsl.server.Directive in trait akka.http.scaladsl.server.directives.MultipartDirectives is inherited by class Directives in current version.
[error]    filter with: ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("akka.http.scaladsl.server.Directives.formAndFiles")

I'm not sure how new code makes this binary incompatible.

@johanandren

This comment has been minimized.

Member

johanandren commented May 4, 2018

The reason it is potentially binary incompatible is that akka.http.scaladsl.server.directives.MultipartDirectives is a trait which users may have subclassed/mixed into their own classes, where they could potentially already have a conflicting method.

Not sure how we have done with new directive methods before. @raboof ?

@raboof

This comment has been minimized.

Member

raboof commented May 4, 2018

OK TO TEST

@akka-ci akka-ci added the validating label May 4, 2018

@raboof

This comment has been minimized.

Member

raboof commented May 4, 2018

Hmm, in https://github.com/akka/akka-http/pull/1828/files#diff-390e61628477725ff016449aa751dea4R25 we ignored it, though I'm not completely sure whether that was an oversight. Not sure why it didn't seem to be needed for https://github.com/akka/akka-http/pull/504/files?

@akka-ci akka-ci added needs-attention and removed validating labels May 4, 2018

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented May 4, 2018

Test FAILed.

@daddykotex

This comment has been minimized.

Contributor

daddykotex commented May 8, 2018

I'm sorry I thought I would receive email for this PR, but I did not.

Ok, I understand the binary compatibility issue. But I don't know how to fix it, if I don't add it to the Directives trait, then it will hurt usability IMO.

WIP:Multipart form directive
Given a list of FileField, the directive will look for parts with a name equal to the FileField#fieldName.
These parts will be streamed on disk in the file returned by FileField#fileNameF call. All the other,
non-binary and strict part will be gathered in a Map[String, List[String]] similar to what
FormFieldDirectives#formFieldMultiMap returns.

All other parts will be discarded.

Given `Seq("file" -> (_ => new File("/tmp/content.txt")))` and the following request:

```
POST /submit
Content-Type: multipart/form-data; boundary=------------------------boundary
--------------------------boundary
Content-Disposition: form-data; name="name"
Content-Type: text/plain; charset=UTF-8

name_value
--------------------------boundary
Content-Disposition: form-data; name="version"
Content-Type: text/plain; charset=UTF-8

version_value
--------------------------boundary
Content-Disposition: form-data; name="unannounced"; filename="dicarded.txt"
Content-Type: text/plain; charset=UTF-8

discarded
--------------------------boundary
Content-Disposition: form-data; name="file"; filename="content.txt"
Content-Type: text/plain; charset=UTF-8

file content
--------------------------boundary--
```

The map would be: `Map("name" -> List("name_value"), "version" -> List("version_value"))`
The list would be `List(FileInfo("file", "content.txt", Content.Type.`text/plain`) -> File("/tmp/content.txt"))`

@daddykotex daddykotex force-pushed the daddykotex:upload-form-files branch from 52c02e3 to 62fa27d May 8, 2018

@daddykotex

This comment has been minimized.

Contributor

daddykotex commented May 8, 2018

Also if this is something that makes senses, I can add documentation regarding this new directive.

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented May 8, 2018

Test FAILed.

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