You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by mi...@apache.org on 2018/08/30 22:16:20 UTC
[maven-wagon] 01/01: [WAGON-529] Clean up inconsistent status code
and reason phrase handling
This is an automated email from the ASF dual-hosted git repository.
michaelo pushed a commit to branch WAGON-529
in repository https://gitbox.apache.org/repos/asf/maven-wagon.git
commit 53bdf1c2f1907868787fcdb2686e99eecbcc602a
Author: Michael Osipov <mi...@apache.org>
AuthorDate: Fri Aug 31 00:13:56 2018 +0200
[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.
---
.../wagon/shared/http/AbstractHttpClientWagon.java | 51 ++++++++++++++--------
...seServlet.java => ErrorWithMessageServlet.java} | 17 +++-----
...asonPhraseTest.java => HttpWagonErrorTest.java} | 8 +---
3 files changed, 41 insertions(+), 35 deletions(-)
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 @@ public abstract class AbstractHttpClientWagon
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 @@ public abstract class AbstractHttpClientWagon
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 @@ public abstract class AbstractHttpClientWagon
//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 @@ public abstract class AbstractHttpClientWagon
try
{
int statusCode = response.getStatusLine().getStatusCode();
- String reasonPhrase = ", ReasonPhrase: " + response.getStatusLine().getReasonPhrase() + ".";
boolean result;
switch ( statusCode )
{
@@ -793,13 +800,13 @@ public abstract class AbstractHttpClientWagon
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 @@ public abstract class AbstractHttpClientWagon
//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 @@ public abstract class AbstractHttpClientWagon
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 @@ public abstract class AbstractHttpClientWagon
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 @@ public abstract class AbstractHttpClientWagon
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.HttpServletRequest;
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 @@ import java.io.File;
/**
* 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 @@ public class HttpWagonReasonPhraseTest
assertNotNull( thrown );
assertEquals( AuthorizationException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
public void testGetReasonPhase403()
@@ -110,7 +109,6 @@ public class HttpWagonReasonPhraseTest
assertNotNull( thrown );
assertEquals( AuthorizationException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
@@ -146,7 +144,6 @@ public class HttpWagonReasonPhraseTest
assertNotNull( thrown );
assertEquals( AuthorizationException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
public void testGetReasonPhase500()
@@ -181,6 +178,5 @@ public class HttpWagonReasonPhraseTest
assertNotNull( thrown );
assertEquals( TransferFailedException.class, thrown.getClass() );
- assertTrue( thrown.getMessage().contains( ErrorWithReasonPhaseServlet.REASON ) );
}
}