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

Implement Servlet 6.1 Error dispatch changes #11821

Merged
merged 4 commits into from
May 23, 2024
Merged

Implement Servlet 6.1 Error dispatch changes #11821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83e2e07
Implement Servlet 6.1 Error dispatch changes
gregw May 20, 2024
abd40ed
updates from review
gregw May 21, 2024
f116fb3
Update jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/…
gregw May 22, 2024
9243b91
Merge branch 'jetty-12.1.x' into fix/jetty-12.1.x/errorDispatch
gregw May 22, 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
43 changes: 38 additions & 5 deletions jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/Dispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,11 @@
import jakarta.servlet.http.HttpServletResponse;
import jakarta.servlet.http.HttpServletResponseWrapper;
import org.eclipse.jetty.ee11.servlet.util.ServletOutputStreamWrapper;
import org.eclipse.jetty.http.HttpMethod;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.pathmap.MatchedResource;
import org.eclipse.jetty.io.WriterOutputStream;
import org.eclipse.jetty.server.handler.ErrorHandler;
import org.eclipse.jetty.util.Fields;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.StringUtil;
Expand Down Expand Up @@ -765,9 +767,12 @@ public Enumeration<String> getAttributeNames()

private class ErrorRequest extends ParameterRequestWrapper
{
public ErrorRequest(HttpServletRequest httpRequest)
private final HttpServletRequest _httpServletRequest;

public ErrorRequest(HttpServletRequest httpServletRequest)
{
super(httpRequest);
super(httpServletRequest);
_httpServletRequest = Objects.requireNonNull(httpServletRequest);
}

@Override
Expand All @@ -776,16 +781,22 @@ public DispatcherType getDispatcherType()
return DispatcherType.ERROR;
}

@Override
public String getMethod()
{
return HttpMethod.GET.asString();
}

@Override
public String getPathInfo()
{
return _servletPathMapping.getPathInfo();
return Objects.requireNonNull(_servletPathMapping).getPathInfo();
}

@Override
public String getServletPath()
{
return _servletPathMapping.getServletPath();
return Objects.requireNonNull(_servletPathMapping).getServletPath();
}

@Override
Expand All @@ -797,7 +808,7 @@ public HttpServletMapping getHttpServletMapping()
@Override
public String getRequestURI()
{
return _uri.getPath();
return Objects.requireNonNull(_uri).getPath();
}

@Override
Expand All @@ -809,6 +820,28 @@ public StringBuffer getRequestURL()
.port(getServerPort())
.asString());
}

@Override
public Object getAttribute(String name)
{
return switch (name)
{
case ERROR_METHOD -> _httpServletRequest.getMethod();
case ERROR_REQUEST_URI -> _httpServletRequest.getRequestURI();
case ERROR_QUERY_STRING -> _httpServletRequest.getQueryString();
case ERROR_STATUS_CODE -> super.getAttribute(ErrorHandler.ERROR_STATUS);
case ERROR_MESSAGE -> super.getAttribute(ErrorHandler.ERROR_MESSAGE);
default -> super.getAttribute(name);
};
}

@Override
public Enumeration<String> getAttributeNames()
{
List<String> names = new ArrayList<>(List.of(ERROR_METHOD, ERROR_REQUEST_URI, ERROR_QUERY_STRING, ERROR_STATUS_CODE, ERROR_MESSAGE));
names.addAll(Collections.list(super.getAttributeNames()));
return Collections.enumeration(names);
}
}

@Override
Expand Down
3 changes: 1 addition & 2 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 @@ -50,9 +50,8 @@

public class ErrorHandler implements Request.Handler
{
// TODO This classes API needs to be majorly refactored/cleanup in jetty-10
// TODO This class's API needs to be majorly refactored/cleanup
private static final Logger LOG = LoggerFactory.getLogger(ErrorHandler.class);
public static final String ERROR_CONTEXT = "org.eclipse.jetty.server.error_context";
public static final String ERROR_CHARSET = "org.eclipse.jetty.server.error_charset";

boolean _showServlet = true;
Expand Down
4 changes: 3 additions & 1 deletion ...etty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ErrorPageErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS;

/**
* An ErrorHandler that maps exceptions and status codes to URIs for dispatch using
* the internal ERROR style of dispatch.
Expand Down Expand Up @@ -108,7 +110,7 @@ public String getErrorPage(HttpServletRequest request)
pageSource = PageLookupTechnique.STATUS_CODE;

// look for an exact code match
errorStatusCode = (Integer)request.getAttribute(Dispatcher.ERROR_STATUS_CODE);
errorStatusCode = (Integer)request.getAttribute(ERROR_STATUS);
if (errorStatusCode != null)
{
errorPage = _errorPages.get(Integer.toString(errorStatusCode));
Expand Down
21 changes: 12 additions & 9 deletions ...11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,19 +843,22 @@ public Object getAttribute(String name)
@Override
public Enumeration<String> getAttributeNames()
{
Set<String> set = getRequest().getAttributeNameSet();
if (_async != null)
{
set = new HashSet<>(set);
set.add(AsyncContext.ASYNC_REQUEST_URI);
set.add(AsyncContext.ASYNC_CONTEXT_PATH);
set.add(AsyncContext.ASYNC_SERVLET_PATH);
set.add(AsyncContext.ASYNC_PATH_INFO);
set.add(AsyncContext.ASYNC_QUERY_STRING);
set.add(AsyncContext.ASYNC_MAPPING);
Set<String> names = new HashSet<>(Set.of(

AsyncContext.ASYNC_REQUEST_URI,
AsyncContext.ASYNC_CONTEXT_PATH,
AsyncContext.ASYNC_SERVLET_PATH,
AsyncContext.ASYNC_PATH_INFO,
AsyncContext.ASYNC_QUERY_STRING,
AsyncContext.ASYNC_MAPPING
));
names.addAll(getRequest().getAttributeNameSet());
return Collections.enumeration(names);
}

return Collections.enumeration(set);
return Collections.enumeration(getRequest().getAttributeNameSet());
}

@Override
Expand Down
7 changes: 4 additions & 3 deletions ...-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static org.eclipse.jetty.server.handler.ErrorHandler.ERROR_CONTEXT;
import static org.eclipse.jetty.util.thread.Invocable.InvocationType.NON_BLOCKING;

/**
Expand Down Expand Up @@ -459,7 +460,7 @@ public void handle()

// the following is needed as you cannot trust the response code and reason
// as those could have been modified after calling sendError
Integer code = (Integer)_servletContextRequest.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
Integer code = (Integer)_servletContextRequest.getAttribute(org.eclipse.jetty.server.handler.ErrorHandler.ERROR_STATUS);
gregw marked this conversation as resolved.
Show resolved Hide resolved
if (code == null)
code = HttpStatus.INTERNAL_SERVER_ERROR_500;
getServletContextResponse().setStatus(code);
Expand All @@ -472,7 +473,7 @@ public void handle()
if (!_httpInput.consumeAvailable())
ResponseUtils.ensureNotPersistent(_servletContextRequest, _servletContextRequest.getServletContextResponse());

ContextHandler.ScopedContext context = (ContextHandler.ScopedContext)_servletContextRequest.getAttribute(ErrorHandler.ERROR_CONTEXT);
ContextHandler.ScopedContext context = (ContextHandler.ScopedContext)_servletContextRequest.getAttribute(ERROR_CONTEXT);
Request.Handler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler());

// If we can't have a body or have no ErrorHandler, then create a minimal error response.
Expand Down Expand Up @@ -527,7 +528,7 @@ public void handle()
finally
{
// clean up the context that was set in Response.sendError
_servletContextRequest.removeAttribute(ErrorHandler.ERROR_CONTEXT);
_servletContextRequest.removeAttribute(ERROR_CONTEXT);
}
break;
}
Expand Down
8 changes: 1 addition & 7 deletions .../jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletChannelState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,7 @@

import static jakarta.servlet.RequestDispatcher.ERROR_EXCEPTION;
import static jakarta.servlet.RequestDispatcher.ERROR_EXCEPTION_TYPE;
import static jakarta.servlet.RequestDispatcher.ERROR_MESSAGE;
import static jakarta.servlet.RequestDispatcher.ERROR_REQUEST_URI;
import static jakarta.servlet.RequestDispatcher.ERROR_SERVLET_NAME;
import static jakarta.servlet.RequestDispatcher.ERROR_STATUS_CODE;

/**
* holder of the state of request-response cycle.
Expand Down Expand Up @@ -1039,11 +1036,8 @@ public void sendError(int code, String message)
response.setStatus(code);
servletContextRequest.errorClose();

request.setAttribute(org.eclipse.jetty.ee11.servlet.ErrorHandler.ERROR_CONTEXT, servletContextRequest.getErrorContext());
request.setAttribute(ERROR_REQUEST_URI, httpServletRequest.getRequestURI());
request.setAttribute(ERROR_SERVLET_NAME, servletContextRequest.getServletName());
request.setAttribute(ERROR_STATUS_CODE, code);
request.setAttribute(ERROR_MESSAGE, message);
// Additional servlet error attributes are provided in org.eclipse.jetty.ee11.servlet.Dispatcher.ErrorRequest

// Set Jetty Specific Attributes.
request.setAttribute(ErrorHandler.ERROR_CONTEXT, servletContextRequest.getServletContext());
Expand Down
60 changes: 59 additions & 1 deletion ...y-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/ErrorPageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
assertThat(body, containsString("ERROR_EXCEPTION: null"));
assertThat(body, containsString("ERROR_EXCEPTION_TYPE: null"));
assertThat(body, containsString("ERROR_SERVLET: " + errorContentServlet.getClass().getName()));
assertThat(body, containsString("ERROR_METHOD: GET"));
assertThat(body, containsString("ERROR_REQUEST_URI: /error-mime-charset-writer/"));
}

Expand Down Expand Up @@ -734,6 +735,61 @@ protected void doGet(HttpServletRequest req, HttpServletResponse response) throw
assertThat(responseBody, Matchers.containsString("getParameterMap()[ ]=[]"));
}

@Test
public void testErrorAttributes() throws Exception
{
ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS);
contextHandler.setContextPath("/");

HttpServlet failServlet = new HttpServlet()
{
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse response) throws IOException
{
response.sendError(599);
}

@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
{
doGet(req, resp);
}
};

contextHandler.addServlet(failServlet, "/fail/599");
contextHandler.addServlet(ErrorDumpServlet.class, "/error/*");

ErrorPageErrorHandler errorPageErrorHandler = new ErrorPageErrorHandler();
errorPageErrorHandler.addErrorPage(599, "/error/599");
contextHandler.setErrorHandler(errorPageErrorHandler);

startServer(contextHandler);

String rawRequest = """
POST /fail/599?name=value HTTP/1.1\r
Host: test\r
Connection: close\r
\r
""";

String rawResponse = _connector.getResponse(rawRequest);

HttpTester.Response response = HttpTester.parseResponse(rawResponse);
assertThat(response.getStatus(), is(599));
assertThat(response.get(HttpHeader.DATE), notNullValue());

String responseBody = response.getContent();

assertThat(responseBody, Matchers.containsString("ERROR_PAGE: /599"));
assertThat(responseBody, Matchers.containsString("ERROR_CODE: 599"));
assertThat(responseBody, Matchers.containsString("ERROR_EXCEPTION: null"));
assertThat(responseBody, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
assertThat(responseBody, Matchers.containsString("ERROR_SERVLET: " + failServlet.getClass().getName()));
assertThat(responseBody, Matchers.containsString("ERROR_METHOD: POST"));
assertThat(responseBody, Matchers.containsString("ERROR_REQUEST_URI: /fail/599"));
assertThat(responseBody, Matchers.containsString("ERROR_QUERY_STRING: name=value"));
}

@Test
public void testErrorCode() throws Exception
{
Expand Down Expand Up @@ -1733,7 +1789,7 @@ protected void service(HttpServletRequest req, HttpServletResponse resp)
public static class ErrorDumpServlet extends HttpServlet
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{
if (request.getDispatcherType() != DispatcherType.ERROR && request.getDispatcherType() != DispatcherType.ASYNC)
throw new IllegalStateException("Bad Dispatcher Type " + request.getDispatcherType());
Expand All @@ -1746,7 +1802,9 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
writer.println("ERROR_EXCEPTION: " + request.getAttribute(Dispatcher.ERROR_EXCEPTION));
writer.println("ERROR_EXCEPTION_TYPE: " + request.getAttribute(Dispatcher.ERROR_EXCEPTION_TYPE));
writer.println("ERROR_SERVLET: " + request.getAttribute(Dispatcher.ERROR_SERVLET_NAME));
writer.println("ERROR_METHOD: " + request.getAttribute(Dispatcher.ERROR_METHOD));
writer.println("ERROR_REQUEST_URI: " + request.getAttribute(Dispatcher.ERROR_REQUEST_URI));
writer.println("ERROR_QUERY_STRING: " + request.getAttribute(Dispatcher.ERROR_QUERY_STRING));

writer.printf("getRequestURI()=%s%n", valueOf(request.getRequestURI()));
writer.printf("getRequestURL()=%s%n", valueOf(request.getRequestURL()));
Expand Down
Loading