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

Jetty 12.1.x tck error message #12011

Merged
merged 4 commits into from
Jul 8, 2024
Merged

Jetty 12.1.x tck error message #12011

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a06a60
Fix for bad error message in tck test
janbartel Jun 25, 2024
d5865e6
Merge remote-tracking branch 'origin/jetty-12.1.x' into jetty-12.1.x-…
janbartel Jun 25, 2024
748dcf9
Merge remote-tracking branch 'origin/jetty-12.1.x' into jetty-12.1.x-…
janbartel Jun 25, 2024
25972ec
Add TODO and change param names
janbartel Jul 8, 2024
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
18 changes: 13 additions & 5 deletions jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,14 @@ public boolean errorPageForMethod(String method)
};
}

private static Object getRequestErrorAttribute(HttpServletRequest request, String servletApiName, String jettyName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this method necessary?
I thought the policy was that we would always set the jetty(core) attribute and then any request wrapper used for an error dispatch to a filter/servlet would intercept the servlet attributes and lookup the jetty ones

So our code should then only ever need to lookup jetty(core) attributes? Unless there is a case where an application sets them and we are expected to act on them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is part of the refactoring effort, which is a much wider scope than just satisfying the tck.

{
Object o = request.getAttribute(servletApiName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Object o = request.getAttribute(servletApiName);
Object o = request.getAttribute(servletAttributeName);

if (o == null)
o = request.getAttribute(jettyName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
o = request.getAttribute(jettyName);
o = request.getAttribute(coreAttributeName);

return o;
}

@Override
public boolean handle(Request request, Response response, Callback callback) throws Exception
{
Expand Down Expand Up @@ -126,7 +134,7 @@ public boolean handle(Request request, Response response, Callback callback) thr
}
}

String message = (String)request.getAttribute(Dispatcher.ERROR_MESSAGE);
String message = (String)getRequestErrorAttribute(httpServletRequest, Dispatcher.ERROR_MESSAGE, org.eclipse.jetty.server.handler.ErrorHandler.ERROR_MESSAGE);
if (message == null)
message = HttpStatus.getMessage(response.getStatus());
generateAcceptableResponse(servletContextRequest, httpServletRequest, httpServletResponse, response.getStatus(), message);
Expand Down Expand Up @@ -416,7 +424,7 @@ protected void writeErrorPageMessage(HttpServletRequest request, Writer writer,
{
htmlRow(writer, "SERVLET", request.getAttribute(Dispatcher.ERROR_SERVLET_NAME));
}
Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Throwable cause = (Throwable)getRequestErrorAttribute(request, Dispatcher.ERROR_EXCEPTION, org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
while (cause != null)
{
htmlRow(writer, "CAUSED BY", cause);
Expand Down Expand Up @@ -451,7 +459,7 @@ protected void writeErrorPlain(HttpServletRequest request, PrintWriter writer, i
{
writer.printf("SERVLET: %s%n", request.getAttribute(Dispatcher.ERROR_SERVLET_NAME));
}
Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Throwable cause = (Throwable)getRequestErrorAttribute(request, Dispatcher.ERROR_EXCEPTION, org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
while (cause != null)
{
writer.printf("CAUSED BY %s%n", cause);
Expand All @@ -465,7 +473,7 @@ protected void writeErrorPlain(HttpServletRequest request, PrintWriter writer, i

protected void writeErrorJson(HttpServletRequest request, PrintWriter writer, int code, String message)
{
Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
Throwable cause = (Throwable)getRequestErrorAttribute(request, Dispatcher.ERROR_EXCEPTION, org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
Object servlet = request.getAttribute(Dispatcher.ERROR_SERVLET_NAME);
Map<String, String> json = new HashMap<>();

Expand All @@ -490,7 +498,7 @@ protected void writeErrorJson(HttpServletRequest request, PrintWriter writer, in

protected void writeErrorPageStacks(HttpServletRequest request, Writer writer) throws IOException
{
Throwable th = (Throwable)request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
Throwable th = (Throwable)getRequestErrorAttribute(request, RequestDispatcher.ERROR_EXCEPTION, org.eclipse.jetty.server.handler.ErrorHandler.ERROR_EXCEPTION);
if (th != null)
{
writer.write("<h3>Caused by:</h3><pre>");
Expand Down
39 changes: 39 additions & 0 deletions jetty-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/ResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package org.eclipse.jetty.ee11.servlet;

import java.io.IOException;
import java.io.PrintWriter;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand All @@ -22,6 +23,7 @@
import java.util.stream.Stream;

import jakarta.servlet.ServletException;
import jakarta.servlet.http.Cookie;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
Expand Down Expand Up @@ -105,6 +107,43 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
assertThat(response.getContent(), containsString("Hello"));
}

@Test
public void testErrorWithMessage() throws Exception
{
ServletContextHandler contextHandler = new ServletContextHandler();
contextHandler.setErrorHandler(new ErrorPageErrorHandler());
contextHandler.setContextPath("/");
HttpServlet servlet = new HttpServlet()
{
@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{
PrintWriter pw = response.getWriter();
pw.println("THIS TEXT SHOULD NOT APPEAR");
response.addHeader("header", "sendError_StringTest");
response.addCookie(new Cookie("cookie1", "value1"));
response.sendError(HttpServletResponse.SC_GONE, "The content is gone.");
}
};

contextHandler.addServlet(servlet, "/error/*");
startServer(contextHandler);

HttpTester.Request request = new HttpTester.Request();
request.setMethod("GET");
request.setURI("/error/");
request.setVersion(HttpVersion.HTTP_1_1);
request.setHeader("Connection", "close");
request.setHeader("Host", "test");

ByteBuffer responseBuffer = _connector.getResponse(request.generate());
HttpTester.Response response = HttpTester.parseResponse(responseBuffer);

assertThat(response.getStatus(), is(410));
assertThat(response.get("Content-Type"), is("text/html;charset=ISO-8859-1"));
assertThat(response.getContent(), containsString("The content is gone."));
}

public static Stream<Arguments> redirects()
{
List<Arguments> cases = new ArrayList<>();
Expand Down
Loading