Merge pull request #163 from microsoftgraph/bugfix/retry-handler-stream-request

- fixes a bug where the retry handler would use the response streaming information instead of the requst to evaluate

Author: baywet

src/main/java/com/microsoft/graph/httpcore/RetryHandler.java

@@ -22,10 +22,8 @@ public class RetryHandler implements Interceptor{
      */
     private final String RETRY_ATTEMPT_HEADER = "Retry-Attempt";
     private final String RETRY_AFTER = "Retry-After";
-    private final String TRANSFER_ENCODING = "Transfer-Encoding";
-    private final String TRANSFER_ENCODING_CHUNKED = "chunked";
-    private final String APPLICATION_OCTET_STREAM = "application/octet-stream";
-    private final String CONTENT_TYPE = "Content-Type";
+    /** Content length request header value */
+    private final String CONTENT_LENGTH = "Content-Length";
 
     public static final int MSClientErrorCodeTooManyRequests = 429;
     public static final int MSClientErrorCodeServiceUnavailable  = 503;
@@ -66,7 +64,7 @@ boolean retryRequest(Response response, int executionCount, Request request, Ret
 		// without any retry attempt.
 		shouldRetry =
 				(executionCount <= retryOptions.maxRetries())
-				&& checkStatus(statusCode) && isBuffered(response, request)
+				&& checkStatus(statusCode) && isBuffered(request)
 				&& shouldRetryCallback != null
 				&& shouldRetryCallback.shouldRetry(retryOptions.delay(), executionCount, request, response);
 
@@ -106,27 +104,22 @@ boolean checkStatus(int statusCode) {
 	    		|| statusCode == MSClientErrorCodeGatewayTimeout);
 	}
 
-	boolean isBuffered(Response response, Request request) {
-		String methodName = request.method();
-		if(methodName.equalsIgnoreCase("GET") || methodName.equalsIgnoreCase("DELETE") || methodName.equalsIgnoreCase("HEAD") || methodName.equalsIgnoreCase("OPTIONS"))
-			return true;
+	boolean isBuffered(Request request) {
+		final String methodName = request.method();
 
-		boolean isHTTPMethodPutPatchOrPost = methodName.equalsIgnoreCase("POST") ||
+		final boolean isHTTPMethodPutPatchOrPost = methodName.equalsIgnoreCase("POST") ||
 				methodName.equalsIgnoreCase("PUT") ||
 				methodName.equalsIgnoreCase("PATCH");
 
-		if(isHTTPMethodPutPatchOrPost) {
-			boolean isStream = response.header(CONTENT_TYPE)!=null && response.header(CONTENT_TYPE).equalsIgnoreCase(APPLICATION_OCTET_STREAM);
-			if(!isStream) {
-				String transferEncoding = response.header(TRANSFER_ENCODING);
-				boolean isTransferEncodingChunked = (transferEncoding != null) &&
-						transferEncoding.equalsIgnoreCase(TRANSFER_ENCODING_CHUNKED);
-
-				if(request.body() != null && isTransferEncodingChunked)
-					return true;
-			}
+		if(isHTTPMethodPutPatchOrPost && request.body() != null) {
+            try {
+                return request.body().contentLength() != -1L;
+            } catch (IOException ex) {
+                // expected
+                return false;
+            }
 		}
-		return false;
+		return true;
 	}
 
 	@Override

src/test/java/com/microsoft/graph/httpcore/RetryHandlerTest.java

@@ -4,6 +4,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
+import java.io.IOException;
 import java.net.HttpURLConnection;
 
 import org.junit.Test;
@@ -16,6 +17,7 @@
 import okhttp3.Request;
 import okhttp3.RequestBody;
 import okhttp3.Response;
+import okio.BufferedSink;
 
 public class RetryHandlerTest {
 
@@ -76,7 +78,7 @@ public void testRetryRequestWithMaxRetryAttempts() {
 		// Default retry options with Number of maxretries default to 3
 		RetryOptions retryOptions = new RetryOptions();
 		// Try to execute one more than allowed default max retries
-		int executionCount = RetryOptions.DEFAULT_MAX_RETRIES + 1; 
+		int executionCount = RetryOptions.DEFAULT_MAX_RETRIES + 1;
 		assertFalse(retryhandler.retryRequest(response, executionCount, httpget, retryOptions));
 	}
 
@@ -87,7 +89,7 @@ public void testRetryRequestForStatusCode() {
 		Response response = new Response.Builder()
 				.protocol(Protocol.HTTP_1_1)
 				// For status code 500 which is not in (429 503 504), So NO retry
-				.code(HTTP_SERVER_ERROR)  
+				.code(HTTP_SERVER_ERROR)
 				.message( "Internal Server Error")
 				.request(httpget)
 				.build();
@@ -100,7 +102,7 @@ public void testRetryRequestWithTransferEncoding() {
 		Request httppost = new Request.Builder().url(testmeurl).post(RequestBody.create(MediaType.parse("application/json"), "TEST")).build();
 		Response response = new Response.Builder()
 				.protocol(Protocol.HTTP_1_1)
-				.code(HttpURLConnection.HTTP_GATEWAY_TIMEOUT)  
+				.code(HttpURLConnection.HTTP_GATEWAY_TIMEOUT)
 				.message( "gateway timeout")
 				.request(httppost)
 				.addHeader("Transfer-Encoding", "chunked")
@@ -114,15 +116,15 @@ public void testRetryRequestWithExponentialBackOff() {
 		Request httppost = new Request.Builder().url(testmeurl).post(RequestBody.create(MediaType.parse("application/json"), "TEST")).build();
 		Response response = new Response.Builder()
 				.protocol(Protocol.HTTP_1_1)
-				.code(HttpURLConnection.HTTP_GATEWAY_TIMEOUT)  
+				.code(HttpURLConnection.HTTP_GATEWAY_TIMEOUT)
 				.message( "gateway timeout")
 				.request(httppost)
 				.addHeader("Transfer-Encoding", "chunked")
 				.build();
-		
+
 		assertTrue(retryhandler.retryRequest(response, 1, httppost, new RetryOptions()));
 	}
-	
+
 	@Test
 	public void testGetRetryAfterWithHeader() {
 		RetryHandler retryHandler = new RetryHandler();
@@ -131,7 +133,7 @@ public void testGetRetryAfterWithHeader() {
 		delay = retryHandler.getRetryAfter(TestResponse().newBuilder().addHeader("Retry-After", "1").build(), 2, 3);
 		assertTrue(delay == 1000);
 	}
-	
+
 	@Test
 	public void testGetRetryAfterOnFirstExecution() {
 		RetryHandler retryHandler = new RetryHandler();
@@ -140,14 +142,48 @@ public void testGetRetryAfterOnFirstExecution() {
 		delay = retryHandler.getRetryAfter(TestResponse(), 3, 2);
 		assertTrue(delay > 3100);
 	}
-	
+
 	@Test
 	public void testGetRetryAfterMaxExceed() {
 		RetryHandler retryHandler = new RetryHandler();
 		long delay = retryHandler.getRetryAfter(TestResponse(), 190, 1);
 		assertTrue(delay == 180000);
 	}
-	
+    @Test
+    public void testIsBuffered() {
+		final RetryHandler retryHandler = new RetryHandler();
+        Request request = new Request.Builder().url("https://localhost").method("GET", null).build();
+        assertTrue("Get Request is buffered", retryHandler.isBuffered(request));
+
+        request = new Request.Builder().url("https://localhost").method("DELETE", null).build();
+        assertTrue("Delete Request is buffered", retryHandler.isBuffered(request));
+
+        request = new Request.Builder().url("https://localhost")
+                                        .method("POST",
+                                            RequestBody.create(MediaType.parse("application/json"),
+                                                                "{\"key\": 42 }"))
+                                        .build();
+        assertTrue("Post Request is buffered", retryHandler.isBuffered(request));
+
+        request = new Request.Builder().url("https://localhost")
+                                        .method("POST",
+                                            new RequestBody() {
+
+												@Override
+												public MediaType contentType() {
+													return MediaType.parse("application/octet-stream");
+												}
+
+												@Override
+												public void writeTo(BufferedSink sink) throws IOException {
+													// TODO Auto-generated method stub
+
+												}
+                                            })
+                                        .build();
+        assertFalse("Post Stream Request is not buffered", retryHandler.isBuffered(request));
+    }
+
 	Response TestResponse() {
 		return new Response.Builder()
 				.code(429)