[418] multipart/form-data support #948

andymc12 · 2021-03-04T20:49:36Z

Fixes #418

Includes new API, spec text and some simple examples.

mkarg · 2021-03-05T20:55:43Z

@andymc12 Thank you for contributing this! Unfortunately I am busy with JavaLand organization recently, so the next two weeks I won't have time to actually review this code. Sorry for that. I pretend to have a look at it after JavaLand! :-)

andymc12 · 2021-03-18T20:25:02Z

@mkarg - I hope JavaLand went well - it looks like it was well attended with lots of notable speakers! Hopefully you have recovered and can review this PR?

@chkal @spericas - could you take a look as well? Thanks!

mkarg · 2021-03-19T18:00:30Z

@mkarg - I hope JavaLand went well - it looks like it was well attended with lots of notable speakers! Hopefully you have recovered and can review this PR?

Thanks, Andy, yes, we had approx. 1500 online attendees and a quite shiny lineup of notable speakers. Despite all hopes, I am still busy today and tomorrow, so if things run well, will look into this PR on Sunday. Stay tuned! :-)

mkarg

I'm done with an initial preview. Kudos, this is great work! But I have some questions, see inlined. Also I wonder why we force the use of streams? I understand that streams have benefits, by why not also allows parameter conversion or using Entity classes?

examples/src/main/java/jaxrs/examples/multipart/MultipartClient.java

examples/src/main/java/jaxrs/examples/multipart/MultipartResource.java

jaxrs-api/src/main/java/jakarta/ws/rs/ext/Part.java

jaxrs-spec/src/main/asciidoc/chapters/resources/_declaring_method_capabilities.adoc

jaxrs-api/src/main/java/jakarta/ws/rs/ext/Part.java

examples/src/main/java/jaxrs/examples/multipart/MultipartClient.java

jaxrs-spec/src/main/asciidoc/chapters/resources/_declaring_method_capabilities.adoc

andymc12 · 2021-04-01T16:07:25Z

Hi @mkarg @spericas - thanks for all of the feedback - excellent suggestions! I think I have addressed all of your concerns. Would you mind taking another look and approving if you think it's ready to go in? Thanks again.

mkarg · 2021-04-01T17:53:40Z

Hi @mkarg @spericas - thanks for all of the feedback - excellent suggestions! I think I have addressed all of your concerns. Would you mind taking another look and approving if you think it's ready to go in? Thanks again.

@andymc12 Excellent PR, but please let's have Easter first. :-)

chkal

@andymc12 Thank you so much for working on this. See my comment in the source.

jaxrs-api/src/main/java/jakarta/ws/rs/ext/Part.java

andymc12 · 2021-04-08T19:16:10Z

@spericas @mkarg @chkal Thanks for the great review comments so far! I think I've addressed all concerns and suggestions. Would you mind taking another look and approving? Thanks!

spericas

LGTM

chkal

@andymc12 Thanks a lot for integrating the latest changes. This looks awesome. I'm basically +1 for this, but added one comment. It would be great to hear your thoughts.

jaxrs-spec/src/main/asciidoc/chapters/resources/_declaring_method_capabilities.adoc

jaxrs-api/src/main/java/jakarta/ws/rs/ext/Part.java

andymc12 · 2021-04-13T19:16:08Z

Added support for @FormParam("partName") SomeObject param in the spec text.

@chkal, @spericas, @mkarg, others - would you take another look and (re-)approve? Thanks!

chkal · 2021-04-14T16:01:59Z

jaxrs-spec/src/main/asciidoc/chapters/resources/_declaring_method_capabilities.adoc

+annotation corresponds to the name of the part. The parameter type may be
+a `jakarta.ws.rs.ext.Part`, a `java.io.InputStream`, a `String`, or any type
+that can be converted to using a registered `MessageBodyReader` or
+`ParamConverter`. Here is


Sorry. One last question.

The API docs of Part#getContent(Class|GenericType) only mentions MessageBodyReader, but here we allow ParamConverter as well.

I'm wondering if ParamConverter makes sense here. If we allow both, we will most likely have to define which one of the two mechanisms is preferred over the other. And ParamConverter uses a String as the input and in the next paragraph we explicitly warn about String being not optimal as it requires to read the full entity into the heap.

My thinking here was that ParamConverter providers are already responsible for converting Strings to parameter types. The only real difference here (with @FormParam) is that we have an InputStream instead of a a String - but since we allow the implementation to convert the InputStream to a String for @FormParam(...) String, it seemed intuitive (to me, at least :) ) that the implementation should first check that it can convert to the parameter type using registered MBRs, and if it finds none, then it could check to see if a ParamConverter is registered for the parameter type. If so, then the implementation would convert the part's InputStream to String and then invoke the ParamConverter.

For Part#getContent(...), I don't think it makes sense to use ParamConverter because at this point, it's not a parameter; the parameter was Part. A MBR does make sense because it is reading the message body of the individual part to the specified class or GenericType.

BTW, I appreciate the thoroughness of your review. I'd certainly rather spend time up front to get it right rather than trying to patch something that was broken from the start. :)

@andymc12 Thanks for your explanation and sorry for the delayed reply.

After thinking about this more, it somehow makes sense to allow ParamConverter in case of @FormParam, as it is also used for standard @FormParam cases. However, I also think it could be a bit weird for the user that @FormParam and getContent may behave differently.

So basically we have three options:

@FormParam uses the MBR and falls back to a ParamConverter (if available), but getContent() only uses the MBR.

Both @FormParam and getContent() use the MBR and fall back to ParamConverter.

Both @FormParam and getContent() only use the MBR.

To be honest, I'm not sure yet which option I personally like most.

@eclipse-ee4j/ee4j-jaxrs-committers Other opinions?

Documentation of ParamConverter says it applies to parameters injected via (among other annotations) @FormParam.:

Defines a contract for a delegate responsible for converting between a String form of a message parameter value and the corresponding custom Java type T. Conversion of message parameter values injected via @PathParam, @QueryParam, @MatrixParam, @FormParam, @CookieParam and @HeaderParam is supported.

I am not aware of an existing API method that would utilize converters. Therefore to maintain consistency, I'd suggest staying with option 1.

@FormParam uses the MBR and falls back to a ParamConverter (if available), but getContent() only uses the MBR.

Both @FormParam and getContent() use the MBR and fall back to ParamConverter.

Both @FormParam and getContent() only use the MBR.

getContent() is not intended for parameters, so it shouldn't use ParamConverter.

My preference would be 1.

Actually after some thinking I wonder why we want @FormParam to use MBR, actually, as it is a parameter.

In my view @FormParam on types other than EntityPart is basically a convenient way to parse and access part data.

So instead of doing this:

public Response someMethod( List<EntityPart> parts ) { JsonObject data = parts.stream() .filter( p -> p.getId().equals("file") ) .findFirst() .map( p -> p.getContent( JsonObject.class ) ) .orElse( null ); // ... }

or instead of this:

public Response someMethod( @FormParam("file") EntityPart part ) { JsonObject data = part.getContent( JsonObject.class ); // ... }

You can simply do this:

public Response someMethod( @FormParam("file") JsonObject data ) { // ... }

And therefore it would make sense that using @FormParam on some specific type T is the same as calling EntityPart.getContent(T.class).

However, I agree that this may be problematic given the originally intended usage of ParamConverter and MessageBodyReader.

jansupol · 2021-04-14T17:52:38Z

@andymc12 Part seems to be quite a general name. Possibly, a part of anything else can arise and the name would clash. Would it be better to tell Part of what this class is? EntityPart perhaps?

mkarg · 2021-04-15T17:23:05Z

Speaking in terms of Java, it would be Entity.Part, as it is not to be seen as an alternative to an entity, but as a part of it.

andymc12 · 2021-04-21T19:53:14Z

@jansupol @mkarg

@andymc12 Part seems to be quite a general name. Possibly, a part of anything else can arise and the name would clash. Would it be better to tell Part of what this class is? EntityPart perhaps?

Speaking in terms of Java, it would be Entity.Part, as it is not to be seen as an alternative to an entity, but as a part of it.

I see that Part is used in the servlet spec (for pretty much the same thing as we're using it for here) and in the Java/Jakarta Mail spec. I think it's probably rare enough of a usage that it probably won't clash (and worst case, the package name isn't super long), but I'm not averse to renaming it.

I like Markus's idea of Entity.Part, but the Entity class is in the client package. Since this class can be used on both the client and server, I'd like to avoid confusion and would probably go with EntityPart in the ext package.

andymc12 · 2021-04-21T20:59:28Z

Renamed Part to EntityPart.

@spericas @chkal @mkarg @jansupol could you take another look and approve? Thanks again!

mkarg · 2021-04-22T11:24:15Z

@jansupol @mkarg

@andymc12 Part seems to be quite a general name. Possibly, a part of anything else can arise and the name would clash. Would it be better to tell Part of what this class is? EntityPart perhaps?

Speaking in terms of Java, it would be Entity.Part, as it is not to be seen as an alternative to an entity, but as a part of it.

I see that Part is used in the servlet spec (for pretty much the same thing as we're using it for here) and in the Java/Jakarta Mail spec. I think it's probably rare enough of a usage that it probably won't clash (and worst case, the package name isn't super long), but I'm not averse to renaming it.

I like Markus's idea of Entity.Part, but the Entity class is in the client package. Since this class can be used on both the client and server, I'd like to avoid confusion and would probably go with EntityPart in the ext package.

Another idea would be to actually use Entity.Part, but to refactor it in a future release so it is getting deprecated in favor of the same class found in the core package. That would be straightforward.

andymc12 · 2021-05-04T21:46:48Z

@chkal @spericas

@andymc12 The latest changes look good. I think at some point we need to answer @chkal 's question about ParamConverter's but, honestly, I'm not sure which option is best at this time. Perhaps we need more time using the API to determine that.

Agreed on this one. Maybe we should only allow @FormParam for EntityPart for now? We can still add support for other types later on if we want.

What if we also left in the ability to automatically convert to InputStream and String? i.e.

public Response someMethod( @FormParam("file") InputStream data ) {
  // ...
}

and

public Response someMethod( @FormParam("file") String data ) {
  // ...
}

This should avoid the confusion with ParamConverters, but still allow some of the more common use cases - like writing a part to disk or reading it directly as a String.

This would mean that code block 1 and 2 from my comment would work fine. Just code block 3 won't be possible. But IMO that would be acceptable.

That should be fine. The only difference between your 2nd block and 3rd is one line of code. I can live with that! :-)

spericas · 2021-05-05T13:00:34Z

@chkal @spericas

@andymc12 The latest changes look good. I think at some point we need to answer @chkal 's question about ParamConverter's but, honestly, I'm not sure which option is best at this time. Perhaps we need more time using the API to determine that.

Agreed on this one. Maybe we should only allow @FormParam for EntityPart for now? We can still add support for other types later on if we want.

What if we also left in the ability to automatically convert to InputStream and String? i.e.
public Response someMethod( @FormParam("file") InputStream data ) {
  // ...
}
and
public Response someMethod( @FormParam("file") String data ) {
  // ...
}
This should avoid the confusion with ParamConverters, but still allow some of the more common use cases - like writing a part to disk or reading it directly as a String.

This would mean that code block 1 and 2 from my comment would work fine. Just code block 3 won't be possible. But IMO that would be acceptable.

That should be fine. The only difference between your 2nd block and 3rd is one line of code. I can live with that! :-)

This seems like a reasonable solution at this time; getting the param converters in the mix seems reasonable but also adds additional complexity.

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

- "ex:" to "e.g." - Using modern java.nio.file examples instead of java.io Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

- Change "entityStream" to "content" - Indicate who must close content streams - Update examples to use new method name / close behavior Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Also, add `withFileName` convenience method and document the encoding charset to use when converting a part's content. Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

- Moves `EntityPart` from `ext` to `core` package - Adds new `content` methods to `EntityPart.Builder` that use MBWs - Cleanup of references to old `Part` name updated to `EntityPart` Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

andymc12 · 2021-05-05T14:51:44Z

The latest commit should address @chkal's question - basically allowing code blocks 1 & 2, but not 3. It also allows String and InputStream as @FormParam types. I believe that this was the last concern.

@spericas @chkal @mkarg @pdudits @jansupol @deki - thanks for all of the discussion and reviews! Can I ask that you take another look and approve?

chkal

Looks good!

@andymc12 Thanks a lot for all the adjustments. :-)

andymc12 · 2021-05-17T21:33:16Z

@spericas @mkarg @pdudits @jansupol @deki - I think we're all happy with these changes (if not, please let me know). If one of you could approve this PR, I'll go ahead and merge. Thanks!

kalgon · 2021-05-19T14:15:49Z

Hi, I have a simple remark: can't EntityPart use javax.ws.rs.core.HttpHeaders ?

andymc12 · 2021-05-19T14:58:01Z

Hi, I have a simple remark: can't EntityPart use javax.ws.rs.core.HttpHeaders ?

Not directly, but you can specify headers when building an EntityPart or read them when consuming one - e.g.:

EntityPart spanishPart = EntityPart.withName("spanishData").header(HttpHeaders.CONTENT_LANGUAGE, "es")...

or

String language = entityPart.getHeader(HttpHeaders.CONTENT_LANGUAGE); 
assertEquals("es", language);

mkarg · 2021-05-22T10:07:15Z

@andymc12 Great feature! Thank you for driving this!

andymc12 requested review from chkal, asoldano, ronsigal, mkarg, arjantijms, spericas, sdaschner and jansupol March 4, 2021 20:49

andymc12 self-assigned this Mar 4, 2021

mkarg reviewed Mar 25, 2021

View reviewed changes

spericas reviewed Mar 31, 2021

View reviewed changes

chkal reviewed Apr 2, 2021

View reviewed changes

jaxrs-api/src/main/java/jakarta/ws/rs/ext/Part.java Outdated Show resolved Hide resolved

spericas previously approved these changes Apr 9, 2021

View reviewed changes

chkal reviewed Apr 11, 2021

View reviewed changes

jaxrs-spec/src/main/asciidoc/chapters/resources/_declaring_method_capabilities.adoc Outdated Show resolved Hide resolved

jaxrs-api/src/main/java/jakarta/ws/rs/ext/Part.java Outdated Show resolved Hide resolved

andymc12 dismissed spericas’s stale review via 59620cd April 13, 2021 19:14

spericas previously approved these changes Apr 13, 2021

View reviewed changes

chkal reviewed Apr 14, 2021

View reviewed changes

andymc12 dismissed spericas’s stale review via 82341e9 April 21, 2021 20:58

andymc12 added 10 commits May 5, 2021 08:44

[418] multipart/form-data support

3f48590

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] Review comments

14c002a

- "ex:" to "e.g." - Using modern java.nio.file examples instead of java.io Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] Review comment changes 2

869d055

- Change "entityStream" to "content" - Indicate who must close content streams - Update examples to use new method name / close behavior Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] Rename newBuilder to withName

2eacac0

Also, add `withFileName` convenience method and document the encoding charset to use when converting a part's content. Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] getContent Class/GenericType methods

856ecd2

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] Allow @FormParam(...) AnyType param w/MBR

6e74b3c

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] Renamed Part to EntityPart

35f3ef1

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] Convenience method for content(T)

88e9517

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

[418] @FormParam spec and example changes

b5319b0

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

andymc12 dismissed stale reviews from spericas and chkal via b5319b0 May 5, 2021 14:44

andymc12 force-pushed the 418-multipart-support branch from eb54a83 to b5319b0 Compare May 5, 2021 14:44

chkal approved these changes May 8, 2021

View reviewed changes

deki approved these changes May 18, 2021

View reviewed changes

spericas self-requested a review May 18, 2021 13:14

spericas approved these changes May 18, 2021

View reviewed changes

andymc12 merged commit 48987b9 into jakartaee:master May 18, 2021

dhutchison mentioned this pull request Dec 31, 2021

[BUG] [jaxrs-spec] FormParam used instead of FormDataParam for "multipart/form-data" OpenAPITools/openapi-generator#5957

Open

NicoNes mentioned this pull request Jul 28, 2023

Regression in release-4.0 branch #1165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[418] multipart/form-data support #948

[418] multipart/form-data support #948

andymc12 commented Mar 4, 2021

mkarg commented Mar 5, 2021

andymc12 commented Mar 18, 2021

mkarg commented Mar 19, 2021

mkarg left a comment

andymc12 commented Apr 1, 2021

mkarg commented Apr 1, 2021

chkal left a comment

andymc12 commented Apr 8, 2021

spericas left a comment

chkal left a comment

andymc12 commented Apr 13, 2021

chkal Apr 14, 2021

andymc12 Apr 14, 2021

andymc12 Apr 14, 2021

chkal Apr 24, 2021

pdudits Apr 30, 2021

mkarg May 2, 2021

chkal May 3, 2021

jansupol commented Apr 14, 2021

mkarg commented Apr 15, 2021

andymc12 commented Apr 21, 2021

andymc12 commented Apr 21, 2021

mkarg commented Apr 22, 2021

andymc12 commented May 4, 2021

spericas commented May 5, 2021

andymc12 commented May 5, 2021

chkal left a comment

andymc12 commented May 17, 2021

kalgon commented May 19, 2021

andymc12 commented May 19, 2021

mkarg commented May 22, 2021

[418] multipart/form-data support #948

[418] multipart/form-data support #948

Conversation

andymc12 commented Mar 4, 2021

mkarg commented Mar 5, 2021

andymc12 commented Mar 18, 2021

mkarg commented Mar 19, 2021

mkarg left a comment

Choose a reason for hiding this comment

andymc12 commented Apr 1, 2021

mkarg commented Apr 1, 2021

chkal left a comment

Choose a reason for hiding this comment

andymc12 commented Apr 8, 2021

spericas left a comment

Choose a reason for hiding this comment

chkal left a comment

Choose a reason for hiding this comment

andymc12 commented Apr 13, 2021

chkal Apr 14, 2021

Choose a reason for hiding this comment

andymc12 Apr 14, 2021

Choose a reason for hiding this comment

andymc12 Apr 14, 2021

Choose a reason for hiding this comment

chkal Apr 24, 2021

Choose a reason for hiding this comment

pdudits Apr 30, 2021

Choose a reason for hiding this comment

mkarg May 2, 2021

Choose a reason for hiding this comment

chkal May 3, 2021

Choose a reason for hiding this comment

jansupol commented Apr 14, 2021

mkarg commented Apr 15, 2021

andymc12 commented Apr 21, 2021

andymc12 commented Apr 21, 2021

mkarg commented Apr 22, 2021

andymc12 commented May 4, 2021

spericas commented May 5, 2021

andymc12 commented May 5, 2021

chkal left a comment

Choose a reason for hiding this comment

andymc12 commented May 17, 2021

kalgon commented May 19, 2021

andymc12 commented May 19, 2021

mkarg commented May 22, 2021