From 1fc2679bd4d9f228f1471642e66d255c899687b7 Mon Sep 17 00:00:00 2001
From: rfc2822 <hirner@bitfire.at>
Date: Sun, 17 Nov 2013 15:37:17 +0100
Subject: [PATCH] Bug fixes, refactoring, version bump

* don't manually decode URLs for comparing (because we now always use encoded URLs)
* .ics: VERSION before PRODID
* refactoring: move WebDavResource.httpClient to new singleton DavHttpClient
* close input streams in propfind/multi-get
* version bumpt to 0.4.1
---
 AndroidManifest.xml                           |  4 +-
 src/at/bitfire/davdroid/Constants.java        |  2 +-
 src/at/bitfire/davdroid/URIUtils.java         | 11 +--
 src/at/bitfire/davdroid/resource/Event.java   |  2 +-
 .../davdroid/webdav/DavHttpClient.java        | 58 ++++++++++++++
 .../webdav/InvalidDavResponseException.java   |  4 +-
 .../davdroid/webdav/WebDavResource.java       | 75 +++++++++----------
 .../bitfire/davdroid/test/URIUtilsTest.java   |  5 --
 .../webdav/test/WebDavResourceTest.java       | 20 ++---
 9 files changed, 110 insertions(+), 71 deletions(-)
 create mode 100644 src/at/bitfire/davdroid/webdav/DavHttpClient.java

diff --git a/AndroidManifest.xml b/AndroidManifest.xml
index b2954544..c4ea55e2 100644
--- a/AndroidManifest.xml
+++ b/AndroidManifest.xml
@@ -1,8 +1,8 @@
 <?xml version="1.0" encoding="utf-8"?>
 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
     package="at.bitfire.davdroid"
-    android:versionCode="13"
-    android:versionName="0.4-alpha" >
+    android:versionCode="14"
+    android:versionName="0.4.1-alpha" >
 
     <uses-sdk
         android:minSdkVersion="14"
diff --git a/src/at/bitfire/davdroid/Constants.java b/src/at/bitfire/davdroid/Constants.java
index 21e50d62..a2687f09 100644
--- a/src/at/bitfire/davdroid/Constants.java
+++ b/src/at/bitfire/davdroid/Constants.java
@@ -9,7 +9,7 @@ package at.bitfire.davdroid;
 
 public class Constants {
 	public static final String
-		APP_VERSION = "0.4-alpha",
+		APP_VERSION = "0.4.1-alpha",
 		
 		ACCOUNT_TYPE = "bitfire.at.davdroid",
 		
diff --git a/src/at/bitfire/davdroid/URIUtils.java b/src/at/bitfire/davdroid/URIUtils.java
index 5b87c7c1..1fedd8da 100644
--- a/src/at/bitfire/davdroid/URIUtils.java
+++ b/src/at/bitfire/davdroid/URIUtils.java
@@ -9,16 +9,7 @@ import android.util.Log;
 @SuppressLint("DefaultLocale")
 public class URIUtils {
 	private static final String TAG = "davdroid.URIUtils";
-	
-	public static boolean isSame(URI a, URI b) {
-	     try {
-	    	a = new URI(a.getScheme(), null, a.getHost(), a.getPort(), sanitize(a.getPath()), sanitize(a.getQuery()), null);
-	    	b = new URI(b.getScheme(), null, b.getHost(), b.getPort(), sanitize(b.getPath()), sanitize(b.getQuery()), null);
-			return a.equals(b);
-		} catch (URISyntaxException e) {
-			return false;
-		}
-	}
+
 	
 	// handles invalid URLs/paths as good as possible
 	public static String sanitize(String original) {
diff --git a/src/at/bitfire/davdroid/resource/Event.java b/src/at/bitfire/davdroid/resource/Event.java
index 54af7739..7eda047c 100644
--- a/src/at/bitfire/davdroid/resource/Event.java
+++ b/src/at/bitfire/davdroid/resource/Event.java
@@ -148,8 +148,8 @@ public class Event extends Resource {
 	@Override
 	public String toEntity() {
 		net.fortuna.ical4j.model.Calendar ical = new net.fortuna.ical4j.model.Calendar();
-		ical.getProperties().add(new ProdId("-//bitfire web engineering//DAVdroid " + Constants.APP_VERSION + "//EN"));
 		ical.getProperties().add(Version.VERSION_2_0);
+		ical.getProperties().add(new ProdId("-//bitfire web engineering//DAVdroid " + Constants.APP_VERSION + "//EN"));
 		
 		VEvent event = new VEvent();
 		PropertyList props = event.getProperties();
diff --git a/src/at/bitfire/davdroid/webdav/DavHttpClient.java b/src/at/bitfire/davdroid/webdav/DavHttpClient.java
new file mode 100644
index 00000000..617899ae
--- /dev/null
+++ b/src/at/bitfire/davdroid/webdav/DavHttpClient.java
@@ -0,0 +1,58 @@
+package at.bitfire.davdroid.webdav;
+
+import org.apache.http.client.params.HttpClientParams;
+import org.apache.http.conn.ClientConnectionManager;
+import org.apache.http.conn.scheme.PlainSocketFactory;
+import org.apache.http.conn.scheme.Scheme;
+import org.apache.http.conn.scheme.SchemeRegistry;
+import org.apache.http.impl.client.DefaultHttpClient;
+import org.apache.http.impl.conn.tsccm.ThreadSafeClientConnManager;
+import org.apache.http.params.BasicHttpParams;
+import org.apache.http.params.CoreProtocolPNames;
+import org.apache.http.params.HttpConnectionParams;
+import org.apache.http.params.HttpParams;
+
+import at.bitfire.davdroid.Constants;
+import at.bitfire.davdroid.webdav.GzipDecompressingEntity;
+import at.bitfire.davdroid.webdav.TlsSniSocketFactory;
+
+// see AndroidHttpClient
+
+
+public class DavHttpClient extends DefaultHttpClient {
+	private static DavHttpClient httpClient = null;
+	
+	private DavHttpClient(ClientConnectionManager connectionManager, HttpParams params) {
+		super(connectionManager, params);
+	}
+	
+	
+	public static synchronized DefaultHttpClient getInstance() {
+		if (httpClient == null) {
+			HttpParams params = new BasicHttpParams();
+			params.setParameter(CoreProtocolPNames.USER_AGENT, "DAVdroid/" + Constants.APP_VERSION);
+			
+			// use defaults of AndroidHttpClient
+			HttpConnectionParams.setConnectionTimeout(params, 20 * 1000);
+			HttpConnectionParams.setSoTimeout(params, 20 * 1000);
+			HttpConnectionParams.setSocketBufferSize(params, 8192);
+			HttpConnectionParams.setStaleCheckingEnabled(params, false);
+			
+			// don't allow redirections
+			HttpClientParams.setRedirecting(params, false);
+			
+			// use our own, SNI-capable LayeredSocketFactory for https://
+		    SchemeRegistry schemeRegistry = new SchemeRegistry();
+		    schemeRegistry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80));
+		    schemeRegistry.register(new Scheme("https", new TlsSniSocketFactory(), 443));
+		    
+			httpClient = new DavHttpClient(new ThreadSafeClientConnManager(params, schemeRegistry), params);
+			
+			// allow gzip compression
+			GzipDecompressingEntity.enable(httpClient);
+		}
+		
+		return httpClient;
+	}
+	
+}
diff --git a/src/at/bitfire/davdroid/webdav/InvalidDavResponseException.java b/src/at/bitfire/davdroid/webdav/InvalidDavResponseException.java
index 009f648b..884f3389 100644
--- a/src/at/bitfire/davdroid/webdav/InvalidDavResponseException.java
+++ b/src/at/bitfire/davdroid/webdav/InvalidDavResponseException.java
@@ -5,7 +5,7 @@ import org.apache.http.HttpException;
 public class InvalidDavResponseException extends HttpException {
 	private static final long serialVersionUID = -2118919144443165706L;
 	
-	public InvalidDavResponseException() {
-		super("Invalid DAV response");
+	public InvalidDavResponseException(String message) {
+		super("Invalid DAV response: " + message);
 	}
 }
diff --git a/src/at/bitfire/davdroid/webdav/WebDavResource.java b/src/at/bitfire/davdroid/webdav/WebDavResource.java
index e1a2807f..63853e9f 100644
--- a/src/at/bitfire/davdroid/webdav/WebDavResource.java
+++ b/src/at/bitfire/davdroid/webdav/WebDavResource.java
@@ -39,18 +39,13 @@ import org.apache.http.client.methods.HttpDelete;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpOptions;
 import org.apache.http.client.methods.HttpPut;
-import org.apache.http.client.params.ClientPNames;
-import org.apache.http.conn.scheme.Scheme;
-import org.apache.http.conn.scheme.SchemeRegistry;
 import org.apache.http.entity.ByteArrayEntity;
 import org.apache.http.impl.client.DefaultHttpClient;
 import org.apache.http.message.BasicLineParser;
-import org.apache.http.params.CoreProtocolPNames;
 import org.simpleframework.xml.Serializer;
 import org.simpleframework.xml.core.Persister;
 
 import android.util.Log;
-import at.bitfire.davdroid.Constants;
 import at.bitfire.davdroid.URIUtils;
 
 
@@ -91,7 +86,7 @@ public class WebDavResource {
 	// content (available after GET)
 	@Getter protected InputStream content;
 
-	protected DefaultHttpClient client;
+	protected DefaultHttpClient client = DavHttpClient.getInstance();
 	
 	
 	public WebDavResource(URI baseURL, boolean trailingSlash) throws URISyntaxException {
@@ -99,20 +94,6 @@ public class WebDavResource {
 		
 		if (trailingSlash && !location.getRawPath().endsWith("/"))
 			location = new URI(location.getScheme(), location.getSchemeSpecificPart() + "/", null);
-		
-		// create new HTTP client
-		client = new DefaultHttpClient();
-		client.getParams().setParameter(CoreProtocolPNames.USER_AGENT, "DAVdroid/" + Constants.APP_VERSION);
-		
-		// use our own, SNI-capable LayeredSocketFactory for https://
-	    SchemeRegistry schemeRegistry = client.getConnectionManager().getSchemeRegistry();
-	    schemeRegistry.register(new Scheme("https", new TlsSniSocketFactory(), 443));
-		
-		// allow gzip compression
-		GzipDecompressingEntity.enable(client);
-		
-		// redirections
-		client.getParams().setBooleanParameter(ClientPNames.HANDLE_REDIRECTS, false);
 	}
 	
 	public WebDavResource(URI baseURL, String username, String password, boolean preemptive, boolean trailingSlash) throws URISyntaxException {
@@ -130,12 +111,10 @@ public class WebDavResource {
 
 	protected WebDavResource(WebDavResource parent, URI uri) {
 		location = uri;
-		client = parent.client;
 	}
 	
 	public WebDavResource(WebDavResource parent, String member) {
 		location = parent.location.resolve(URIUtils.sanitize(member));
-		client = parent.client;
 	}
 	
 	public WebDavResource(WebDavResource parent, String member, boolean trailingSlash) {
@@ -146,7 +125,7 @@ public class WebDavResource {
 		this(parent, member);
 		properties.put(Property.ETAG, ETag);
 	}
-	
+
 	
 	protected void checkResponse(HttpResponse response) throws HttpException {
 		checkResponse(response.getStatusLine());
@@ -261,29 +240,36 @@ public class WebDavResource {
 	
 	/* collection operations */
 	
-	public boolean propfind(HttpPropfind.Mode mode) throws IOException, InvalidDavResponseException, HttpException {
+	public void propfind(HttpPropfind.Mode mode) throws IOException, InvalidDavResponseException, HttpException {
 		HttpPropfind propfind = new HttpPropfind(location, mode);
 		HttpResponse response = client.execute(propfind);
 		checkResponse(response);
-		
+
 		if (response.getStatusLine().getStatusCode() == HttpStatus.SC_MULTI_STATUS) {
+			InputStream content = response.getEntity().getContent();
+			if (content == null)
+				throw new InvalidDavResponseException("Multistatus response without content");
+
+			// duplicate content for logging
+			ByteArrayOutputStream logStream = new ByteArrayOutputStream();
+			InputStream is = new TeeInputStream(content, logStream);
+
 			DavMultistatus multistatus;
 			try {
 				Serializer serializer = new Persister();
-				ByteArrayOutputStream baos = new ByteArrayOutputStream();
-				InputStream is = new TeeInputStream(response.getEntity().getContent(), baos);
 				multistatus = serializer.read(DavMultistatus.class, is, false);
-				
-				Log.d(TAG, "Received multistatus response: " + baos.toString("UTF-8"));
 			} catch (Exception ex) {
 				Log.w(TAG, "Invalid PROPFIND XML response", ex);
-				throw new InvalidDavResponseException();
+				throw new InvalidDavResponseException("Invalid PROPFIND response");
+			} finally {
+				Log.d(TAG, "Received multistatus response:\n" + logStream.toString("UTF-8"));
+				is.close();
+				content.close();
 			}
 			processMultiStatus(multistatus);
-			return true;
 			
 		} else
-			return false;
+			throw new InvalidDavResponseException("Multistatus response expected");
 	}
 
 	public void multiGet(String[] names, MultigetType type) throws IOException, InvalidDavResponseException, HttpException {
@@ -307,7 +293,7 @@ public class WebDavResource {
 			serializer.write(multiget, writer);
 		} catch (Exception ex) {
 			Log.e(TAG, "Couldn't create XML multi-get request", ex);
-			throw new InvalidDavResponseException();
+			throw new InvalidDavResponseException("Couldn't create multi-get request");
 		}
 
 		HttpReport report = new HttpReport(location, writer.toString());
@@ -315,21 +301,30 @@ public class WebDavResource {
 		checkResponse(response);
 		
 		if (response.getStatusLine().getStatusCode() == HttpStatus.SC_MULTI_STATUS) {
+			InputStream content = response.getEntity().getContent();
+			if (content == null)
+				throw new InvalidDavResponseException("Multistatus response without content");
+			
 			DavMultistatus multistatus;
+			
+			// duplicate content for logging
+			ByteArrayOutputStream logStream = new ByteArrayOutputStream();
+			InputStream is = new TeeInputStream(content, logStream, true);
+			
 			try {
-				ByteArrayOutputStream baos = new ByteArrayOutputStream();
-				InputStream is = new TeeInputStream(response.getEntity().getContent(), baos);
 				multistatus = serializer.read(DavMultistatus.class, is, false);
-				
-				Log.d(TAG, "Received multistatus response: " + baos.toString("UTF-8"));
 			} catch (Exception ex) {
 				Log.e(TAG, "Couldn't parse multi-get response", ex);
-				throw new InvalidDavResponseException();
+				throw new InvalidDavResponseException("Invalid multi-get response");
+			} finally {
+				Log.d(TAG, "Received multistatus response:\n" + logStream.toString("UTF-8"));
+				is.close();
+				content.close();
 			}
 			processMultiStatus(multistatus);
 			
 		} else
-			throw new InvalidDavResponseException();
+			throw new InvalidDavResponseException("Multistatus response expected");
 	}
 
 	
@@ -393,7 +388,7 @@ public class WebDavResource {
 			
 			// about which resource is this response?
 			WebDavResource referenced = null;
-			if (URIUtils.isSame(location, href)) {	// -> ourselves
+			if (location.equals(href)) {	// -> ourselves
 				referenced = this;
 				
 			} else {						// -> about a member
diff --git a/test/src/at/bitfire/davdroid/test/URIUtilsTest.java b/test/src/at/bitfire/davdroid/test/URIUtilsTest.java
index 558523a6..549643b2 100644
--- a/test/src/at/bitfire/davdroid/test/URIUtilsTest.java
+++ b/test/src/at/bitfire/davdroid/test/URIUtilsTest.java
@@ -17,11 +17,6 @@ public class URIUtilsTest extends InstrumentationTestCase {
 		baseURI = new URI(BASE_URI);
 	}
 
-	
-	public void testIsSame() throws URISyntaxException {
-		assertTrue(URIUtils.isSame(new URI(ROOT_URI + "my@email/"), new URI(ROOT_URI + "my%40email/")));
-	}
-
 	public void testSanitize() {
 		assertEquals("/my%40email.com/dir", URIUtils.sanitize("/my@email.com/dir"));
 		assertEquals("my%3Afile.vcf", URIUtils.sanitize("my:file.vcf"));
diff --git a/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java b/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java
index 20ecfa8a..322c61e6 100644
--- a/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java
+++ b/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java
@@ -82,10 +82,14 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 	}
 
 	public void testPropfindCurrentUserPrincipal() throws IOException, HttpException {
-		assertTrue(davCollection.propfind(HttpPropfind.Mode.CURRENT_USER_PRINCIPAL));
+		davCollection.propfind(HttpPropfind.Mode.CURRENT_USER_PRINCIPAL);
 		assertEquals("/dav/principals/users/test", davCollection.getCurrentUserPrincipal());
 		
-		assertFalse(simpleFile.propfind(HttpPropfind.Mode.CURRENT_USER_PRINCIPAL));
+		try {
+			simpleFile.propfind(HttpPropfind.Mode.CURRENT_USER_PRINCIPAL);
+			fail();
+		} catch(InvalidDavResponseException ex) {
+		}
 		assertNull(simpleFile.getCurrentUserPrincipal());
 	}
 		
@@ -130,10 +134,9 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 		WebDavResource redirection = new WebDavResource(new URI(ROBOHYDRA_BASE + "redirect"), false);
 		try {
 			redirection.get();
+			fail();
 		} catch (HttpException e) {
-			return;
 		}
-		fail();
 	}
 	
 	public void testGet() throws URISyntaxException, IOException, HttpException {
@@ -160,10 +163,9 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 		// should fail on an existing file
 		try {
 			davExistingFile.put(SAMPLE_CONTENT, PutMode.ADD_DONT_OVERWRITE);
+			fail();
 		} catch(PreconditionFailedException ex) {
-			return;
 		}
-		fail();
 	}
 	
 	public void testPutUpdateDontOverwrite() throws IOException, HttpException {
@@ -173,10 +175,9 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 		// should fail on a non-existing file
 		try {
 			davNonExistingFile.put(SAMPLE_CONTENT, PutMode.UPDATE_DONT_OVERWRITE);
+			fail();
 		} catch(PreconditionFailedException ex) {
-			return;
 		}
-		fail();
 	}
 	
 	public void testDelete() throws IOException, HttpException {
@@ -186,10 +187,9 @@ public class WebDavResourceTest extends InstrumentationTestCase {
 		// should fail on a non-existing file
 		try {
 			davNonExistingFile.delete();
+			fail();
 		} catch (NotFoundException e) {
-			return;
 		}
-		fail();
 	}