[icedtea-web] RFC: Reduce URLConnection knowledge in CacheEntry
Omair Majid
omajid at redhat.com
Mon May 12 13:51:25 UTC 2014
* Omair Majid <omajid at redhat.com> [2014-05-02 13:24]:
> I had to work on CacheEntry for a few fixes (I will be posting those
> later) and the class seems to be doing a bit too much. The knowledge of
> URLConnection is especially troubling, since the metadata for a
> CacheEntry doesn't really match the URL in the presence of compression.
>
> The attached patch tries to reduce how much CacheEntry knows about
> URLConnection.
Since the other reviews asked for more unit tests, I have updated this
patch to add even more. Most of the functionality in CacheEntry should
now be covered by unit tests. Some of the corner cases are not, sadly.
Thanks,
Omair
--
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681
-------------- next part --------------
diff --git a/ChangeLog b/ChangeLog
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2014-05-09 Omair Majid <omajid at redhat.com>
+
+ * netx/net/sourceforge/jnlp/cache/CacheEntry.java: Use
+ constants for strings.
+ (initialize): Remove.
+ (getRemoteContentLength, setRemoteContentLength, getLastModified)
+ (setLastModified, getLongKey, setLongKey): New method.
+ * tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java: New
+ file.
+ * netx/net/sourceforge/jnlp/cache/ResourceTracker.java
+ (initializeResource, downloadResource): Use
+ CacheEntry.setRemoteContentLength and CacheEntry.setLastModified instead
+ of CacheEntry.initialize.
+
2014-05-09 Andrew Azores <aazores at redhat.com>
* netx/net/sourceforge/jnlp/util/TimedHashMap.java: implements Map
diff --git a/netx/net/sourceforge/jnlp/cache/CacheEntry.java b/netx/net/sourceforge/jnlp/cache/CacheEntry.java
--- a/netx/net/sourceforge/jnlp/cache/CacheEntry.java
+++ b/netx/net/sourceforge/jnlp/cache/CacheEntry.java
@@ -23,7 +23,6 @@
import java.net.*;
import net.sourceforge.jnlp.*;
-import net.sourceforge.jnlp.runtime.*;
import net.sourceforge.jnlp.util.*;
/**
@@ -34,6 +33,10 @@
*/
public class CacheEntry {
+ private static final String KEY_CONTENT_LENGTH = "content-length";
+ private static final String KEY_LAST_MODIFIED = "last-modified";
+ private static final String KEY_LAST_UPDATED = "last-updated";
+
/** the remote resource location */
private URL location;
@@ -61,18 +64,6 @@
}
/**
- * Initialize the cache entry data from a connection to the
- * remote resource (does not store data).
- */
- void initialize(URLConnection connection) {
- long modified = connection.getLastModified();
- long length = connection.getContentLength(); // an int
-
- properties.setProperty("content-length", Long.toString(length));
- properties.setProperty("last-modified", Long.toString(modified));
- }
-
- /**
* Returns the remote location this entry caches.
*/
public URL getLocation() {
@@ -85,11 +76,7 @@
* @return
*/
public long getLastUpdated() {
- try {
- return Long.parseLong(properties.getProperty("last-updated"));
- } catch (Exception ex) {
- return 0;
- }
+ return getLongKey(KEY_LAST_UPDATED);
}
/**
@@ -98,7 +85,35 @@
* @param updatedTime
*/
public void setLastUpdated(long updatedTime) {
- properties.setProperty("last-updated", Long.toString(updatedTime));
+ setLongKey(KEY_LAST_UPDATED, updatedTime);
+ }
+
+ public long getRemoteContentLength() {
+ return getLongKey(KEY_CONTENT_LENGTH);
+ }
+
+ public void setRemoteContentLength(long length) {
+ setLongKey(KEY_CONTENT_LENGTH, length);
+ }
+
+ public long getLastModified() {
+ return getLongKey(KEY_LAST_MODIFIED);
+ }
+
+ public void setLastModified(long modifyTime) {
+ setLongKey(KEY_LAST_MODIFIED, modifyTime);
+ }
+
+ private long getLongKey(String key) {
+ try {
+ return Long.parseLong(properties.getProperty(key));
+ } catch (Exception ex) {
+ return 0;
+ }
+ }
+
+ private void setLongKey(String key, long value) {
+ properties.setProperty(key, Long.toString(value));
}
/**
@@ -117,7 +132,7 @@
try {
long remoteModified = lastModified;
- long cachedModified = Long.parseLong(properties.getProperty("last-modified"));
+ long cachedModified = Long.parseLong(properties.getProperty(KEY_LAST_MODIFIED));
if (remoteModified > 0 && remoteModified <= cachedModified)
return true;
@@ -137,13 +152,13 @@
* @return true if the resource is in the cache
*/
public boolean isCached() {
- File localFile = CacheUtil.getCacheFile(location, version);
+ File localFile = getCacheFile();
if (!localFile.exists())
return false;
try {
long cachedLength = localFile.length();
- long remoteLength = Long.parseLong(properties.getProperty("content-length", "-1"));
+ long remoteLength = Long.parseLong(properties.getProperty(KEY_CONTENT_LENGTH, "-1"));
if (remoteLength >= 0 && cachedLength != remoteLength)
return false;
@@ -157,6 +172,13 @@
}
/**
+ * Seam for testing
+ */
+ File getCacheFile() {
+ return CacheUtil.getCacheFile(location, version);
+ }
+
+ /**
* Save the current information for the cache entry.
*/
protected void store() {
@@ -183,4 +205,5 @@
protected void unlock() {
CacheUtil.unlockFile(properties);
}
+
}
diff --git a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
--- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
+++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
@@ -695,6 +695,9 @@
byte buf[] = new byte[1024];
int rlen;
+ long contentLength = con.getContentLengthLong();
+ long lastModified = con.getLastModified();
+
InputStream in = new BufferedInputStream(con.getInputStream());
OutputStream out = CacheUtil.getOutputStream(downloadLocation, resource.downloadVersion);
@@ -710,11 +713,16 @@
if (con instanceof HttpURLConnection)
((HttpURLConnection) con).disconnect();
+ if (packgz || gzip) {
+ // TODO why not set this otherwise?
+ downloadEntry.setRemoteContentLength(contentLength);
+ downloadEntry.setLastModified(lastModified);
+ }
+
/*
* If the file was compressed, uncompress it.
*/
if (packgz) {
- downloadEntry.initialize(con);
GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(CacheUtil
.getCacheFile(downloadLocation, resource.downloadVersion)));
InputStream inputStream = new BufferedInputStream(gzInputStream);
@@ -729,7 +737,6 @@
inputStream.close();
gzInputStream.close();
} else if (gzip) {
- downloadEntry.initialize(con);
GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(CacheUtil
.getCacheFile(downloadLocation, resource.downloadVersion)));
InputStream inputStream = new BufferedInputStream(gzInputStream);
@@ -828,8 +835,10 @@
}
// update cache entry
- if (!current)
- entry.initialize(connection);
+ if (!current) {
+ entry.setRemoteContentLength(connection.getContentLengthLong());
+ entry.setLastModified(connection.getLastModified());
+ }
entry.setLastUpdated(System.currentTimeMillis());
entry.store();
diff --git a/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java
new file mode 100644
--- /dev/null
+++ b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java
@@ -0,0 +1,179 @@
+/* CacheEntryTest -- unit test for CacheEntry
+ Copyright (C) 2014 Red Hat, Inc.
+
+This file is part of IcedTea.
+
+IcedTea is free software; you can redistribute it and/or
+modify it under the terms of the GNU General Public License as published by
+the Free Software Foundation, version 2.
+
+IcedTea is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with IcedTea; see the file COPYING. If not, write to
+the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+02110-1301 USA.
+
+Linking this library statically or dynamically with other modules is
+making a combined work based on this library. Thus, the terms and
+conditions of the GNU General Public License cover the whole
+combination.
+
+As a special exception, the copyright holders of this library give you
+permission to link this library with independent modules to produce an
+executable, regardless of the license terms of these independent
+modules, and to copy and distribute the resulting executable under
+terms of your choice, provided that you also meet, for each linked
+independent module, the terms and conditions of the license of that
+module. An independent module is a module which is not derived from
+or based on this library. If you modify this library, you may extend
+this exception to your version of the library, but you are not
+obligated to do so. If you do not wish to do so, delete this
+exception statement from your version.
+ */
+
+package net.sourceforge.jnlp.cache;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.nio.file.Files;
+
+import net.sourceforge.jnlp.Version;
+
+import org.junit.Before;
+import org.junit.Test;
+
+public class CacheEntryTest {
+
+ /** A custom subclass that allows supplying a predefined cache file */
+ static class TestCacheEntry extends CacheEntry {
+ private File cacheFile;
+ public TestCacheEntry(URL location, Version version, File cacheFile) {
+ super(location, version);
+ this.cacheFile = cacheFile;
+ }
+ @Override
+ protected File getCacheFile() {
+ return cacheFile;
+ }
+ }
+
+ private URL url;
+ private Version version;
+
+ @Before
+ public void setUp() throws MalformedURLException {
+ url = new URL("http://example.com/example.jar");
+ version = new Version("1.0");
+ }
+
+ @Test
+ public void verifyLocationIsSame() {
+ CacheEntry entry = new TestCacheEntry(url, version, null);
+ assertEquals(url, entry.getLocation());
+ }
+
+ @Test
+ public void verifyLastModifiedIsSetCorrectly() {
+ long LAST_MODIFIED = 1000;
+
+ CacheEntry entry = new TestCacheEntry(url, version, null);
+ entry.setLastModified(LAST_MODIFIED);
+
+ assertEquals(LAST_MODIFIED, entry.getLastModified());
+ }
+
+ @Test
+ public void verifyLastUpdatedIsSetCorrectly() {
+ long LAST_UPDATED = 1000;
+
+ CacheEntry entry = new TestCacheEntry(url, version, null);
+ entry.setLastUpdated(LAST_UPDATED);
+
+ assertEquals(LAST_UPDATED, entry.getLastUpdated());
+ }
+
+ @Test
+ public void verifyContentLengthIsSetCorrectly() {
+ long CONTENT_LENGTH = 1000;
+
+ CacheEntry entry = new TestCacheEntry(url, version, null);
+ entry.setRemoteContentLength(CONTENT_LENGTH);
+
+ assertEquals(CONTENT_LENGTH, entry.getRemoteContentLength());
+ }
+
+ @Test
+ public void verifyNotCachedIfFileIsAbsent() {
+ File doesNotExist = new File("/foo/bar/baz/spam/eggs");
+
+ CacheEntry entry = new TestCacheEntry(url, version, doesNotExist);
+
+ assertFalse(entry.isCached());
+ }
+
+ @Test
+ public void verifyNotCachedIfContentLengthsDiffer() throws IOException {
+ File cachedFile = createCacheFile("Foo");
+
+ CacheEntry entry = new TestCacheEntry(url, version, cachedFile);
+ entry.setRemoteContentLength(10000);
+
+ assertFalse(entry.isCached());
+ }
+
+ @Test
+ public void verifyCachedIfContentLengthsAreSame() throws IOException {
+ String contents = "Foo";
+ File cachedFile = createCacheFile(contents);
+
+ CacheEntry entry = new TestCacheEntry(url, version, cachedFile);
+ entry.setRemoteContentLength(contents.length());
+
+ assertTrue(entry.isCached());
+ }
+
+ @Test
+ public void verifyCurrentWhenCacheEntryHasSameTimeStamp() throws IOException {
+ long lastModified = 10;
+ String contents = "Foo";
+ File cachedFile = createCacheFile(contents);
+
+ CacheEntry entry = new TestCacheEntry(url, version, cachedFile);
+ entry.setRemoteContentLength(contents.length());
+ entry.setLastModified(lastModified);
+
+ assertTrue(entry.isCurrent(lastModified));
+ }
+
+ @Test
+ public void verifyNotCurrentWhenRemoteContentIsNewer() throws IOException {
+ long oldTimeStamp = 10;
+ long newTimeStamp = 100;
+ String contents = "Foo";
+ File cachedFile = createCacheFile(contents);
+
+ CacheEntry entry = new TestCacheEntry(url, version, cachedFile);
+ entry.setRemoteContentLength(contents.length());
+ entry.setLastModified(oldTimeStamp);
+
+ assertFalse(entry.isCurrent(newTimeStamp));
+ }
+
+ protected File createCacheFile(String contents) throws IOException {
+ File cachedFile = File.createTempFile("CacheEntryTest", null);
+ Files.write(cachedFile.toPath(), contents.getBytes());
+ cachedFile.deleteOnExit();
+ return cachedFile;
+ }
+
+}
More information about the distro-pkg-dev
mailing list