Skip to content

Commit 509ec52

Browse files
tomas-langertomas-langer
authored
4.x: Return 500 when 204 with entity is sent from routing. (helidon-io#8357)
* Return 500 when 204 with entity is sent from routing. * Never send entity with status 204 (from our own code) * Updated to include 205 and 304 statuses
1 parent 8c061a2 commit 509ec52

10 files changed

Lines changed: 249 additions & 15 deletions

File tree

‎archetypes/archetypes/src/main/archetype/se/database/files/src/main/java/__pkg__/PokemonService.java.mustache‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -270,10 +270,10 @@ public class PokemonService implements HttpService {
270270
.pathParameters()
271271
.first("id").map(Integer::parseInt)
272272
.orElseThrow(() -> new BadRequestException("No pokemon id"));
273-
long count = dbClient.execute().createNamedDelete("delete-pokemon-by-id")
273+
dbClient.execute().createNamedDelete("delete-pokemon-by-id")
274274
.addParam("id", id)
275275
.execute();
276276
response.status(Status.NO_CONTENT_204)
277-
.send("Deleted: " + count + " values\n");
277+
.send();
278278
}
279279
}

‎examples/dbclient/pokemons/src/main/java/io/helidon/examples/dbclient/pokemons/PokemonService.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2019, 2023 Oracle and/or its affiliates.
2+
* Copyright (c) 2019, 2024 Oracle and/or its affiliates.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -286,10 +286,10 @@ private void deletePokemonById(ServerRequest request, ServerResponse response) {
286286
.pathParameters()
287287
.first("id").map(Integer::parseInt)
288288
.orElseThrow(() -> new BadRequestException("No pokemon id"));
289-
long count = dbClient.execute().createNamedDelete("delete-pokemon-by-id")
289+
dbClient.execute().createNamedDelete("delete-pokemon-by-id")
290290
.addParam("id", id)
291291
.execute();
292292
response.status(Status.NO_CONTENT_204)
293-
.send("Deleted: " + count + " values\n");
293+
.send();
294294
}
295295
}

‎examples/metrics/http-status-count-se/src/test/java/io/helidon/examples/se/httpstatuscount/StatusService.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2022, 2023 Oracle and/or its affiliates.
2+
* Copyright (c) 2022, 2024 Oracle and/or its affiliates.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -13,6 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16+
1617
package io.helidon.examples.se.httpstatuscount;
1718

1819
import io.helidon.http.Status;
@@ -43,6 +44,12 @@ private void respondWithRequestedStatus(ServerRequest request, ServerResponse re
4344
status = Status.INTERNAL_SERVER_ERROR_500.code();
4445
msg = "Unsuccessful conversion";
4546
}
46-
response.status(status).send(msg);
47+
Status httpStatus = Status.create(status);
48+
response.status(status);
49+
if (httpStatus != Status.NO_CONTENT_204) {
50+
response.send(msg);
51+
} else {
52+
response.send();
53+
}
4754
}
4855
}

‎microprofile/tests/server/pom.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,5 +61,10 @@
6161
<artifactId>helidon-microprofile-testing-junit5</artifactId>
6262
<scope>test</scope>
6363
</dependency>
64+
<dependency>
65+
<groupId>io.helidon.logging</groupId>
66+
<artifactId>helidon-logging-jul</artifactId>
67+
<scope>test</scope>
68+
</dependency>
6469
</dependencies>
6570
</project>

‎microprofile/tests/server/src/test/java/io/helidon/microprofile/tests/server/NoContentWithEntityTest.java‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/*
2+
* Copyright (c) 2024 Oracle and/or its affiliates.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.helidon.microprofile.tests.server;
18+
19+
import io.helidon.http.Status;
20+
import io.helidon.microprofile.server.JaxRsCdiExtension;
21+
import io.helidon.microprofile.server.ServerCdiExtension;
22+
import io.helidon.microprofile.testing.junit5.AddBean;
23+
import io.helidon.microprofile.testing.junit5.AddExtension;
24+
import io.helidon.microprofile.testing.junit5.DisableDiscovery;
25+
import io.helidon.microprofile.testing.junit5.HelidonTest;
26+
27+
import jakarta.inject.Inject;
28+
import jakarta.ws.rs.GET;
29+
import jakarta.ws.rs.Path;
30+
import jakarta.ws.rs.client.WebTarget;
31+
import jakarta.ws.rs.core.Response;
32+
import org.glassfish.jersey.ext.cdi1x.internal.CdiComponentProvider;
33+
import org.junit.jupiter.api.Test;
34+
35+
import static org.hamcrest.CoreMatchers.is;
36+
import static org.hamcrest.MatcherAssert.assertThat;
37+
38+
@HelidonTest
39+
@DisableDiscovery
40+
@AddBean(NoContentWithEntityTest.TestResource.class)
41+
@AddExtension(ServerCdiExtension.class)
42+
@AddExtension(JaxRsCdiExtension.class)
43+
@AddExtension(CdiComponentProvider.class)
44+
class NoContentWithEntityTest {
45+
@Inject
46+
WebTarget target;
47+
48+
@Test
49+
void streamingOutput() {
50+
Response response = target.path("/noContent")
51+
.request()
52+
.get();
53+
54+
assertThat(response.getStatus(), is(Status.INTERNAL_SERVER_ERROR_500.code()));
55+
56+
response = target.path("/ok")
57+
.request()
58+
.get();
59+
assertThat(response.getStatus(), is(Status.OK_200.code()));
60+
assertThat(response.readEntity(String.class), is("hello"));
61+
}
62+
63+
@Path("/")
64+
public static class TestResource {
65+
66+
@GET
67+
@Path("/noContent")
68+
public Response noContent() {
69+
return Response.noContent()
70+
.entity("hello")
71+
.build();
72+
}
73+
74+
@GET
75+
@Path("/ok")
76+
public Response ok() {
77+
return Response
78+
.ok("hello")
79+
.build();
80+
}
81+
}
82+
}

‎microprofile/tests/server/src/test/resources/logging-test.properties‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
#
2+
# Copyright (c) 2024 Oracle and/or its affiliates.
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
#
16+
handlers=java.util.logging.ConsoleHandler
17+
java.util.logging.ConsoleHandler.level=FINEST
18+
java.util.logging.ConsoleHandler.formatter=java.util.logging.SimpleFormatter
19+
java.util.logging.SimpleFormatter.format=%1$tH:%1$tM:%1$tS %4$s %3$s %5$s%6$s%n
20+
# Global logging level. Can be overridden by specific loggers
21+
.level=INFO

‎webserver/tests/webserver/pom.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,5 +53,10 @@
5353
<artifactId>hamcrest-all</artifactId>
5454
<scope>test</scope>
5555
</dependency>
56+
<dependency>
57+
<groupId>io.helidon.logging</groupId>
58+
<artifactId>helidon-logging-jul</artifactId>
59+
<scope>test</scope>
60+
</dependency>
5661
</dependencies>
5762
</project>

‎webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/NoContentWithEntityTest.java‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/*
2+
* Copyright (c) 2024 Oracle and/or its affiliates.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.helidon.webserver.tests;
18+
19+
import java.io.IOException;
20+
import java.io.OutputStream;
21+
import java.nio.charset.StandardCharsets;
22+
23+
import io.helidon.http.Status;
24+
import io.helidon.logging.common.LogConfig;
25+
import io.helidon.webclient.api.ClientResponseTyped;
26+
import io.helidon.webclient.http1.Http1Client;
27+
import io.helidon.webclient.http1.Http1ClientResponse;
28+
import io.helidon.webserver.http.HttpRules;
29+
import io.helidon.webserver.http.ServerRequest;
30+
import io.helidon.webserver.http.ServerResponse;
31+
import io.helidon.webserver.testing.junit5.ServerTest;
32+
import io.helidon.webserver.testing.junit5.SetUpRoute;
33+
34+
import org.junit.jupiter.api.Test;
35+
36+
import static org.hamcrest.CoreMatchers.is;
37+
import static org.hamcrest.MatcherAssert.assertThat;
38+
39+
@ServerTest
40+
class NoContentWithEntityTest {
41+
private final Http1Client client;
42+
43+
NoContentWithEntityTest(Http1Client client) {
44+
this.client = client;
45+
}
46+
47+
@SetUpRoute
48+
static void routing(HttpRules rules) {
49+
rules.post("/noContent", NoContentWithEntityTest::noContent)
50+
.post("/noContentStream", NoContentWithEntityTest::noContentStream)
51+
.post("/ok", NoContentWithEntityTest::ok);
52+
53+
LogConfig.configureRuntime();
54+
}
55+
56+
@Test
57+
void testNoContentWorksFollowedByOk() {
58+
try (Http1ClientResponse response = client.post("/noContent")
59+
.submit("text")) {
60+
61+
assertThat(response.status(), is(Status.INTERNAL_SERVER_ERROR_500));
62+
}
63+
64+
ClientResponseTyped<String> okResponse = client.post("/ok")
65+
.submit("text", String.class);
66+
67+
assertThat(okResponse.status(), is(Status.OK_200));
68+
assertThat(okResponse.entity(), is("text"));
69+
}
70+
71+
@Test
72+
void testNoContentWorksFollowedByOkStreaming() {
73+
try (Http1ClientResponse response = client.post("/noContentStream")
74+
.submit("text")) {
75+
76+
assertThat(response.status(), is(Status.INTERNAL_SERVER_ERROR_500));
77+
}
78+
79+
ClientResponseTyped<String> okResponse = client.post("/ok")
80+
.submit("text", String.class);
81+
82+
assertThat(okResponse.status(), is(Status.OK_200));
83+
assertThat(okResponse.entity(), is("text"));
84+
}
85+
86+
private static void ok(ServerRequest req, ServerResponse res) {
87+
res.status(Status.OK_200).send("text");
88+
}
89+
90+
private static void noContentStream(ServerRequest req, ServerResponse res) throws IOException {
91+
res.status(Status.NO_CONTENT_204);
92+
try (OutputStream out = res.outputStream()) {
93+
out.write("text".getBytes(StandardCharsets.UTF_8));
94+
}
95+
}
96+
97+
private static void noContent(ServerRequest req, ServerResponse res) {
98+
res.status(Status.NO_CONTENT_204).send("text");
99+
}
100+
101+
}
102+

‎webserver/tests/webserver/src/test/resources/logging-test.properties‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#
2-
# Copyright (c) 2022, 2023 Oracle and/or its affiliates.
2+
# Copyright (c) 2022, 2024 Oracle and/or its affiliates.
33
#
44
# Licensed under the Apache License, Version 2.0 (the "License");
55
# you may not use this file except in compliance with the License.
@@ -19,8 +19,3 @@ java.util.logging.ConsoleHandler.formatter=java.util.logging.SimpleFormatter
1919
java.util.logging.SimpleFormatter.format=%1$tH:%1$tM:%1$tS %4$s %3$s %5$s%6$s%n
2020
# Global logging level. Can be overridden by specific loggers
2121
.level=INFO
22-
io.helidon.webserver.level=INFO
23-
io.helidon.webserver.http.Http1Connection.level=INFO
24-
io.helidon.webserver.http.HttpRouting.level=INFO
25-
io.helidon.common.testing.http.junit5.level=INFO
26-

‎webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2022, 2023 Oracle and/or its affiliates.
2+
* Copyright (c) 2022, 2024 Oracle and/or its affiliates.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -106,8 +106,25 @@ static void nonEntityBytes(ServerResponseHeaders headers,
106106
boolean keepAlive,
107107
boolean validateHeaders) {
108108

109+
status = status == null ? Status.OK_200 : status;
110+
111+
if (status.code() == Status.NO_CONTENT_204.code()
112+
|| status.code() == Status.RESET_CONTENT_205.code()
113+
|| status.code() == Status.NOT_MODIFIED_304.code()) {
114+
// https://www.rfc-editor.org/rfc/rfc9110#status.204
115+
// A 204 response is terminated by the end of the header section; it cannot contain content or trailers
116+
// ditto for 205, and 304
117+
if ((headers.contains(HeaderNames.CONTENT_LENGTH) && !headers.contains(HeaderValues.CONTENT_LENGTH_ZERO))
118+
|| headers.contains(HeaderValues.TRANSFER_ENCODING_CHUNKED)) {
119+
status = Status.INTERNAL_SERVER_ERROR_500;
120+
LOGGER.log(System.Logger.Level.ERROR, "Attempt to send status " + status.text() + " with entity."
121+
+ " Server responded with Internal Server Error. Please fix your routing, this is not allowed "
122+
+ "by HTTP specification, such responses MUST NOT contain an entity.");
123+
}
124+
}
125+
109126
// first write status
110-
if (status == null || status == Status.OK_200) {
127+
if (status == Status.OK_200) {
111128
buffer.write(OK_200);
112129
} else {
113130
buffer.write(HTTP_BYTES);

0 commit comments

Comments
 (0)