Merge pull request #3331 from eclipse/jetty-10.0.x-3225-sendErrorNoReason

Issue #3225 sendError does not set reason.

jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRule.java

@@ -30,23 +30,23 @@ import org.eclipse.jetty.util.annotation.Name;
  */
 public class ResponsePatternRule extends PatternRule
 {
-    private String _code;
+    private int _code;
-    private String _reason;
+    private String _message;
 
     /* ------------------------------------------------------------ */
     public ResponsePatternRule()
     {
-        this(null,null,"");
+        this(null,null,null);
     }
 
     /* ------------------------------------------------------------ */
-    public ResponsePatternRule(@Name("pattern") String pattern, @Name("code") String code, @Name("reason") String reason)
+    public ResponsePatternRule(@Name("pattern") String pattern, @Name("code") String code, @Name("message") String message)
     {
         super(pattern);
         _handling = true;
         _terminating = true;
         setCode(code);
-        setReason(reason);
+        setMessage(message);
     }
 
     /* ------------------------------------------------------------ */
@@ -56,19 +56,34 @@ public class ResponsePatternRule extends PatternRule
      */
     public void setCode(String code)
     {
-        _code = code;
+        _code = code==null ? 0 : Integer.parseInt(code);
     }
 
     /* ------------------------------------------------------------ */
     /**
      * Sets the reason for the response status code. Reasons will only reflect
      * if the code value is greater or equal to 400.
-     * 
+     * @deprecated Reason has been replaced by message
      * @param reason the reason
+     * @see #setMessage(String)
      */
+    @Deprecated
     public void setReason(String reason)
     {
-        _reason = reason;
+        setMessage(reason);
+    }
+
+    /* ------------------------------------------------------------ */
+    /**
+     * Sets the message for the {@link org.eclipse.jetty.server.Response#sendError(int, String)} method.
+     * Reasons will only reflect
+     * if the code value is greater or equal to 400.
+     *
+     * @param message the reason
+     */
+    public void setMessage(String message)
+    {
+        _message = message;
     }
 
     /* ------------------------------------------------------------ */
@@ -79,16 +94,13 @@ public class ResponsePatternRule extends PatternRule
     @Override
     public String apply(String target, HttpServletRequest request, HttpServletResponse response) throws IOException
     {
-        int code = Integer.parseInt(_code);
-
         // status code 400 and up are error codes
-        if (code >= 400)
+        if (_code>0)
         {
-            response.sendError(code, _reason);
+            if (_message != null && !_message.isEmpty())
-        }
+                response.sendError(_code, _message);
-        else
+            else
-        {
+                response.setStatus(_code);
-            response.setStatus(code);
         }
         return target;
     }
@@ -100,6 +112,6 @@ public class ResponsePatternRule extends PatternRule
     @Override
     public String toString()
     {
-        return super.toString()+"["+_code+","+_reason+"]";
+        return super.toString()+"["+_code+","+ _message +"]";
     }
 }

jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java

@@ -42,7 +42,7 @@ public class ValidUrlRule extends Rule
     private static final Logger LOG = Log.getLogger(ValidUrlRule.class);
 
     String _code = "400";
-    String _reason = "Illegal Url";
+    String _message = "Illegal Url";
     
     public ValidUrlRule()
     {
@@ -64,13 +64,26 @@ public class ValidUrlRule extends Rule
 
     /* ------------------------------------------------------------ */
     /**
-     * Sets the reason for the response status code. Reasons will only reflect if the code value is greater or equal to 400.
+     * Sets the reason for the response status code.
      * 
      * @param reason the reason
+     * @deprecated use {@link #setMessage(String)}
      */
+    @Deprecated
     public void setReason(String reason)
     {
-        _reason = reason;
+        _message = reason;
+    }
+
+    /* ------------------------------------------------------------ */
+    /**
+     * Sets the message for the {@link org.eclipse.jetty.server.Response#sendError(int, String)} method.
+     *
+     * @param message the message
+     */
+    public void setMessage(String message)
+    {
+        _message = message;
     }
 
     @Override
@@ -90,14 +103,10 @@ public class ValidUrlRule extends Rule
                 int code = Integer.parseInt(_code);
 
                 // status code 400 and up are error codes so include a reason
-                if (code >= 400)
+                if (_message!=null && !_message.isEmpty())
-                {
+                    response.sendError(code, _message);
-                    response.sendError(code,_reason);
-                }
                 else
-                {
                     response.setStatus(code);
-                }
 
                 // we have matched, return target and consider it is handled
                 return target;
@@ -121,6 +130,6 @@ public class ValidUrlRule extends Rule
     @Override
     public String toString()
     {
-        return super.toString() + "[" + _code + ":" + _reason + "]";
+        return super.toString() + "[" + _code + ":" + _message + "]";
     }
 }

jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java

@@ -18,13 +18,15 @@
 
 package org.eclipse.jetty.rewrite.handler;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
 import java.io.IOException;
 
+import org.eclipse.jetty.server.Dispatcher;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
 public class ResponsePatternRuleTest extends AbstractRuleTestCase
 {
     private ResponsePatternRule _rule;
@@ -38,57 +40,27 @@ public class ResponsePatternRuleTest extends AbstractRuleTestCase
     }
 
     @Test
-    public void testStatusCodeNoReason() throws IOException
+    public void testStatusCodeNoMessage() throws IOException
     {
-        for (int i = 1; i < 400; i++)
+        for (int i = 1; i < 600; i++)
         {
             _rule.setCode("" + i);
+            _rule.setMessage(null);
             _rule.apply(null, _request, _response);
 
             assertEquals(i, _response.getStatus());
+            assertNull(_request.getAttribute(Dispatcher.ERROR_MESSAGE));
         }
     }
 
     @Test
-    public void testStatusCodeWithReason() throws IOException
+    public void testStatusCodeMessage() throws IOException
     {
-        for (int i = 1; i < 400; i++)
+        _rule.setCode("499");
-        {
+        _rule.setMessage("Message 499");
-            _rule.setCode("" + i);
+        _rule.apply(null, _request, _response);
-            _rule.setReason("reason" + i);
-            _rule.apply(null, _request, _response);
 
-            assertEquals(i, _response.getStatus());
+        assertEquals(499, _response.getStatus());
-            assertEquals(null, _response.getReason());
+        assertEquals( "Message 499", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
-        }
-    }
-
-    @Test
-    public void testErrorStatusNoReason() throws IOException
-    {
-        for (int i = 400; i < 600; i++)
-        {
-            _rule.setCode("" + i);
-            _rule.apply(null, _request, _response);
-
-            assertEquals(i, _response.getStatus());
-            assertEquals("", _response.getReason());
-            super.reset();
-        }
-    }
-
-    @Test
-    public void testErrorStatusWithReason() throws IOException
-    {
-        for (int i = 400; i < 600; i++)
-        {
-            _rule.setCode("" + i);
-            _rule.setReason("reason-" + i);
-            _rule.apply(null, _request, _response);
-
-            assertEquals(i, _response.getStatus());
-            assertEquals("reason-" + i, _response.getReason());
-            super.reset();
-        }
     }
 }

jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java

@@ -18,14 +18,15 @@
 
 package org.eclipse.jetty.rewrite.handler;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import org.eclipse.jetty.server.Dispatcher;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 @SuppressWarnings("unused")
 public class ValidUrlRuleTest extends AbstractRuleTestCase
 {
@@ -61,16 +62,16 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
     }
 
     @Test
-    public void testInvalidUrlWithReason() throws Exception
+    public void testInvalidUrlWithMessage() throws Exception
     {
         _rule.setCode("405");
-        _rule.setReason("foo");
+        _rule.setMessage("foo");
         _request.setURIPathQuery("/%00/");
         
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
         assertEquals(405,_response.getStatus());
-        assertEquals("foo",_response.getReason());
+        assertEquals( "foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
     }
     
     @Test
@@ -83,7 +84,7 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
         assertEquals(405,_response.getStatus());
-        assertEquals("foo",_response.getReason());
+        assertEquals( "foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
     }
     
     @Disabled("Not working in jetty-9")
@@ -97,7 +98,7 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
         assertEquals(405,_response.getStatus());
-        assertEquals("foo",_response.getReason());
+        assertEquals( "foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
     }
 
     @Disabled("Not working in jetty-9")

jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java

@@ -18,11 +18,6 @@
 
 package org.eclipse.jetty.security;
 
-import static org.hamcrest.Matchers.startsWith;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashSet;
@@ -48,6 +43,11 @@ import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.startsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 /**
  * @version $Revision: 1441 $ $Date: 2010-04-02 12:28:17 +0200 (Fri, 02 Apr 2010) $
  */
@@ -323,7 +323,7 @@ public class SpecExampleConstraintTest
         response = _connector.getResponse("POST /ctx/acme/wholesale/index.html HTTP/1.0\r\n" +
                                            "Authorization: Basic " + B64Code.encode("chris:password") + "\r\n" +
                                            "\r\n");
-        assertThat(response,startsWith("HTTP/1.1 403 !"));
+        assertThat(response,startsWith("HTTP/1.1 403 "));
         
         //a user in role HOMEOWNER can do a GET
         response = _connector.getResponse("GET /ctx/acme/retail/index.html HTTP/1.0\r\n" +

jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

@@ -633,13 +633,9 @@ public class Response implements HttpServletResponse
 
         Request request = _channel.getRequest();
         Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION);
+        _reason=HttpStatus.getMessage(code);
         if (message==null)
-        {    
-            _reason=HttpStatus.getMessage(code);
             message=cause==null?_reason:cause.toString();
-        }    
-        else
-            _reason=message;
 
         // If we are allowed to have a body, then produce the error page.
         if (code != SC_NO_CONTENT && code != SC_NOT_MODIFIED &&
@@ -873,19 +869,19 @@ public class Response implements HttpServletResponse
 
     @Override
     @Deprecated
-    public void setStatus(int sc, String sm)
+    public void setStatus(int sc, String  message)
     {
-        setStatusWithReason(sc,sm);
+        setStatusWithReason(sc, null);
     }
 
-    public void setStatusWithReason(int sc, String sm)
+    public void setStatusWithReason(int sc, String message)
     {
         if (sc <= 0)
             throw new IllegalArgumentException();
         if (!isIncluding())
         {
             _status = sc;
-            _reason = sm;
+            _reason = message;
         }
     }
 

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java

@@ -139,7 +139,14 @@ public class ErrorHandler extends AbstractHandler
 
         if (_cacheControl != null)
             response.setHeader(HttpHeader.CACHE_CONTROL.asString(), _cacheControl);
-        generateAcceptableResponse(baseRequest,request,response,response.getStatus(),baseRequest.getResponse().getReason());
+
+        String message = (String)request.getAttribute(RequestDispatcher.ERROR_MESSAGE);
+        if (message==null)
+            message = baseRequest.getResponse().getReason();
+        if (message==null)
+            message = HttpStatus.getMessage(response.getStatus());
+
+        generateAcceptableResponse(baseRequest,request,response,response.getStatus(),message);
     }
 
     /** 

jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java

@@ -18,21 +18,6 @@
 
 package org.eclipse.jetty.server;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.hamcrest.CoreMatchers.allOf;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsInAnyOrder;
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.not;
-import static org.hamcrest.Matchers.nullValue;
-import static org.hamcrest.Matchers.startsWith;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.LineNumberReader;
@@ -62,6 +47,7 @@ import org.eclipse.jetty.http.CookieCompliance;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpURI;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.MetaData;
@@ -86,6 +72,21 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.startsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 public class ResponseTest
 {
 
@@ -676,7 +677,9 @@ public class ResponseTest
 
         response.sendError(500, "Database Error");
         assertEquals(500, response.getStatus());
-        assertEquals("Database Error", response.getReason());
+        assertEquals("Server Error", response.getReason());
+        assertThat(BufferUtil.toString(_content), containsString("Database Error"));
+
         assertEquals("must-revalidate,no-cache,no-store", response.getHeader(HttpHeader.CACHE_CONTROL.asString()));
 
         response = getResponse();
@@ -689,7 +692,8 @@ public class ResponseTest
 
         response.sendError(406, "Super Nanny");
         assertEquals(406, response.getStatus());
-        assertEquals("Super Nanny", response.getReason());
+        assertEquals(HttpStatus.Code.NOT_ACCEPTABLE.getMessage(), response.getReason());
+        assertThat(BufferUtil.toString(_content), containsString("Super Nanny"));
         assertEquals("must-revalidate,no-cache,no-store", response.getHeader(HttpHeader.CACHE_CONTROL.asString()));
     }
     
@@ -707,7 +711,8 @@ public class ResponseTest
 
         response.sendError(500, "Database Error");
         assertEquals(500, response.getStatus());
-        assertEquals("Database Error", response.getReason());
+        assertEquals("Server Error", response.getReason());
+        assertThat(BufferUtil.toString(_content), is(""));
         assertThat(response.getHeader(HttpHeader.CACHE_CONTROL.asString()),Matchers.nullValue());
 
         response = getResponse();
@@ -720,7 +725,8 @@ public class ResponseTest
 
         response.sendError(406, "Super Nanny");
         assertEquals(406, response.getStatus());
-        assertEquals("Super Nanny", response.getReason());
+        assertEquals(HttpStatus.Code.NOT_ACCEPTABLE.getMessage(), response.getReason());
+        assertThat(BufferUtil.toString(_content), is(""));
         assertThat(response.getHeader(HttpHeader.CACHE_CONTROL.asString()),Matchers.nullValue());
     }
 
@@ -1384,6 +1390,7 @@ public class ResponseTest
     {
         _channel.recycle();
         _channel.getRequest().setMetaData(new MetaData.Request("GET",new HttpURI("/path/info"),HttpVersion.HTTP_1_0,new HttpFields()));
+        BufferUtil.clear(_content);
         return _channel.getResponse();
     }
 

jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java

@@ -18,12 +18,6 @@
 
 package org.eclipse.jetty.server.ssl;
 
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.startsWith;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -70,6 +64,12 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.startsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 public class SniSslConnectionFactoryTest
 {
     private Server _server;
@@ -268,8 +268,9 @@ public class SniSslConnectionFactoryTest
             output.flush();
 
             response = response(input);
+            String body = IO.toString(input);
             assertThat(response,startsWith("HTTP/1.1 400 "));
-            assertThat(response, containsString("Host does not match SNI"));
+            assertThat(body, containsString("Host does not match SNI"));
         }
         finally
         {
@@ -326,7 +327,8 @@ public class SniSslConnectionFactoryTest
 
             response = response(input);
             assertTrue(response.startsWith("HTTP/1.1 400 "));
-            assertThat(response, Matchers.containsString("Host does not match SNI"));
+            String body = IO.toString(input);
+            assertThat(body, Matchers.containsString("Host does not match SNI"));
         }
         finally
         {

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java

@@ -18,10 +18,6 @@
 
 package org.eclipse.jetty.servlet;
 
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.CoreMatchers.not;
-import static org.hamcrest.MatcherAssert.assertThat;
-
 import java.io.IOException;
 import java.io.PrintWriter;
 
@@ -42,6 +38,10 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
 public class ErrorPageTest
 {
     private Server _server;
@@ -168,7 +168,7 @@ public class ErrorPageTest
         try (StacklessLogging ignore = new StacklessLogging(Dispatcher.class))
         {
             String response = _connector.getResponse("GET /app?baa=%88%A4 HTTP/1.0\r\n\r\n");
-            assertThat(response, Matchers.containsString("HTTP/1.1 400 Bad query encoding"));
+            assertThat(response, Matchers.containsString("HTTP/1.1 400 "));
             assertThat(response, Matchers.containsString("ERROR_PAGE: /BadMessageException"));
             assertThat(response, Matchers.containsString("ERROR_MESSAGE: Bad query encoding"));
             assertThat(response, Matchers.containsString("ERROR_CODE: 400"));