[WAGON-529] Clean up inconsistent status code and reason phrase handling
* Write a cleaned up message to TransferFailedException
* Drop reason phrase from other exceptions where the message already
contains the phrase
* Renamed the HttpWagonReasonPhraseTest to HttpWagonErrorTest because it
was ill-designed: the test assumed that the message set by
HttpServletResponse#sendError() is returned in the status line to the
client by the server, but this is not true. The Servlet Spec says that
this message (not reason) has to be written to an HTML page. In this
case, Jetty simply wrote that to the status line, but we shall not rely
on an implementation detail of just one server. The spec does not offer
an option to actively set the reason phrase.
The reason phrase is optional after all, don't use it. Use the status
code.
diff --git a/wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java b/wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
index 097167f..8387e93 100755
--- a/wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
+++ b/wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
@@ -682,8 +682,16 @@
try
{
int statusCode = response.getStatusLine().getStatusCode();
- String reasonPhrase = ", ReasonPhrase: " + response.getStatusLine().getReasonPhrase() + ".";
- fireTransferDebug( url + " - Status code: " + statusCode + reasonPhrase );
+ String reasonPhrase = response.getStatusLine().getReasonPhrase();
+ StringBuilder debugMessage = new StringBuilder();
+ debugMessage.append( url );
+ debugMessage.append( " -- " );
+ debugMessage.append( "status code: " ).append( statusCode );
+ if ( StringUtils.isNotEmpty( reasonPhrase ) )
+ {
+ debugMessage.append( ", reason phrase: " ).append( reasonPhrase );
+ }
+ fireTransferDebug( debugMessage.toString() );
// Check that we didn't run out of retries.
switch ( statusCode )
@@ -703,10 +711,10 @@
return;
case HttpStatus.SC_FORBIDDEN:
fireSessionConnectionRefused();
- throw new AuthorizationException( "Access denied to: " + url + reasonPhrase );
+ throw new AuthorizationException( "Access denied to: " + url );
case HttpStatus.SC_NOT_FOUND:
- throw new ResourceDoesNotExistException( "File: " + url + " does not exist" + reasonPhrase );
+ throw new ResourceDoesNotExistException( "File " + url + " does not exist" );
case SC_TOO_MANY_REQUESTS:
put( backoff( wait, url ), resource, source, httpEntity, url );
@@ -714,7 +722,7 @@
//add more entries here
default:
TransferFailedException e = new TransferFailedException(
- "Failed to transfer file: " + url + ". Return code is: " + statusCode + reasonPhrase );
+ "Failed to transfer file " + url + " with status code " + statusCode );
fireTransferError( resource, e, TransferEvent.REQUEST_PUT );
throw e;
}
@@ -782,7 +790,6 @@
try
{
int statusCode = response.getStatusLine().getStatusCode();
- String reasonPhrase = ", ReasonPhrase: " + response.getStatusLine().getReasonPhrase() + ".";
boolean result;
switch ( statusCode )
{
@@ -793,13 +800,13 @@
result = true;
break;
case HttpStatus.SC_FORBIDDEN:
- throw new AuthorizationException( "Access denied to: " + url + reasonPhrase );
+ throw new AuthorizationException( "Access denied to: " + url );
case HttpStatus.SC_UNAUTHORIZED:
- throw new AuthorizationException( "Not authorized " + reasonPhrase );
+ throw new AuthorizationException( "Not authorized" );
case HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED:
- throw new AuthorizationException( "Not authorized by proxy " + reasonPhrase );
+ throw new AuthorizationException( "Not authorized by proxy" );
case HttpStatus.SC_NOT_FOUND:
result = false;
@@ -811,7 +818,7 @@
//add more entries here
default:
throw new TransferFailedException(
- "Failed to transfer file: " + url + ". Return code is: " + statusCode + reasonPhrase );
+ "Failed to transfer file " + url + " with status code " + statusCode );
}
EntityUtils.consume( response.getEntity() );
@@ -1079,10 +1086,16 @@
CloseableHttpResponse response = execute( getMethod );
closeable = response;
int statusCode = response.getStatusLine().getStatusCode();
-
- String reasonPhrase = ", ReasonPhrase:" + response.getStatusLine().getReasonPhrase() + ".";
-
- fireTransferDebug( url + " - Status code: " + statusCode + reasonPhrase );
+ String reasonPhrase = response.getStatusLine().getReasonPhrase();
+ StringBuilder debugMessage = new StringBuilder();
+ debugMessage.append( url );
+ debugMessage.append( " -- " );
+ debugMessage.append( "status code: " ).append( statusCode );
+ if ( StringUtils.isNotEmpty( reasonPhrase ) )
+ {
+ debugMessage.append( ", reason phrase: " ).append( reasonPhrase );
+ }
+ fireTransferDebug( debugMessage.toString() );
switch ( statusCode )
{
@@ -1094,18 +1107,18 @@
return;
case HttpStatus.SC_FORBIDDEN:
fireSessionConnectionRefused();
- throw new AuthorizationException( "Access denied to: " + url + " " + reasonPhrase );
+ throw new AuthorizationException( "Access denied to: " + url );
case HttpStatus.SC_UNAUTHORIZED:
fireSessionConnectionRefused();
- throw new AuthorizationException( "Not authorized " + reasonPhrase );
+ throw new AuthorizationException( "Not authorized" );
case HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED:
fireSessionConnectionRefused();
- throw new AuthorizationException( "Not authorized by proxy " + reasonPhrase );
+ throw new AuthorizationException( "Not authorized by proxy" );
case HttpStatus.SC_NOT_FOUND:
- throw new ResourceDoesNotExistException( "File: " + url + " " + reasonPhrase );
+ throw new ResourceDoesNotExistException( "File " + url + " does not exist" );
case SC_TOO_MANY_REQUESTS:
fillInputData( backoff( wait, url ), inputData );
@@ -1115,7 +1128,7 @@
default:
cleanupGetTransfer( resource );
TransferFailedException e = new TransferFailedException(
- "Failed to transfer file: " + url + ". Return code is: " + statusCode + " " + reasonPhrase );
+ "Failed to transfer file " + url + " with status code " + statusCode );
fireTransferError( resource, e, TransferEvent.REQUEST_GET );
throw e;
}
diff --git a/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/ErrorWithReasonPhaseServlet.java b/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/ErrorWithMessageServlet.java
similarity index 79%
rename from wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/ErrorWithReasonPhaseServlet.java
rename to wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/ErrorWithMessageServlet.java
index 6528c41..95a86c1 100644
--- a/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/ErrorWithReasonPhaseServlet.java
+++ b/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/ErrorWithMessageServlet.java
@@ -25,39 +25,36 @@
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
/**
* User: jdumay
* Date: 24/01/2008
* Time: 17:25:27
*/
-public class ErrorWithReasonPhaseServlet
+public class ErrorWithMessageServlet
extends HttpServlet
{
- public static final String REASON = "it sucks!";
+ private static final long serialVersionUID = -1419714266724471393L;
- private Logger logger = LoggerFactory.getLogger( ErrorWithReasonPhaseServlet.class );
+ public static final String MESSAGE = "it sucks!";
public void service( HttpServletRequest request, HttpServletResponse response )
throws ServletException, IOException
{
if ( request.getRequestURL().toString().contains( "401" ) )
{
- response.sendError( 401, REASON );
+ response.sendError( 401, MESSAGE );
}
else if ( request.getRequestURL().toString().contains( "403" ) )
{
- response.sendError( 403, REASON );
+ response.sendError( 403, MESSAGE );
}
else if ( request.getRequestURL().toString().contains( "407" ) )
{
- response.sendError( 407, REASON );
+ response.sendError( 407, MESSAGE );
}
else if ( request.getRequestURL().toString().contains( "500" ) )
{
- response.sendError( 500, REASON );
+ response.sendError( 500, MESSAGE );
}
}
diff --git a/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/HttpWagonReasonPhraseTest.java b/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/HttpWagonErrorTest.java
similarity index 91%
rename from wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/HttpWagonReasonPhraseTest.java
rename to wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/HttpWagonErrorTest.java
index 484d638..b48ee12 100644
--- a/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/HttpWagonReasonPhraseTest.java
+++ b/wagon-providers/wagon-http/src/test/java/org/apache/maven/wagon/providers/http/HttpWagonErrorTest.java
@@ -31,14 +31,14 @@
/**
* User: jdumay Date: 24/01/2008 Time: 17:17:34
*/
-public class HttpWagonReasonPhraseTest
+public class HttpWagonErrorTest
extends HttpWagonHttpServerTestCase
{
protected void setUp()
throws Exception
{
super.setUp();
- ServletHolder servlets = new ServletHolder( new ErrorWithReasonPhaseServlet() );
+ ServletHolder servlets = new ServletHolder( new ErrorWithMessageServlet() );
context.addServlet( servlets, "/*" );
startServer();
}
@@ -75,7 +75,6 @@
assertNotNull( thrown );
assertEquals( AuthorizationException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
public void testGetReasonPhase403()
@@ -110,7 +109,6 @@
assertNotNull( thrown );
assertEquals( AuthorizationException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
@@ -146,7 +144,6 @@
assertNotNull( thrown );
assertEquals( AuthorizationException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
public void testGetReasonPhase500()
@@ -181,6 +178,5 @@
assertNotNull( thrown );
assertEquals( TransferFailedException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
}
