Skip to content

Commit

Permalink
Implement Servlet 6.1 Error dispatch changes (#11821)
Browse files Browse the repository at this point in the history 
Implement Servlet 6.1 Error dispatch changes

+ Error dispatches are done as GET methods
+ New ERROR_METHOD and ERROR_QUERY_STRING attributes
+ Moved most error attributes to Dispatcher.ErrorRequest
+ removed duplicate attributes

* updates from review

* Update jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ErrorHandler.java

Co-authored-by: Jan Bartel <janb@webtide.com>
  • Loading branch information
@gregw @janbartel
gregw and janbartel authored May 23, 2024
1 parent c10df2d commit 1e95caa
Show file tree
Hide file tree
Showing 7 changed files with 118 additions and 28 deletions.
43 changes: 38 additions & 5 deletions jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/Dispatcher.java
View file
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
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
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
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
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);
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
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
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

0 comments on commit 1e95caa

Please sign in to comment.