Crashes after updating to 4.4.3-beta.1

[rfc2822]

Shortly after updating to 4.4.3-beta.1, I got a "DAVx5 keeps crashing" dialog on my production device.

Will have to look up the crash in Google Play.

[sunkup]

I found one single crash in the play console

Exception java.lang.IllegalArgumentException:
  at at.bitfire.davdroid.sync.SyncAdapterService$SyncAdapter.onPerformSync (SyncAdapterServices.kt:97)
  at android.content.AbstractThreadedSyncAdapter$SyncThread.run (AbstractThreadedSyncAdapter.java:354)

See the diff here.

It's probably due to the old address books (after update to version 4.4.3) not beeing removed.

Reproducible with the following steps:

1. Install pre 4.4.3 version / check out, build and install commit before 4.4.3
2. Setup test account and sync one or more address books -> "old" address book accounts are created
3. Update to 4.4.3 version / check out latest commit, build and install
4. Sync again -> "new" address book accounts are created, but "old" address book accounts remain
5. Change/Update a contact of an old address book -> sync via sync adapter framework gets executed and exception is thrown

11:01:02.950  E  FATAL EXCEPTION: SyncAdapterThread-1
                 Process: at.bitfire.davdroid, PID: 7643
                 java.lang.IllegalArgumentException: No valid collection/service/account for address book Account {name=Flap it ([email protected] Uw), type=at.bitfire.davdroid.address_book}
                 	at at.bitfire.davdroid.sync.SyncAdapterService$SyncAdapter.onPerformSync(SyncAdapterServices.kt:97)
                 	at android.content.AbstractThreadedSyncAdapter$SyncThread.run(AbstractThreadedSyncAdapter.java:334)

[sunkup]

@rfc2822 Where and when do you think we should do the migration? Should we simply rename the accounts, right away when the new version is installed? Where exactly do you hook into that?

Since next sync will create the "new" address book accounts, we could - alternatively - just enqueue the AccountsCleanupWorker which would remove the old ones then.

[rfc2822]

Since next sync will create the "new" address book accounts, we could - alternatively - just enqueue the AccountsCleanupWorker which would remove the old ones then.

I wonder why the cleanup worker is even required. Shouldn't the Syncer detect all unused address books and delete them? Or is it because they don't have a collection ID, so they will be ignored?

My original thought was that the Syncer will create & remove the address books so that we wouldn't even need to call the cleanup worker. (The cleanup worker is by the way also called daily.)

However I now think there's a reason for migrating the address books: when the address books are just created & deleted, pending changes might be lost:

1. User adds contact, but server is not reachable at the moment
2. DAVx⁵ is updated
3. DAVx⁵ creates & syncs new address books
4. DAVx⁵ deletes the old address books, including the pending changes

So we should migrate the address books to have a correct collection ID I guess. They should then be renamed automatically by LocalAddressBook.update(). So we don't have to synchronize everything anew and pending changes are not list.

Nevertheless we should make sure that address books without collection ID are also handled gracefully. The case that the collection ID is lost by the system etc. can always occur.

Where and when do you think we should do the migration?

AccountSettings, as you did already

---

What do you think?

[sunkup]

I wonder why the cleanup worker is even required. Shouldn't the Syncer detect all unused address books and delete them? Or is it because they don't have a collection ID, so they will be ignored?

I think that's because the sync algorithm in Syncer ist still using the URL instead of the collection ID.

My original thought was that the Syncer will create & remove the address books so that we wouldn't even need to call the cleanup worker. (The cleanup worker is by the way also called daily.)

That would of course be the best.

So we should migrate the address books to have a correct collection ID I guess. They should then be renamed automatically by LocalAddressBook.update(). So we don't have to synchronize everything anew and pending changes are not list.

I agree.

Nevertheless we should make sure that address books without collection ID are also handled gracefully. The case that the collection ID is lost by the system etc. can always occur.

The AccountsCleanupWorker would take care of that then.

[rfc2822]

I agree.

So we should have a migration that just adds the collection ID.

---

I think that's because the sync algorithm in Syncer ist still using the URL instead of the collection ID.
…
The AccountsCleanupWorker would take care of that then.

Ah, well, then let's the cleanup worker do that (usually when it runs periodically). Maybe we should have an AccountsCleanupWorkerTest for that?

However in case of proper migration, the cleanup worker should have nothing to do.