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.

Sign up for GitHub

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

Jump to bottom

Expand types which are considered text in multipart handling #38810

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Expand types which are considered text in multipart handling #38810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions .../io/quarkus/resteasy/reactive/server/test/multipart/MultipartTextWithoutFilenameTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package io.quarkus.resteasy.reactive.server.test.multipart;

import static io.restassured.RestAssured.given;

import java.io.IOException;
import java.util.function.Supplier;

import jakarta.ws.rs.FormParam;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.Path;

import org.jboss.resteasy.reactive.RestResponse;
import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;

public class MultipartTextWithoutFilenameTest {

@RegisterExtension
static QuarkusUnitTest test = new QuarkusUnitTest()
.setArchiveProducer(new Supplier<>() {
@Override
public JavaArchive get() {
return ShrinkWrap.create(JavaArchive.class)
.addClasses(Resource.class);
}
});

@Test
public void test() throws IOException {
given()
.contentType("multipart/form-data")
.multiPart("firstParam", "{\"id\":\"myId\",\"name\":\"myName\"}", "application/json")
.when()
.post("/test")
.then()
.statusCode(200);
}

@Path("/test")
public static class Resource {

@POST
public RestResponse<Void> testMultipart(@FormParam("firstParam") final String firstParam,
@FormParam("secondParam") final String secondParam) {
return RestResponse.ok();
}
}
}
8 changes: 5 additions & 3 deletions ...common/runtime/src/main/java/org/jboss/resteasy/reactive/common/util/MediaTypeHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,10 +267,12 @@ public static boolean equivalentParams(MediaType m1, MediaType m2) {
return true;
}

// TODO: does this need to be more complex?
public static boolean isTextLike(MediaType mediaType) {
return "text".equalsIgnoreCase(mediaType.getType())
|| ("application".equalsIgnoreCase(mediaType.getType())
&& mediaType.getSubtype().toLowerCase().startsWith("xml"));
String type = mediaType.getType();
String subtype = mediaType.getSubtype();
return (type.equals("application") && (subtype.contains("json") || subtype.contains("xml") || subtype.contains("yaml")))
|| type.equals("text");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I wonder if there's any more formal definition of what can be turned into a text.

Also, I wonder how this all compares to "toplevel" body parsing.

In theory, multipart members and body entities should now have the same parsing mechanism, in that they each have an encoding, content type, and go via MessageBodyReader for deserialisation, which is what allows us to have not just primitive types but also JSON/XML deserialisation.

This logic appears to sit before we look up the right MessageBodyReader for the deserialisation. I wonder why we have this. Perhaps everything at this point should be read as byte[] (well, and associated with their content-type), and then later when we figure out whether to deserialise this into a String or JsonObject or Person entity, we use the same binary data?

Here we're accepting "deserialising" XML/JSON/YAML into String which means: no deserialisation, just give them "raw" (still respecting the encoding). For a mime part. Do we accept the same for a toplevel http request body? I think we do. But I wonder if we also limit it there for XML/JSON/YAML or if we have another sort of check of text-like, or… "String-convertible".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole thing is weird and this is only needed to try to decide what type of part of this is (see calls to FormData#add).
I really don't like what we are doing, but I don't have any ideas to make it better.

}

public static boolean isUnsupportedWildcardSubtype(MediaType mediaType) {
Expand Down
9 changes: 2 additions & 7 deletions ...erver/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/EncodedMediaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import jakarta.ws.rs.core.MediaType;

import org.jboss.resteasy.reactive.common.util.MediaTypeHelper;
import org.jboss.resteasy.reactive.server.spi.ContentType;

/**
Expand All @@ -21,7 +22,7 @@ public EncodedMediaType(MediaType mediaType) {
MediaType effectiveMediaType = mediaType;
String effectiveCharset;
String originalCharset = mediaType.getParameters().get("charset");
if (isStringMediaType(mediaType)) {
if (MediaTypeHelper.isTextLike(mediaType)) {
effectiveCharset = originalCharset;
if (effectiveCharset == null) {
effectiveCharset = StandardCharsets.UTF_8.name();
Expand All @@ -38,12 +39,6 @@ public EncodedMediaType(MediaType mediaType) {
}

// TODO: does this need to be more complex?
private boolean isStringMediaType(MediaType mediaType) {
String type = mediaType.getType();
String subtype = mediaType.getSubtype();
return (type.equals("application") && (subtype.contains("json") || subtype.contains("xml") || subtype.contains("yaml")))
|| type.equals("text");
}

@Override
public String toString() {
Expand Down
3 changes: 2 additions & 1 deletion ...ain/java/org/jboss/resteasy/reactive/server/core/multipart/MultiPartParserDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.jboss.logging.Logger;
import org.jboss.resteasy.reactive.common.headers.HeaderUtil;
import org.jboss.resteasy.reactive.common.util.CaseInsensitiveMap;
import org.jboss.resteasy.reactive.common.util.MediaTypeHelper;
import org.jboss.resteasy.reactive.server.core.ResteasyReactiveRequestContext;
import org.jboss.resteasy.reactive.server.spi.ServerHttpRequest;

Expand Down Expand Up @@ -366,7 +367,7 @@ private boolean isText(String contentType) {
if (contentType == null || contentType.isEmpty()) { // https://www.rfc-editor.org/rfc/rfc7578.html#section-4.4 says the default content-type if missing is text/plain
return true;
}
return MediaType.TEXT_PLAIN_TYPE.isCompatible(MediaType.valueOf(contentType));
return MediaTypeHelper.isTextLike(MediaType.valueOf(contentType));
}

public List<Path> getCreatedFiles() {
Expand Down
Loading