Fix crash when using HurlStack with POST requests. (google#202)

In google#193 we moved setting of request headers to after setting the connection parameters (Content-Type header and request body). However, setting a request body (or more precisely, calling HttpUrlConection#getOutputStream) prevents us from setting additional headers, because the connection is opened already. Instead, we just skip setting the Content-Type header if one has already been set via Request#getHeaders. This is a bit more error-prone, but unlikely to change in the future, and hard to express in a cleaner way because with the DEPRECATED_GET_OR_POST method, we need to call getPostBody to determine if the request will be a GET or POST request, and we should only call this once. Verified on a real device (as Robolectric did not catch this). Fixes google#198
dut3062796s · Jun 14, 2018 · 2695d3c · 2695d3c
1 parent 0c32d6a
commit 2695d3c
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 8 deletions.
diff --git a/src/main/java/com/android/volley/toolbox/HurlStack.java b/src/main/java/com/android/volley/toolbox/HurlStack.java
@@ -88,12 +88,10 @@ public HttpResponse executeRequest(Request<?> request, Map<String, String> addit
         HttpURLConnection connection = openConnection(parsedUrl, request);
         boolean keepConnectionOpen = false;
         try {
-            setConnectionParametersForRequest(connection, request);
-            // Apply the request headers last (they take precedence over any headers set by
-            // setConnectionParametersForRequest, like the Content-Type header).
             for (String headerName : map.keySet()) {
                 connection.setRequestProperty(headerName, map.get(headerName));
             }
+            setConnectionParametersForRequest(connection, request);
             // Initialize HttpResponse with data from the HttpURLConnection.
             int responseCode = connection.getResponseCode();
             if (responseCode == -1) {
@@ -222,6 +220,8 @@ private HttpURLConnection openConnection(URL url, Request<?> request) throws IOE
         return connection;
     }
 
+    // NOTE: Any request headers added here (via setRequestProperty or addRequestProperty) should be
+    // checked against the existing properties in the connection and not overridden if already set.
     @SuppressWarnings("deprecation")
     /* package */ static void setConnectionParametersForRequest(
             HttpURLConnection connection, Request<?> request) throws IOException, AuthFailureError {
@@ -279,13 +279,16 @@ private static void addBodyIfExists(HttpURLConnection connection, Request<?> req
     }
 
     private static void addBody(HttpURLConnection connection, Request<?> request, byte[] body)
-            throws IOException, AuthFailureError {
+            throws IOException {
         // Prepare output. There is no need to set Content-Length explicitly,
         // since this is handled by HttpURLConnection using the size of the prepared
         // output stream.
         connection.setDoOutput(true);
-        connection.setRequestProperty(
-                HttpHeaderParser.HEADER_CONTENT_TYPE, request.getBodyContentType());
+        // Set the content-type unless it was already set (by Request#getHeaders).
+        if (!connection.getRequestProperties().containsKey(HttpHeaderParser.HEADER_CONTENT_TYPE)) {
+            connection.setRequestProperty(
+                    HttpHeaderParser.HEADER_CONTENT_TYPE, request.getBodyContentType());
+        }
         DataOutputStream out = new DataOutputStream(connection.getOutputStream());
         out.write(body);
         out.close();

diff --git a/src/test/java/com/android/volley/toolbox/HttpStackConformanceTest.java b/src/test/java/com/android/volley/toolbox/HttpStackConformanceTest.java
@@ -15,7 +15,9 @@
 import java.io.OutputStream;
 import java.net.HttpURLConnection;
 import java.net.URL;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import org.apache.http.Header;
 import org.apache.http.HttpRequest;
@@ -58,8 +60,7 @@ public void setOutputHeaderMap(final Map<String, String> outputHeaderMap) {
                         doAnswer(
                                         new Answer<Void>() {
                                             @Override
-                                            public Void answer(InvocationOnMock invocation)
-                                                    throws Throwable {
+                                            public Void answer(InvocationOnMock invocation) {
                                                 outputHeaderMap.put(
                                                         invocation.<String>getArgument(0),
                                                         invocation.<String>getArgument(1));
@@ -68,6 +69,24 @@ public Void answer(InvocationOnMock invocation)
                                         })
                                 .when(mMockConnection)
                                 .setRequestProperty(anyString(), anyString());
+                        doAnswer(
+                                        new Answer<Map<String, List<String>>>() {
+                                            @Override
+                                            public Map<String, List<String>> answer(
+                                                    InvocationOnMock invocation) {
+                                                Map<String, List<String>> result = new HashMap<>();
+                                                for (Map.Entry<String, String> entry :
+                                                        outputHeaderMap.entrySet()) {
+                                                    result.put(
+                                                            entry.getKey(),
+                                                            Collections.singletonList(
+                                                                    entry.getValue()));
+                                                }
+                                                return result;
+                                            }
+                                        })
+                                .when(mMockConnection)
+                                .getRequestProperties();
                     }
                 },