diff --git a/integration/volley/src/androidTest/java/com/bumptech/glide/integration/volley/VolleyStreamFetcherServerTest.java b/integration/volley/src/androidTest/java/com/bumptech/glide/integration/volley/VolleyStreamFetcherServerTest.java index 8d50837ec1..e70c616569 100644 --- a/integration/volley/src/androidTest/java/com/bumptech/glide/integration/volley/VolleyStreamFetcherServerTest.java +++ b/integration/volley/src/androidTest/java/com/bumptech/glide/integration/volley/VolleyStreamFetcherServerTest.java @@ -8,6 +8,7 @@ import com.android.volley.toolbox.Volley; import com.bumptech.glide.Priority; import com.bumptech.glide.load.data.DataFetcher; +import com.bumptech.glide.load.model.GlideUrl; import com.squareup.okhttp.mockwebserver.MockResponse; import com.squareup.okhttp.mockwebserver.MockWebServer; import org.junit.After; @@ -196,7 +197,7 @@ public InputStream get() throws InterruptedException, ExecutionException { return super.get(); } }; - return new VolleyStreamFetcher(requestQueue, url.toString(), requestFuture); + return new VolleyStreamFetcher(requestQueue, new GlideUrl(url.toString()), requestFuture); } private static String isToString(InputStream is) throws IOException { diff --git a/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyStreamFetcher.java b/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyStreamFetcher.java index 8b9e5f9214..d78723d80f 100644 --- a/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyStreamFetcher.java +++ b/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyStreamFetcher.java @@ -7,24 +7,27 @@ import com.android.volley.toolbox.HttpHeaderParser; import com.bumptech.glide.Priority; import com.bumptech.glide.load.data.DataFetcher; +import com.bumptech.glide.load.model.GlideUrl; import java.io.ByteArrayInputStream; import java.io.InputStream; +import java.lang.reflect.Method; /** * A DataFetcher backed by volley for fetching images via http. */ public class VolleyStreamFetcher implements DataFetcher { private final RequestQueue requestQueue; - private final String url; + private final GlideUrl url; private VolleyRequestFuture requestFuture; @SuppressWarnings("unused") - public VolleyStreamFetcher(RequestQueue requestQueue, String url) { + public VolleyStreamFetcher(RequestQueue requestQueue, GlideUrl url) { this(requestQueue, url, null); } - public VolleyStreamFetcher(RequestQueue requestQueue, String url, VolleyRequestFuture requestFuture) { + public VolleyStreamFetcher(RequestQueue requestQueue, GlideUrl url, + VolleyRequestFuture requestFuture) { this.requestQueue = requestQueue; this.url = url; this.requestFuture = requestFuture; @@ -35,7 +38,9 @@ public VolleyStreamFetcher(RequestQueue requestQueue, String url, VolleyRequestF @Override public InputStream loadData(Priority priority) throws Exception { - GlideRequest request = new GlideRequest(url, requestFuture, glideToVolleyPriority(priority)); + // Make sure the string url safely encodes non ascii characters. + String stringUrl = url.toURL().toString(); + GlideRequest request = new GlideRequest(stringUrl, requestFuture, glideToVolleyPriority(priority)); requestFuture.setRequest(requestQueue.add(request)); @@ -49,7 +54,7 @@ public void cleanup() { @Override public String getId() { - return url; + return url.toString(); } @Override diff --git a/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyUrlLoader.java b/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyUrlLoader.java index b08b0224c1..b2093ef52a 100644 --- a/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyUrlLoader.java +++ b/integration/volley/src/main/java/com/bumptech/glide/integration/volley/VolleyUrlLoader.java @@ -67,6 +67,6 @@ public VolleyUrlLoader(RequestQueue requestQueue) { @Override public DataFetcher getResourceFetcher(GlideUrl url, int width, int height) { - return new VolleyStreamFetcher(requestQueue, url.toString(), new VolleyRequestFuture()); + return new VolleyStreamFetcher(requestQueue, url, new VolleyRequestFuture()); } } diff --git a/library/src/androidTest/java/com/bumptech/glide/load/model/GlideUrlTest.java b/library/src/androidTest/java/com/bumptech/glide/load/model/GlideUrlTest.java index ed8559a40d..5d0071b014 100644 --- a/library/src/androidTest/java/com/bumptech/glide/load/model/GlideUrlTest.java +++ b/library/src/androidTest/java/com/bumptech/glide/load/model/GlideUrlTest.java @@ -60,4 +60,26 @@ public void testProducesEquivalentStringFromURL() throws MalformedURLException { assertEquals(expected, glideUrl.toString()); } + + @Test + public void testIssue133() throws MalformedURLException { + // u00e0=à + final String original = "http://www.commitstrip.com/wp-content/uploads/2014/07/" + + "Excel-\u00E0-toutes-les-sauces-650-finalenglish.jpg"; + + final String escaped = "http://www.commitstrip.com/wp-content/uploads/2014/07/" + + "Excel-%C3%A0-toutes-les-sauces-650-finalenglish.jpg"; + + GlideUrl glideUrlFromString = new GlideUrl(original); + assertEquals(escaped, glideUrlFromString.toURL().toString()); + + GlideUrl glideUrlFromEscapedString = new GlideUrl(escaped); + assertEquals(escaped, glideUrlFromEscapedString.toURL().toString()); + + GlideUrl glideUrlFromUrl = new GlideUrl(new URL(original)); + assertEquals(escaped, glideUrlFromUrl.toURL().toString()); + + GlideUrl glideUrlFromEscapedUrl = new GlideUrl(new URL(escaped)); + assertEquals(escaped, glideUrlFromEscapedUrl.toURL().toString()); + } } \ No newline at end of file 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 index ddb8dcb4f3..b87b3cf235 100644 --- a/library/src/main/java/com/bumptech/glide/load/model/GlideUrl.java +++ b/library/src/main/java/com/bumptech/glide/load/model/GlideUrl.java @@ -1,19 +1,30 @@ package com.bumptech.glide.load.model; +import android.net.Uri; import android.text.TextUtils; import java.net.MalformedURLException; import java.net.URL; /** - * This is a simple wrapper for strings representing http/https URLs. new URL() is an excessively expensive operation - * that may be unnecessary if the class loading the image from the URL doesn't actually require a URL object. + * A wrapper for strings representing http/https URLs responsible for ensuring URLs are properly escaped and avoiding + * unnecessary URL instantiations for loaders that require only string urls rather than URL objects. * - * Users wishing to replace the class for handling URLs must register a factory using GlideUrl. + *

+ * Users wishing to replace the class for handling URLs must register a factory using GlideUrl. + *

+ * + *

+ * To obtain a properly escaped URL, call {@link #toURL()}. To obtain a properly escaped string URL, call + * {@link #toURL()} and then {@link java.net.URL#toString()}. + *

*/ public class GlideUrl { private String stringUrl; + private static final String ALLOWED_URI_CHARS = "@#&=*+-_.,:!?()/~'%"; + private URL url; + private URL safeUrl; public GlideUrl(URL url) { if (url == null) { @@ -32,10 +43,21 @@ public GlideUrl(String url) { } public URL toURL() throws MalformedURLException { - if (url == null) { - url = new URL(stringUrl); + return getSafeUrl(); + } + + // See http://stackoverflow.com/questions/3286067/url-encoding-in-android. Although the answer using URI would work, + // using it would require both decoding and encoding each string which is more complicated, slower and generates + // more objects than the solution below. See also issue #133. + private URL getSafeUrl() throws MalformedURLException { + if (safeUrl != null) { + return safeUrl; } - return url; + String unsafe = toString(); + String safe = Uri.encode(unsafe, ALLOWED_URI_CHARS); + + safeUrl = new URL(safe); + return safeUrl; } @Override diff --git a/library/src/main/java/com/bumptech/glide/load/model/stream/HttpUrlGlideUrlLoader.java b/library/src/main/java/com/bumptech/glide/load/model/stream/HttpUrlGlideUrlLoader.java index 086a55ba28..573f7091aa 100644 --- a/library/src/main/java/com/bumptech/glide/load/model/stream/HttpUrlGlideUrlLoader.java +++ b/library/src/main/java/com/bumptech/glide/load/model/stream/HttpUrlGlideUrlLoader.java @@ -5,6 +5,7 @@ import com.bumptech.glide.load.data.HttpUrlFetcher; import com.bumptech.glide.load.model.GenericLoaderFactory; import com.bumptech.glide.load.model.GlideUrl; +import com.bumptech.glide.load.model.ModelCache; import com.bumptech.glide.load.model.ModelLoader; import com.bumptech.glide.load.model.ModelLoaderFactory; @@ -16,13 +17,17 @@ */ public class HttpUrlGlideUrlLoader implements ModelLoader { + private ModelCache modelCache; + /** * The default factory for {@link com.bumptech.glide.load.model.stream.HttpUrlGlideUrlLoader}s. */ public static class Factory implements ModelLoaderFactory { + private ModelCache modelCache = new ModelCache(500); + @Override public ModelLoader build(Context context, GenericLoaderFactory factories) { - return new HttpUrlGlideUrlLoader(); + return new HttpUrlGlideUrlLoader(modelCache); } @Override @@ -31,8 +36,25 @@ public void teardown() { } } + public HttpUrlGlideUrlLoader() { + // Empty. + } + + public HttpUrlGlideUrlLoader(ModelCache modelCache) { + this.modelCache = modelCache; + } + @Override public DataFetcher getResourceFetcher(GlideUrl model, int width, int height) { - return new HttpUrlFetcher(model); + // GlideUrls memoize parsed URLs so caching them saves a few object instantiations and time spent parsing urls. + GlideUrl url = model; + if (modelCache != null) { + url = modelCache.get(model, 0, 0); + if (url == null) { + modelCache.put(model, 0, 0, model); + url = model; + } + } + return new HttpUrlFetcher(url); } }