From 3cab688782648715c0677a20efac255316d59d8c Mon Sep 17 00:00:00 2001 From: rfc2822 Date: Fri, 23 May 2014 13:00:19 +0200 Subject: [PATCH] Various upload changes * fix issue when first upload fails (fixes #233) * show debug settings menu item in main activity * update ETag in local database when a PUT request returns an updated ETag, meaning that locally created/updated records won't be downloaded from the server immediately after the upload --- res/menu/main_activity.xml | 1 + src/at/bitfire/davdroid/MainActivity.java | 6 ++ .../davdroid/resource/LocalCalendar.java | 2 +- .../davdroid/resource/LocalCollection.java | 28 +++++-- .../davdroid/resource/RemoteCollection.java | 16 +++- .../bitfire/davdroid/resource/Resource.java | 2 +- .../davdroid/syncadapter/SyncManager.java | 8 +- .../davdroid/webdav/WebDavResource.java | 9 ++- test/robohydra/plugins/dav-default/index.js | 8 +- .../resource/test/LocalCalendarTest.java | 78 +++++++++++++++---- .../webdav/test/WebDavResourceTest.java | 4 +- 11 files changed, 127 insertions(+), 35 deletions(-) diff --git a/res/menu/main_activity.xml b/res/menu/main_activity.xml index 257651ea..c4915016 100644 --- a/res/menu/main_activity.xml +++ b/res/menu/main_activity.xml @@ -3,4 +3,5 @@ + diff --git a/src/at/bitfire/davdroid/MainActivity.java b/src/at/bitfire/davdroid/MainActivity.java index 005c7d31..1c7e265b 100644 --- a/src/at/bitfire/davdroid/MainActivity.java +++ b/src/at/bitfire/davdroid/MainActivity.java @@ -22,6 +22,7 @@ import android.view.MenuInflater; import android.view.MenuItem; import android.view.View; import android.widget.TextView; +import at.bitfire.davdroid.syncadapter.GeneralSettingsActivity; public class MainActivity extends Activity { @@ -55,6 +56,11 @@ public class MainActivity extends Activity { Intent intent = new Intent(Settings.ACTION_ADD_ACCOUNT); startActivity(intent); } + + public void showDebugSettings(MenuItem item) { + Intent intent = new Intent(this, GeneralSettingsActivity.class); + startActivity(intent); + } public void showSyncSettings(MenuItem item) { Intent intent = new Intent(Settings.ACTION_SYNC_SETTINGS); diff --git a/src/at/bitfire/davdroid/resource/LocalCalendar.java b/src/at/bitfire/davdroid/resource/LocalCalendar.java index 5761d07a..11a24353 100644 --- a/src/at/bitfire/davdroid/resource/LocalCalendar.java +++ b/src/at/bitfire/davdroid/resource/LocalCalendar.java @@ -158,7 +158,7 @@ public class LocalCalendar extends LocalCollection { return calendars.toArray(new LocalCalendar[0]); } - public LocalCalendar(Account account, ContentProviderClient providerClient, int id, String url, String cTag) throws RemoteException { + public LocalCalendar(Account account, ContentProviderClient providerClient, long id, String url, String cTag) throws RemoteException { super(account, providerClient); this.id = id; this.url = url; diff --git a/src/at/bitfire/davdroid/resource/LocalCollection.java b/src/at/bitfire/davdroid/resource/LocalCollection.java index c284c92e..8bf43585 100644 --- a/src/at/bitfire/davdroid/resource/LocalCollection.java +++ b/src/at/bitfire/davdroid/resource/LocalCollection.java @@ -68,8 +68,8 @@ public abstract class LocalCollection { // content provider (= database) querying public long[] findNew() throws LocalStorageException { - // new records are 1) dirty, and 2) don't have a remote file name yet - String where = entryColumnDirty() + "=1 AND " + entryColumnRemoteName() + " IS NULL"; + // new records are 1) dirty, and 2) don't have an ETag yet + String where = entryColumnDirty() + "=1 AND " + entryColumnETag() + " IS NULL"; if (entryColumnParentID() != null) where += " AND " + entryColumnParentID() + "=" + String.valueOf(getId()); try { @@ -101,8 +101,8 @@ public abstract class LocalCollection { } public long[] findUpdated() throws LocalStorageException { - // updated records are 1) dirty, and 2) already have a remote file name - String where = entryColumnDirty() + "=1 AND " + entryColumnRemoteName() + " IS NOT NULL"; + // updated records are 1) dirty, and 2) already have an ETag + String where = entryColumnDirty() + "=1 AND " + entryColumnETag() + " IS NOT NULL"; if (entryColumnParentID() != null) where += " AND " + entryColumnParentID() + "=" + String.valueOf(getId()); try { @@ -110,12 +110,12 @@ public abstract class LocalCollection { new String[] { entryColumnID(), entryColumnRemoteName(), entryColumnETag() }, where, null, null); if (cursor == null) - throw new LocalStorageException("Couldn't query dirty records"); + throw new LocalStorageException("Couldn't query updated records"); - long[] dirty = new long[cursor.getCount()]; + long[] updated = new long[cursor.getCount()]; for (int idx = 0; cursor.moveToNext(); idx++) - dirty[idx] = cursor.getLong(0); - return dirty; + updated[idx] = cursor.getLong(0); + return updated; } catch(RemoteException ex) { throw new LocalStorageException(ex); } @@ -218,6 +218,18 @@ public abstract class LocalCollection { public abstract void deleteAllExceptRemoteNames(Resource[] remoteResources); + public void updateETag(Resource res, String eTag) throws LocalStorageException { + Log.d(TAG, "Setting ETag of local resource " + res + " to " + eTag); + + ContentValues values = new ContentValues(1); + values.put(entryColumnETag(), eTag); + try { + providerClient.update(ContentUris.withAppendedId(entriesURI(), res.getLocalID()), values, null, new String[] {}); + } catch (RemoteException e) { + throw new LocalStorageException(e); + } + } + public void clearDirty(Resource resource) { pendingOperations.add(ContentProviderOperation .newUpdate(ContentUris.withAppendedId(entriesURI(), resource.getLocalID())) diff --git a/src/at/bitfire/davdroid/resource/RemoteCollection.java b/src/at/bitfire/davdroid/resource/RemoteCollection.java index 0fb777a1..a734c6b7 100644 --- a/src/at/bitfire/davdroid/resource/RemoteCollection.java +++ b/src/at/bitfire/davdroid/resource/RemoteCollection.java @@ -127,14 +127,18 @@ public abstract class RemoteCollection { return resource; } - public void add(Resource res) throws IOException, HttpException, ValidationException { + // returns ETag of the created resource, if returned by server + public String add(Resource res) throws IOException, HttpException, ValidationException { WebDavResource member = new WebDavResource(collection, res.getName(), res.getETag()); member.setContentType(memberContentType()); @Cleanup ByteArrayOutputStream os = res.toEntity(); - member.put(os.toByteArray(), PutMode.ADD_DONT_OVERWRITE); + String eTag = member.put(os.toByteArray(), PutMode.ADD_DONT_OVERWRITE); + // after a successful upload, the collection has implicitely changed, too collection.invalidateCTag(); + + return eTag; } public void delete(Resource res) throws IOException, HttpException { @@ -144,13 +148,17 @@ public abstract class RemoteCollection { collection.invalidateCTag(); } - public void update(Resource res) throws IOException, HttpException, ValidationException { + // returns ETag of the updated resource, if returned by server + public String update(Resource res) throws IOException, HttpException, ValidationException { WebDavResource member = new WebDavResource(collection, res.getName(), res.getETag()); member.setContentType(memberContentType()); @Cleanup ByteArrayOutputStream os = res.toEntity(); - member.put(os.toByteArray(), PutMode.UPDATE_DONT_OVERWRITE); + String eTag = member.put(os.toByteArray(), PutMode.UPDATE_DONT_OVERWRITE); + // after a successful upload, the collection has implicitely changed, too collection.invalidateCTag(); + + return eTag; } } diff --git a/src/at/bitfire/davdroid/resource/Resource.java b/src/at/bitfire/davdroid/resource/Resource.java index 1582f05f..80fd1294 100644 --- a/src/at/bitfire/davdroid/resource/Resource.java +++ b/src/at/bitfire/davdroid/resource/Resource.java @@ -20,7 +20,7 @@ import lombok.ToString; @ToString public abstract class Resource { - @Getter protected String name, ETag; + @Getter @Setter protected String name, ETag; @Getter @Setter protected String uid; @Getter protected long localID; diff --git a/src/at/bitfire/davdroid/syncadapter/SyncManager.java b/src/at/bitfire/davdroid/syncadapter/SyncManager.java index e63e8493..558283b9 100644 --- a/src/at/bitfire/davdroid/syncadapter/SyncManager.java +++ b/src/at/bitfire/davdroid/syncadapter/SyncManager.java @@ -138,7 +138,9 @@ public class SyncManager { for (long id : newIDs) try { Resource res = local.findById(id, true); - remote.add(res); + String eTag = remote.add(res); + if (eTag != null) + local.updateETag(res, eTag); local.clearDirty(res); count++; } catch(PreconditionFailedException e) { @@ -162,7 +164,9 @@ public class SyncManager { for (long id : dirtyIDs) { try { Resource res = local.findById(id, true); - remote.update(res); + String eTag = remote.update(res); + if (eTag != null) + local.updateETag(res, eTag); local.clearDirty(res); count++; } catch(PreconditionFailedException e) { diff --git a/src/at/bitfire/davdroid/webdav/WebDavResource.java b/src/at/bitfire/davdroid/webdav/WebDavResource.java index 727eb760..3593a868 100644 --- a/src/at/bitfire/davdroid/webdav/WebDavResource.java +++ b/src/at/bitfire/davdroid/webdav/WebDavResource.java @@ -338,7 +338,8 @@ public class WebDavResource { } } - public void put(byte[] data, PutMode mode) throws IOException, HttpException { + // returns the ETag of the created/updated resource, if available (null otherwise) + public String put(byte[] data, PutMode mode) throws IOException, HttpException { HttpPut put = new HttpPut(location); put.setEntity(new ByteArrayEntity(data)); @@ -357,9 +358,15 @@ public class WebDavResource { CloseableHttpResponse response = httpClient.execute(put, context); try { checkResponse(response); + + Header eTag = response.getLastHeader("ETag"); + if (eTag != null) + return eTag.getValue(); } finally { response.close(); } + + return null; } public void delete() throws IOException, HttpException { diff --git a/test/robohydra/plugins/dav-default/index.js b/test/robohydra/plugins/dav-default/index.js index 37102494..8ca613c3 100644 --- a/test/robohydra/plugins/dav-default/index.js +++ b/test/robohydra/plugins/dav-default/index.js @@ -134,8 +134,10 @@ exports.getBodyParts = function(conf) { if (req.method == "PUT") { if (req.headers['if-match']) /* can't overwrite new file */ res.statusCode = 412; - else + else { res.statusCode = 201; + res.headers["ETag"] = "has-just-been-created"; + } } else if (req.method == "DELETE") res.statusCode = 404; @@ -149,8 +151,10 @@ exports.getBodyParts = function(conf) { if (req.method == "PUT") { if (req.headers['if-none-match']) /* requested "don't overwrite", but this file exists */ res.statusCode = 412; - else + else { res.statusCode = 204; + res.headers["ETag"] = "has-just-been-updated"; + } } else if (req.method == "DELETE") res.statusCode = 204; diff --git a/test/src/at/bitfire/davdroid/resource/test/LocalCalendarTest.java b/test/src/at/bitfire/davdroid/resource/test/LocalCalendarTest.java index 9a171313..3733d71c 100644 --- a/test/src/at/bitfire/davdroid/resource/test/LocalCalendarTest.java +++ b/test/src/at/bitfire/davdroid/resource/test/LocalCalendarTest.java @@ -1,6 +1,9 @@ package at.bitfire.davdroid.resource.test; +import java.util.Calendar; + import lombok.Cleanup; +import android.accounts.Account; import android.annotation.TargetApi; import android.content.ContentProviderClient; import android.content.ContentResolver; @@ -9,40 +12,51 @@ import android.content.ContentValues; import android.database.Cursor; import android.net.Uri; import android.os.Build; +import android.os.RemoteException; import android.provider.CalendarContract; import android.provider.CalendarContract.Attendees; import android.provider.CalendarContract.Calendars; import android.provider.CalendarContract.Events; import android.provider.CalendarContract.Reminders; import android.test.InstrumentationTestCase; +import android.util.Log; +import at.bitfire.davdroid.resource.LocalCalendar; +import at.bitfire.davdroid.resource.LocalStorageException; public class LocalCalendarTest extends InstrumentationTestCase { - private static final String calendarName = "DavdroidTest"; + private static final String + TAG = "davroid.LocalCalendarTest", + calendarName = "DAVdroid_Test"; - ContentProviderClient client; - long calendarID; + ContentProviderClient providerClient; + Account testAccount = new Account(calendarName, CalendarContract.ACCOUNT_TYPE_LOCAL); + LocalCalendar testCalendar; @TargetApi(Build.VERSION_CODES.ICE_CREAM_SANDWICH_MR1) protected void setUp() throws Exception { // get content resolver ContentResolver resolver = getInstrumentation().getContext().getContentResolver(); - client = resolver.acquireContentProviderClient(CalendarContract.AUTHORITY); + providerClient = resolver.acquireContentProviderClient(CalendarContract.AUTHORITY); - @Cleanup Cursor cursor = client.query(Calendars.CONTENT_URI, + long id; + + @Cleanup Cursor cursor = providerClient.query(Calendars.CONTENT_URI, new String[] { Calendars._ID }, Calendars.ACCOUNT_TYPE + "=? AND " + Calendars.NAME + "=?", new String[] { CalendarContract.ACCOUNT_TYPE_LOCAL, calendarName }, null); if (cursor.moveToNext()) { // found local test calendar - calendarID = cursor.getLong(0); + id = cursor.getLong(0); + Log.i(TAG, "Found test calendar with ID " + id); + } else { // no local test calendar found, create ContentValues values = new ContentValues(); - values.put(Calendars.ACCOUNT_NAME, calendarName); - values.put(Calendars.ACCOUNT_TYPE, CalendarContract.ACCOUNT_TYPE_LOCAL); + values.put(Calendars.ACCOUNT_NAME, testAccount.name); + values.put(Calendars.ACCOUNT_TYPE, testAccount.type); values.put(Calendars.NAME, calendarName); values.put(Calendars.CALENDAR_DISPLAY_NAME, calendarName); values.put(Calendars.CALENDAR_ACCESS_LEVEL, Calendars.CAL_ACCESS_OWNER); @@ -55,21 +69,39 @@ public class LocalCalendarTest extends InstrumentationTestCase { values.put(Calendars.ALLOWED_ATTENDEE_TYPES, Attendees.TYPE_NONE + "," + Attendees.TYPE_OPTIONAL + "," + Attendees.TYPE_REQUIRED + "," + Attendees.TYPE_RESOURCE); } - Uri calendarURI = client.insert(calendarsURI(), values); - calendarID = ContentUris.parseId(calendarURI); + Uri calendarURI = providerClient.insert(calendarsURI(), values); + + id = ContentUris.parseId(calendarURI); + Log.i(TAG, "Created test calendar with ID " + id); } + + testCalendar = new LocalCalendar(testAccount, providerClient, id, null, null); } protected void tearDown() throws Exception { - Uri uri = ContentUris.withAppendedId(calendarsURI(), calendarID); - client.delete(uri, null, null); + Uri uri = ContentUris.withAppendedId(calendarsURI(), testCalendar.getId()); + providerClient.delete(uri, null, null); } // tests - public void testNothing() { - assert(true); + public void testFindNew() throws LocalStorageException, RemoteException { + // at the beginning, there are no dirty events + assertTrue(testCalendar.findNew().length == 0); + assertTrue(testCalendar.findUpdated().length == 0); + + // insert a "new" event + insertNewEvent(); + + // there must be one "new" event now + assertTrue(testCalendar.findNew().length == 1); + assertTrue(testCalendar.findUpdated().length == 0); + + // nothing has changed, the record must still be "new" + // see issue #233 + assertTrue(testCalendar.findNew().length == 1); + assertTrue(testCalendar.findUpdated().length == 0); } @@ -82,5 +114,23 @@ public class LocalCalendarTest extends InstrumentationTestCase { .appendQueryParameter(CalendarContract.CALLER_IS_SYNCADAPTER, "true"). build(); } + + protected long insertNewEvent() throws LocalStorageException, RemoteException { + Uri uri = Events.CONTENT_URI.buildUpon() + .appendQueryParameter(Calendars.ACCOUNT_TYPE, testAccount.type) + .appendQueryParameter(Calendars.ACCOUNT_NAME, testAccount.name) + .appendQueryParameter(CalendarContract.CALLER_IS_SYNCADAPTER, "true") + .build(); + + ContentValues values = new ContentValues(); + values.put(Events.CALENDAR_ID, testCalendar.getId()); + values.put(Events.TITLE, "Test Event"); + values.put(Events.ALL_DAY, 0); + values.put(Events.DTSTART, Calendar.getInstance().getTimeInMillis()); + values.put(Events.DTEND, Calendar.getInstance().getTimeInMillis()); + values.put(Events.EVENT_TIMEZONE, "UTC"); + values.put(Events.DIRTY, 1); + return ContentUris.parseId(providerClient.insert(uri, values)); + } } diff --git a/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java b/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java index 0f033ba7..2869f483 100644 --- a/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java +++ b/test/src/at/bitfire/davdroid/webdav/test/WebDavResourceTest.java @@ -183,7 +183,7 @@ public class WebDavResourceTest extends InstrumentationTestCase { public void testPutAddDontOverwrite() throws IOException, HttpException { // should succeed on a non-existing file - davNonExistingFile.put(SAMPLE_CONTENT, PutMode.ADD_DONT_OVERWRITE); + assertEquals("has-just-been-created", davNonExistingFile.put(SAMPLE_CONTENT, PutMode.ADD_DONT_OVERWRITE)); // should fail on an existing file try { @@ -195,7 +195,7 @@ public class WebDavResourceTest extends InstrumentationTestCase { public void testPutUpdateDontOverwrite() throws IOException, HttpException { // should succeed on an existing file - davExistingFile.put(SAMPLE_CONTENT, PutMode.UPDATE_DONT_OVERWRITE); + assertEquals("has-just-been-updated", davExistingFile.put(SAMPLE_CONTENT, PutMode.UPDATE_DONT_OVERWRITE)); // should fail on a non-existing file try {