Add default encoding/user agent to LazyHeaders.

Progress towards bumptech#470.
sjudd · Jun 24, 2015 · 487d9c5 · 487d9c5
1 parent 5884d2d
commit 487d9c5
Show file tree

Hide file tree

Showing 8 changed files with 200 additions and 37 deletions.
diff --git a/...ation/okhttp/src/main/java/com/bumptech/glide/integration/okhttp/OkHttpStreamFetcher.java b/...ation/okhttp/src/main/java/com/bumptech/glide/integration/okhttp/OkHttpStreamFetcher.java
@@ -17,8 +17,6 @@
  * Fetches an {@link InputStream} using the okhttp library.
  */
 public class OkHttpStreamFetcher implements DataFetcher<InputStream> {
-    private static final String USER_AGENT_HEADER = "User-Agent";
-    private static final String DEFAULT_USER_AGENT = System.getProperty("http.agent");
     private final OkHttpClient client;
     private final GlideUrl url;
     private InputStream stream;
@@ -34,15 +32,11 @@ public InputStream loadData(Priority priority) throws Exception {
         Request.Builder requestBuilder = new Request.Builder()
                 .url(url.toStringUrl());
 
-        boolean isUserAgentSet = false;
         for (Map.Entry<String, String> headerEntry : url.getHeaders().entrySet()) {
             String key = headerEntry.getKey();
             requestBuilder.addHeader(key, headerEntry.getValue());
-            isUserAgentSet |= USER_AGENT_HEADER.equalsIgnoreCase(key);
-        }
-        if (!isUserAgentSet) {
-            requestBuilder.addHeader(USER_AGENT_HEADER, DEFAULT_USER_AGENT);
         }
+
         Request request = requestBuilder.build();
 
         Response response = client.newCall(request).execute();

diff --git a/library/src/androidTest/java/com/bumptech/glide/load/data/HttpUrlFetcherServerTest.java b/library/src/androidTest/java/com/bumptech/glide/load/data/HttpUrlFetcherServerTest.java
@@ -231,7 +231,7 @@ public void testAppliesHeadersInGlideUrl() throws Exception {
     }
 
     private HttpUrlFetcher getFetcher() {
-        return getFetcher(Headers.NONE);
+        return getFetcher(Headers.DEFAULT);
     }
 
     private HttpUrlFetcher getFetcher(Headers headers) {

diff --git a/library/src/androidTest/java/com/bumptech/glide/load/model/LazyHeadersTest.java b/library/src/androidTest/java/com/bumptech/glide/load/model/LazyHeadersTest.java
@@ -26,7 +26,6 @@ public void testIncludesEagerHeaders() {
             .build()
             .getHeaders();
         assertThat(headers).containsEntry("key", "value");
-        assertThat(headers).hasSize(1);
     }
 
     @Test
@@ -38,7 +37,6 @@ public void testIncludesLazyHeaders() {
             .build()
             .getHeaders();
 
-        assertThat(headers).hasSize(1);
         assertThat(headers).containsEntry("key", "value");
     }
 
@@ -50,7 +48,6 @@ public void testMultipleEagerValuesAreSeparatedByCommas() {
             .build()
             .getHeaders();
 
-        assertThat(headers).hasSize(1);
         assertThat(headers).containsEntry("key", "first,second");
     }
 
@@ -66,7 +63,6 @@ public void testMultipleLazyValuesAreSeparatedByCommas() {
             .addHeader("key", second)
             .build()
             .getHeaders();
-        assertThat(headers).hasSize(1);
         assertThat(headers).containsEntry("key", "first,second");
     }
 
@@ -80,7 +76,6 @@ public void testMixedEagerAndLazyValuesAreIncluded() {
             .build()
             .getHeaders();
 
-        assertThat(headers).hasSize(1);
         assertThat(headers).containsEntry("key", "first,second");
 
         headers = new Builder()
@@ -89,7 +84,6 @@ public void testMixedEagerAndLazyValuesAreIncluded() {
             .build()
             .getHeaders();
 
-        assertThat(headers).hasSize(1);
         assertThat(headers).containsEntry("key", "second,first");
     }
 
@@ -103,7 +97,6 @@ public void testCanAddMultipleKeys() {
             .build()
             .getHeaders();
 
-        assertThat(headers).hasSize(2);
         assertThat(headers).containsEntry("first", "lazy");
         assertThat(headers).containsEntry("second", "eager");
     }
@@ -112,24 +105,112 @@ public void testCanAddMultipleKeys() {
     public void testUpdatingBuilderAfterBuildingDoesNotModifyOriginalHeaders() {
         Builder builder = new Builder();
         builder.addHeader("key", "firstValue");
+        builder.addHeader("otherKey", "otherValue");
         LazyHeaders first = builder.build();
 
         LazyHeaderFactory factory = mock(LazyHeaderFactory.class);
         when(factory.buildHeader()).thenReturn("otherValue");
         builder.addHeader("key", "secondValue");
-        builder.addHeader("otherKey", factory);
+        builder.setHeader("otherKey", factory);
         LazyHeaders second = builder.build();
 
         assertThat(first.getHeaders()).isNotEqualTo(second.getHeaders());
 
-        assertThat(first.getHeaders()).hasSize(1);
         assertThat(first.getHeaders()).containsEntry("key", "firstValue");
+        assertThat(first.getHeaders()).containsEntry("otherKey", "otherValue");
 
-        assertThat(second.getHeaders()).hasSize(2);
         assertThat(second.getHeaders()).containsEntry("key", "firstValue,secondValue");
         assertThat(second.getHeaders()).containsEntry("otherKey", "otherValue");
     }
 
+    @Test
+    public void testSetHeaderReplacesExistingHeaders() {
+        Builder builder = new Builder();
+        builder.addHeader("key", "first")
+            .addHeader("key", "second")
+            .setHeader("key", "third");
+        LazyHeaders headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("key", "third");
+    }
+
+    @Test
+    public void testSetHeaderWithNullStringRemovesExistingHeader() {
+        Builder builder = new Builder();
+        builder.addHeader("key", "first")
+            .addHeader("key", "second")
+            .setHeader("key", (String) null);
+        LazyHeaders headers = builder.build();
+        assertThat(headers.getHeaders()).doesNotContainKey("key");
+    }
+
+    @Test
+    public void testSetHeaderWithNullLazyHeaderFactoryRemovesExistingHeader() {
+        Builder builder = new Builder();
+        builder.addHeader("key", "first")
+            .addHeader("key", "second")
+            .setHeader("key", (LazyHeaderFactory) null);
+        LazyHeaders headers = builder.build();
+        assertThat(headers.getHeaders()).doesNotContainKey("key");
+    }
+
+    @Test
+    public void testAddingEncodingHeaderReplacesDefaultThenAppends() {
+        Builder builder = new Builder();
+        builder.addHeader("Accept-Encoding", "false");
+
+        LazyHeaders headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("Accept-Encoding", "false");
+
+        builder.addHeader("Accept-Encoding", "true");
+        headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("Accept-Encoding", "false,true");
+    }
+
+    @Test
+    public void testRemovingAndAddingEncodingHeaderReplacesDefaultThenAppends() {
+        Builder builder = new Builder();
+        builder.setHeader("Accept-Encoding", (String) null);
+        LazyHeaders headers = builder.build();
+        assertThat(headers.getHeaders()).doesNotContainKey("Accept-Encoding");
+
+        builder.addHeader("Accept-Encoding", "false");
+        headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("Accept-Encoding", "false");
+
+        builder.addHeader("Accept-Encoding", "true");
+        headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("Accept-Encoding", "false,true");
+    }
+
+    @Test
+    public void testAddingUserAgentHeaderReplacesDefaultThenAppends() {
+        Builder builder = new Builder();
+        builder.addHeader("User-Agent", "false");
+
+        LazyHeaders headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("User-Agent", "false");
+
+        builder.addHeader("User-Agent", "true");
+        headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("User-Agent", "false,true");
+    }
+
+    @Test
+    public void testRemovingAndAddingUserAgentHeaderReplacesDefaultThenAppends() {
+        Builder builder = new Builder();
+        builder.setHeader("User-Agent", (String) null);
+        LazyHeaders headers = builder.build();
+        assertThat(headers.getHeaders()).doesNotContainKey("User-Agent");
+
+        builder.addHeader("User-Agent", "false");
+        headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("User-Agent", "false");
+
+        builder.addHeader("User-Agent", "true");
+        headers = builder.build();
+        assertThat(headers.getHeaders()).containsEntry("User-Agent", "false,true");
+    }
+
     @Test
     public void testEquals() {
         LazyHeaderFactory firstLazyFactory = mock(LazyHeaderFactory.class);

diff --git a/library/src/main/java/com/bumptech/glide/load/data/HttpUrlFetcher.java b/library/src/main/java/com/bumptech/glide/load/data/HttpUrlFetcher.java
@@ -19,8 +19,6 @@
  */
 public class HttpUrlFetcher implements DataFetcher<InputStream> {
     private static final String TAG = "HttpUrlFetcher";
-    private static final String ENCODING_HEADER = "Accept-Encoding";
-    private static final String DEFAULT_ENCODING = "identity";
     private static final int MAXIMUM_REDIRECTS = 5;
     private static final HttpUrlConnectionFactory DEFAULT_CONNECTION_FACTORY = new DefaultHttpUrlConnectionFactory();
 
@@ -65,11 +63,6 @@ private InputStream loadDataWithRedirects(URL url, int redirects, URL lastUrl, M
         for (Map.Entry<String, String> headerEntry : headers.entrySet()) {
           urlConnection.addRequestProperty(headerEntry.getKey(), headerEntry.getValue());
         }
-        // Do our best to avoid gzip since it's both inefficient for images and also makes it more
-        // difficult for us to detect and prevent partial content rendering. See #440.
-        if (TextUtils.isEmpty(urlConnection.getRequestProperty(ENCODING_HEADER))) {
-            urlConnection.setRequestProperty(ENCODING_HEADER, DEFAULT_ENCODING);
-        }
         urlConnection.setConnectTimeout(2500);
         urlConnection.setReadTimeout(2500);
         urlConnection.setUseCaches(false);

diff --git a/library/src/main/java/com/bumptech/glide/load/model/GlideUrl.java b/library/src/main/java/com/bumptech/glide/load/model/GlideUrl.java
@@ -30,11 +30,11 @@ public class GlideUrl {
     private URL safeUrl;
 
     public GlideUrl(URL url) {
-        this(url, Headers.NONE);
+        this(url, Headers.DEFAULT);
     }
 
     public GlideUrl(String url) {
-        this(url, Headers.NONE);
+        this(url, Headers.DEFAULT);
     }
 
     public GlideUrl(URL url, Headers headers) {

diff --git a/library/src/main/java/com/bumptech/glide/load/model/Headers.java b/library/src/main/java/com/bumptech/glide/load/model/Headers.java
@@ -9,14 +9,25 @@
  */
 public interface Headers {
 
-    /** An empty Headers object that can be used if users don't want to provide headers. */
+    /**
+     * An empty Headers object that can be used if users don't want to provide headers.
+     *
+     * @deprecated Use {@link #DEFAULT} instead.
+     */
+    @Deprecated
     Headers NONE = new Headers() {
         @Override
         public Map<String, String> getHeaders() {
             return Collections.emptyMap();
         }
     };
 
+    /**
+     * A Headers object containing reasonable defaults that should be used when users don't want
+     * to provide their own headers.
+     */
+    Headers DEFAULT = new LazyHeaders.Builder().build();
+
     Map<String, String> getHeaders();
 
 }
diff --git a/library/src/main/java/com/bumptech/glide/load/model/LazyHeaders.java b/library/src/main/java/com/bumptech/glide/load/model/LazyHeaders.java
@@ -1,5 +1,7 @@
 package com.bumptech.glide.load.model;
 
+import android.text.TextUtils;
+
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -80,11 +82,42 @@ public int hashCode() {
      * Builder class for {@link LazyHeaders}.
      *
      * <p> This class is not thread safe. </p>
+     *
+     * <p> This class may include default values for User-Agent and Accept-Encoding headers. These
+     * will be replaced by calls to either {@link #setHeader(String, LazyHeaderFactory)} or
+     * {@link #addHeader(String, String)}, even though {@link #addHeader(String, LazyHeaderFactory)}
+     * would usually append an additional value. </p>
      */
+     // PMD doesn't like the necessary static block to initialize DEFAULT_HEADERS.
+    @SuppressWarnings("PMD.FieldDeclarationsShouldBeAtStartOfClass")
     public static final class Builder {
-        private boolean copyOnModify;
-        private Map<String, List<LazyHeaderFactory>> headers =
-            new HashMap<String, List<LazyHeaderFactory>>();
+        private static final String USER_AGENT_HEADER = "User-Agent";
+        private static final String DEFAULT_USER_AGENT = System.getProperty("http.agent");
+        private static final String ENCODING_HEADER = "Accept-Encoding";
+        private static final String DEFAULT_ENCODING = "identity";
+        private static final Map<String, List<LazyHeaderFactory>> DEFAULT_HEADERS;
+
+        // Set Accept-Encoding header to do our best to avoid gzip since it's both inefficient for
+        // images and also makes it more difficult for us to detect and prevent partial content
+        // rendering. See #440.
+        static {
+            Map<String, List<LazyHeaderFactory>> temp
+                = new HashMap<String, List<LazyHeaderFactory>>(2);
+            if (!TextUtils.isEmpty(DEFAULT_USER_AGENT)) {
+                temp.put(USER_AGENT_HEADER,
+                    Collections.<LazyHeaderFactory>singletonList(
+                        new StringHeaderFactory(DEFAULT_USER_AGENT)));
+            }
+            temp.put(ENCODING_HEADER,
+                Collections.<LazyHeaderFactory>singletonList(
+                    new StringHeaderFactory(DEFAULT_ENCODING)));
+            DEFAULT_HEADERS = Collections.unmodifiableMap(temp);
+        }
+
+        private boolean copyOnModify = true;
+        private boolean isEncodingDefault = true;
+        private Map<String, List<LazyHeaderFactory>> headers = DEFAULT_HEADERS;
+        private boolean isUserAgentDefault = headers.containsKey(DEFAULT_USER_AGENT);
 
         /**
          * Adds a value for the given header and returns this builder.
@@ -110,18 +143,69 @@ public Builder addHeader(String key, String value) {
          * times </p>
          */
         public Builder addHeader(String key, LazyHeaderFactory factory) {
-            if (copyOnModify) {
-                copyOnModify = false;
-                headers = copyHeaders();
+            if ((isEncodingDefault && ENCODING_HEADER.equalsIgnoreCase(key))
+                || (isUserAgentDefault && USER_AGENT_HEADER.equalsIgnoreCase(key))) {
+                return setHeader(key, factory);
             }
 
+            copyIfNecessary();
+            getFactories(key).add(factory);
+            return this;
+        }
+
+        /**
+         * Replaces all existing {@link LazyHeaderFactory LazyHeaderFactorys} for the given key
+         * with the given {@link LazyHeaderFactory}.
+         *
+         * <p> If the given value is {@code null}, the header at the given key will be removed. </p>
+         *
+         * <p> Use {@link #setHeader(String, LazyHeaderFactory)} if obtaining the value requires I/O
+         * (ie an oauth token). </p>
+         */
+        public Builder setHeader(String key, String value) {
+            return setHeader(key, value == null ? null : new StringHeaderFactory(value));
+        }
+
+        /**
+         * Replaces all existing {@link LazyHeaderFactory LazyHeaderFactorys} for the given key
+         * with the given {@link LazyHeaderFactory}.
+         *
+         * <p> If the given value is {@code null}, the header at the given key will be removed. </p>
+         */
+        public Builder setHeader(String key, LazyHeaderFactory factory) {
+            copyIfNecessary();
+            if (factory == null) {
+                headers.remove(key);
+            } else {
+                List<LazyHeaderFactory> factories = getFactories(key);
+                factories.clear();
+                factories.add(factory);
+            }
+
+            if (isEncodingDefault && ENCODING_HEADER.equalsIgnoreCase(key)) {
+                isEncodingDefault = false;
+            }
+            if (isUserAgentDefault && USER_AGENT_HEADER.equalsIgnoreCase(key)) {
+                isUserAgentDefault = false;
+            }
+
+            return this;
+        }
+
+        private List<LazyHeaderFactory> getFactories(String key) {
             List<LazyHeaderFactory> factories = headers.get(key);
             if (factories == null) {
                 factories = new ArrayList<LazyHeaderFactory>();
                 headers.put(key, factories);
             }
-            factories.add(factory);
-            return this;
+            return factories;
+        }
+
+        private void copyIfNecessary() {
+            if (copyOnModify) {
+                copyOnModify = false;
+                headers = copyHeaders();
+            }
         }
 
         /**