Handle java multipart form data on requests in test Helpers

[adrianlyjak]

Fixes

Fixes #11496

Purpose

Handles the case of multipart form data in the java Helpers api so that java based routes and tests that accept multipart form data files can be properly tested.

This is my first contribution here, so interested in feedback on implementation:

• I extracted a FilePart.asScala() method to keep the codebase DRY. However this method is now in the public API. Is this acceptable? It seemed somewhat appropriate as there are a bunch of similar methods in the same file for similar classes to convert to scala.
• The different types of generic "files" are still not terribly well handled for multipart form data. Only MultipartFormData[TemporaryFile] is handled. The types are cooerced somewhat awkwardly, but seems like there's likely diminishing returns with how many cases should be handled here, and I'm somewhat unclear on whether other types of form data files are even supported? Currently if you send, e.g. a FilePart[String] through, there will be another blow up in parsing internals from type casting. I could just ignore cases where the FilePart is not matching, and perhaps throw a more descriptive exception at least?
• I added a fairly monstrous test to the Play-Integration-Tests, because I had some general concerns about whether things were actually being serialized correctly. This test creates a temp file, so it may also be somewhat slow. I was basing this behavior off of some of the form tests that do the same. Is this test too much?

Pull Request Checklist

• Have you read How to write the perfect pull request?
• Have you read through the contributor guidelines?
• Have you referenced any issues you're fixing using commit message keywords?
• Have you added copyright headers to new files?
• Have you checked that both Scala and Java APIs are updated?
• Have you updated the documentation for both Scala and Java sections?
• Have you added tests for any changed functionality?

[martinariehm]

I hoped it could simply be possible to extend RequestBody.asBytes() to also handle bodies of type MultipartFormData. However, if this is not easily doable, that the changes look like a good solution with my limited knowledge.

[adrianlyjak]

@martinariehm it seems like returning bytes from MultiPartFormData with RequestBody.asBytes() was an intended optimization judging by the comments--since multipart data is generally reserved for files, and those are stored on disk. I was concerned about exposing that asBytes API to non-test use, but working with small test data bytes in testing seems reasonable

[mkurz]

First of all thanks for taking the time and providing this pull request!

At first it seemed the changes you made were ok, I almost wanted to merge, however then I quickly set up a Play Java app with the "Reproducible Test Case" posted in #11496, running against a local Play build with this patch applied and ... it failed.
I was wondering why that happens because you added tests. Turns out the tests you added did not actually test the changes you made:

• one just tested if the NullPointerException gets avoided, but didn't go deep enough to also test the logic of your change,
• the second test did not at all test your change, it even passes without your patch applied. That is because you added it as routed DSL test, which does not go through the jRoute method you patched. The jRoute method only gets traversed when a router needs to be retrieved from the app. When using the DSL however you already provide a router, so jRoute never will be called.

Since because we need to call jRoute, which in the end gets the router from the app, the easiest and sanest way to provide the application a router (without enabling the RoutesCompiler plugin on the test project), is to test a full application, which we can by using scripted tests. So that's what I did, see this commit.

Now with this scripted test I was able to reproduce the errors I just experienced
This was the exception I was getting

Test controllers.HomeControllerTest.testJavaTemporaryFile failed: java.lang.ClassCastException: class play.libs.Files$DelegateTemporaryFile cannot be cast to class play.api.libs.Files$TemporaryFile (play.libs.Files$DelegateTemporaryFile and play.api.libs.Files$TemporaryFile are in unnamed module of loader 'app'), took 0.095 sec
    at play.api.http.DefaultWriteables.$anonfun$writeableOf_MultipartFormData$1(Writeable.scala:128)
    at play.api.http.DefaultWriteables.$anonfun$writeableOf_MultipartFormData$7(Writeable.scala:169)
    at scala.collection.immutable.List.flatMap(List.scala:293)
    at scala.collection.immutable.List.flatMap(List.scala:79)
    at play.api.http.DefaultWriteables.$anonfun$writeableOf_MultipartFormData$6(Writeable.scala:168)

So the problem here was that you used Writeable.writeableOf_MultipartFormData whichs works with Scala's play.api.mvc.MultipartFormData. You did convert the object from Java's play.mvc.Http.MultipartFormData though, but that was not enough, because the ref of the object still contained Java's DelegateTemporaryFile (subclass of Java's TemporaryFile), but the writable expected the Scala one. That's why we see that exception.

Now, I was able to work around that by creating a writeable that handles Java's TemporaryFile object in th ref pointer (see commit), which made the exception go away. However I got a different problem now:

[info] [error] Test controllers.HomeControllerTest.testScalaTemporaryFile failed: java.lang.AssertionError: expected:<200> but was:<400>, took 1.074 sec
[info] [error]     at controllers.HomeControllerTest.testTemporaryFile(HomeControllerTest.java:66)
[info] [error]     at controllers.HomeControllerTest.testScalaTemporaryFile(HomeControllerTest.java:52)
[info] [error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[info] [error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[info] [error]     at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[info] [error]     at java.lang.reflect.Method.invoke(Method.java:566)
[info] [error]     ...

So I got http 400 "bad request", but expected 200. After a bit debugging I found out that the payload of the 400 error displayed "Unexpected end of input", which is caused by this line:

playframework/core/play/src/main/scala/play/core/parsers/Multipart.scala

Line 377 in 52fc73b

if (!terminated) push(out, Left(ParseError("Unexpected end of input")))

So something had to be wrong with parsing the multipart/form-data request.
And it was: The writables did not correctly escape the name field, fixed in the commit. And even worse: The boundary that is used for splitting the request body into multiple data/file parts was not consistent: The boundary supplied in the Content-Type header did not match any boundary in the request body. That's because the writable just generated its own boundary, not caring about the one of the content-type it got passed. So I fixed that as well (see commit) and now my tests did pass!

However I was not really satisfied with how this was implemented, because, like you already mentioned, this solution only works with TemporaryFiles, as soon as you use a FilePart with something else (like plain String) this whole thing would fail.
So I was experimenting and starting with commit f517b9a I came up with a different solution, which I think is ready to get merged.
The main problem is that the writables have no idea what the ref object actually contains, and because of the lack of that information it is not able to convert the ref into bytes (or work with it in general).
The only one that knows what's in the ref is the FilePart it contains. So the information about how to transform that ref into bytes has to come from that FilePart.
So the solution is that when creating a FilePart object the creator can tell it how to transform the ref to bytes by supplying a refToBytes function. When the bytes are needed we can just call transformRefToBytes() later on. That way a framework user can let Play know how an exotic object can be transformed into bytes (which is already needed when using a Plain String since they can be encoded differently which Play doesn't has a clue about)
I added a couple of default transformers for classes Play knows about and which probably will fit 97% of use cases so most users don't need to do anything.
In the scripted test you can see how to supply the refToBytes method to transform a String FilePart into bytes.

I extracted a FilePart.asScala() method to keep the codebase DRY. However this method is now in the public API. Is this acceptable?

Yes, thanks great!

The different types of generic "files" are still not terribly well handled for multipart form data...

With my solution a framework user can use any "file" type. Like said, I implemented conversions for the most obvious ones.

I added a fairly monstrous test to the Play-Integration-Tests, because I had some general concerns about whether things were actually being serialized correctly. This test creates a temp file, so it may also be somewhat slow. I was basing this behavior off of some of the form tests that do the same. Is this test too much?

The test was not monstrous. Scripted tests take much more resources and usually take much longer. Tests will never be too much, don't worry 😉

[mkurz]

I hoped it could simply be possible to extend RequestBody.asBytes() to also handle bodies of type MultipartFormData. However, if this is not easily doable, that the changes look like a good solution with my limited knowledge.

Just a note on this comment: If calling asBytes() on RequestBody, what would you expect the bytes to contain? Since the body can have many different file and data parts, it would not really make sense to retrieve all data fields and files in one single, concated blob. Having a method on each FilePart to turn it into bytes seems to be the only reasonable way (like the transformRefToBytes I implemented)

[mkurz]

@martinariehm @adrianlyjak Let me know if you have any feedback or concerns regarding my solution. I think if you look at HomeControllerTest.java you will see how my implemention works.

If you are good with this pr, I will merge it soon.
Maybe we can also backport parts of the pull request which do not break binary compatibility (basically my first solution until commit dd1f037) to the 2.8.x branch (which would only support the TemporaryFile classes though).

[adrianlyjak]

@mkurz this looks awesome! Thanks for the thorough review and for fixing it up, this all looks awesome to me!

I put a few very minor comments on the multipart Writable conversion code and tests. I'd be happy to contribute further changes if you think they're worthwhile

The scripted tests are really what I was looking for to verify this change, good to know about them. I saw them running in the github CI, but couldn't quite figure out what they were or how they worked. Still having some trouble. Looks like they depend on a publishLocal and then using the project Sbt-Plugin and then running scripted with maybe an argument? When attempting to do so, I'm getting errors from publishLocal like this

[error] Unable to locate class corresponding to inner class entry for TemporaryFile in owner play.api.libs.Files
[error] Unable to locate class corresponding to inner class entry for TemporaryFileCreator in owner play.api.libs.Files

and then errors from scripted

[info] [error] java.lang.NullPointerException: Cannot invoke "String.split(String)" because the return value of "scala.sys.SystemProperties.apply(Object)" is null

Is there some documentation or files you can point me to that might help me figure out how to run the scripted tests locally?

[mkurz]

I put a few very minor comments on the multipart Writable conversion code and tests. I'd be happy to contribute further changes if you think they're worthwhile

Which comment? I can not see any.

When I run into

[error] Unable to locate class corresponding to inner class entry for TemporaryFile...

I run clean within sbt, also I think usually I quit sbt and start it again (after still running clean) . To be honest I don't really know why this error occurs, I haven't had time to look into that. Either it's a bug in sbt or how Play and sbt play together.
Whatever, it's not your fault and not related to your changes, it happens every now and then.

To run scripted tests:
What I usually do is, I set a fixed version for the whole build, so in build.sbt I put (after the imports):

(ThisBuild / version) := "2.9.0-SNAPSHOT"

this makes sure the artifacts that get created in ~/.ivy2/local/com.typesafe.play/ all have the version 2.9.0-SNAPSHOT and also when running the scripted tests they always use 2.9.0-SNAPSHOT.
If you do not set a version explicitly, then sbt-dynver sets one, based on the last tag, commit-hash and, if you have uncommittet changes, a timestamp. That sometimes can make problems, if sbt-dynver changes the version generated between a publishLocal and when you run scripted tests (the scripted test then try to pick up a version e.g. with another timestamp that you did not actually publish).

Anyway, to run the scripted tests, you should:

$ sbt
sbt:Play-Framework> publishLocal
...
sbt:Play-Framework> project Sbt-Plugin
[info] set current project to Sbt-Plugin (in build file:...)

# Now set the versions to use
sbt:Sbt-Plugin> set scriptedSbt := "1.7.2"; set scriptedLaunchOpts += "-Dscala.version=2.13.10"; set scriptedLaunchOpts += "-Dscala.crossversions=2.13.10";
# You can now check if ithe versions got set correctly:
sbt:Sbt-Plugin> show scriptedSbt; show scriptedLaunchOpts;
# To now run a specific scripted test (e.g. the one in folder `java-test-helpers`):
sbt:Sbt-Plugin> scripted play-sbt-plugin/java-test-helpers
# To run all scripted tests just run:
sbt:Sbt-Plugin> scripted.

There are docs, but I see they are not up-to-date:
https://www.playframework.com/documentation/2.8.x/BuildingFromSource

[mkurz]

@adrianlyjak I opened #11549 to update the docs.

[adrianlyjak]

Which comment? I can not see any.
@mkurz oops, 🤦 submitted pending comments now

[mkurz]

@adrianlyjak Please check the last commit I pushed and my comments. If everything is good I think we can merge 👍

[adrianlyjak]

@adrianlyjak Please check the last commit I pushed and my comments. If everything is good I think we can merge 👍

@mkurz LGTM, this all looks awesome! Thanks for your help and all the work you do for play framework.