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

Prevent creation of the Response object by ContainerResponseContext #45798

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −32
Draft

Prevent creation of the Response object by ContainerResponseContext #45798

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
14 changes: 13 additions & 1 deletion ...oyment/src/test/java/io/quarkus/resteasy/reactive/server/test/providers/FileTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
import java.nio.file.Path;
import java.nio.file.Paths;

import jakarta.ws.rs.container.ContainerResponseContext;
import jakarta.ws.rs.core.HttpHeaders;

import org.hamcrest.Matchers;
import org.jboss.resteasy.reactive.FilePart;
import org.jboss.resteasy.reactive.PathPart;
import org.jboss.resteasy.reactive.server.ServerResponseFilter;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
Expand All @@ -30,7 +32,7 @@ public class FileTestCase {
@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(FileResource.class, WithWriterInterceptor.class, WriterInterceptor.class));
.addClasses(FileResource.class, WithWriterInterceptor.class, WriterInterceptor.class, Filters.class));

@Test
public void testFiles() throws Exception {
Expand Down Expand Up @@ -168,4 +170,14 @@ public void testChecks() throws IOException {
} catch (IllegalArgumentException x) {
}
}

public static class Filters {

@ServerResponseFilter
public void responseHeaders(ContainerResponseContext responseContext) {
// make sure the use of these methods does not change the response code
responseContext.hasEntity();
responseContext.getEntity();
}
}
}
10 changes: 10 additions & 0 deletions ...ve/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/LazyResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ public interface LazyResponse {
*/
Response get();

/**
* Gets the response, but makes change to the state of the object
*/
Response transientGet();

/**
*
* @return <code>true</code> if the response already exists
Expand Down Expand Up @@ -37,6 +42,11 @@ public Response get() {
return response;
}

@Override
public Response transientGet() {
return response;
}

@Override
public boolean isCreated() {
return true;
Expand Down
67 changes: 38 additions & 29 deletions ...er/runtime/src/main/java/org/jboss/resteasy/reactive/server/handlers/ResponseHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,42 +139,51 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti

Response response;

public Response create() {
ResponseBuilderImpl responseBuilder;
if (result instanceof GenericEntity) {
GenericEntity<?> genericEntity = (GenericEntity<?>) result;
requestContext.setGenericReturnType(genericEntity.getType());
responseBuilder = ResponseBuilderImpl.ok(genericEntity.getEntity());
} else if (result == null) {
// FIXME: custom status codes depending on method?
responseBuilder = ResponseBuilderImpl.noContent();
} else {
// FIXME: custom status codes depending on method?
responseBuilder = ResponseBuilderImpl.ok(result);
}
if (responseBuilder.getEntity() != null) {
EncodedMediaType produces = requestContext.getResponseContentType();
if (produces != null) {
responseBuilder.header(HttpHeaders.CONTENT_TYPE, produces.toString());
}
}
if (!responseBuilderCustomizers.isEmpty()) {
for (int i = 0; i < responseBuilderCustomizers.size(); i++) {
responseBuilderCustomizers.get(i).customize(responseBuilder);
}
}
if ((responseBuilder instanceof ResponseBuilderImpl)) {
// avoid unnecessary copying of HTTP headers from the Builder to the Response
return ((ResponseBuilderImpl) responseBuilder).build(false);
} else {
return responseBuilder.build();
}
}

@Override
public Response get() {
if (response == null) {
ResponseBuilderImpl responseBuilder;
if (result instanceof GenericEntity) {
GenericEntity<?> genericEntity = (GenericEntity<?>) result;
requestContext.setGenericReturnType(genericEntity.getType());
responseBuilder = ResponseBuilderImpl.ok(genericEntity.getEntity());
} else if (result == null) {
// FIXME: custom status codes depending on method?
responseBuilder = ResponseBuilderImpl.noContent();
} else {
// FIXME: custom status codes depending on method?
responseBuilder = ResponseBuilderImpl.ok(result);
}
if (responseBuilder.getEntity() != null) {
EncodedMediaType produces = requestContext.getResponseContentType();
if (produces != null) {
responseBuilder.header(HttpHeaders.CONTENT_TYPE, produces.toString());
}
}
if (!responseBuilderCustomizers.isEmpty()) {
for (int i = 0; i < responseBuilderCustomizers.size(); i++) {
responseBuilderCustomizers.get(i).customize(responseBuilder);
}
}
if ((responseBuilder instanceof ResponseBuilderImpl)) {
// avoid unnecessary copying of HTTP headers from the Builder to the Response
response = ((ResponseBuilderImpl) responseBuilder).build(false);
} else {
response = responseBuilder.build();
}
response = create();
}
return response;
}

@Override
public Response transientGet() {
return create();
}

@Override
public boolean isCreated() {
return response != null;
Expand Down
8 changes: 6 additions & 2 deletions .../src/main/java/org/jboss/resteasy/reactive/server/jaxrs/ContainerResponseContextImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,18 @@ public Builder getLinkBuilder(String relation) {
return response().getLinkBuilder(relation);
}

protected Response transientResponse() {
return context.getResponse().transientGet();
}

@Override
public boolean hasEntity() {
return response().hasEntity();
return transientResponse().hasEntity();
}

@Override
public Object getEntity() {
return response().getEntity();
return transientResponse().getEntity();
Copy link
Contributor

Choose a reason for hiding this comment

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

The tricky part is that it not only affects the hasEntity() or getEntity() methods, it affects every method that calls response() LazyResponse, eg. I face this issue with getHeaders().

While debugging this, I found this comment:

quarkus/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/handlers/ResponseHandler.java

Lines 154 to 155 in 3a27b80

// FIXME: custom status codes depending on method?
responseBuilder = ResponseBuilderImpl.ok(result);

Seems that here is where the status code is set to 200.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eg. I face this issue with getHeaders().

Yes I know, hence the draft status of the PR

}

@Override
Expand Down
Loading