Merge Form Provider and Insance Provider to App DB #1940

shubham1g5 · 2018-01-31T13:31:53Z

Summary:

Replaces FormProvider with FormDefRecord
Merges InstanceProvider into FormRecord
Refactors to replace all URIs dependent code to work on FormDefRecord sql ids and in process creates new versions for XFormAndroidInstaller and FormRecord
Takes care of migration from old user/app DBs to their newer versions

…new Form and Instance tables in app DB

ctsims · 2018-02-22T15:42:51Z

@shubham1g5 Hey, Shubham. Maybe easiest for us to chat over voice, I think my main point of confusion is that it's still not obvious to me why we need to maintain FormRecord and InstanceRecord as different models, rather than unifying them. Can you clarify that specific point at a high level?

ctsims · 2018-02-22T15:43:47Z

app/src/org/commcare/android/database/app/models/FormDefRecord.java

+
+    public static final String STORAGE_KEY = "form_def";
+
+    public static final String META_DISPLAY_NAME = "displayName";


Did we audit whether we need to retain all of these? I think many/most of these properties can actually be eliminated, but i could be wrong about that

Yeah, Some of these are actually not getting used right now. I think I was being overly cautious to not create unnecessary changes as part of this migration. Can remove these now though.

@ctsims Here is a list of all fields that are not getting used currently -

Instance Provider/ Instance Record

submissionUri

date (last status change date)

displaySubtext

Form Def

description

submissionUri

displaySubtext

md5Hash

date

jrcacheFilePath

language

Just want to make sure we are fine with removing all of these.

I'm surprised to hear that we aren't using "last status change date" anywhere, is that still true after Aliza's recent fix for form record ordering?

I don't believe we use any of the formdef fields, though.

The private key is also unused, right? that's some weird odk encryption stuff (we use our Symetric key, but not their public/private keypairs I thought)

Reg: "last status change date": You are right. We had 2 date fields one in FormRecord and other in InstacneRecord. Now in the merged FormRecord I have retained the original FormRecord field which also gets used for form ordering.

Reg: "private key" : There is no private key currently in FormDefRecord. So guessing we already removed it earlier. Though we do use the public key here

I am 99% that EncryptedFormInformation stuff is only used by ODK. That class is only called while reading forms currently (and we've never written any that way), if you trace the code back, so I think we can remove it without concern.

Cool. Going to remove the EncryptionUtils.getEncryptedFormInformation as well since that will always return null in case of no public key

ctsims · 2018-02-22T15:48:46Z

app/src/org/commcare/models/database/app/AppDatabaseUpgrader.java

+import org.commcare.provider.FormsProviderAPI;
+import org.commcare.provider.InstanceProviderAPI;
 import org.commcare.resources.model.Resource;
+import org.javarosa.core.util.externalizable.PrototypeFactory;


I think that this file needs to have all of the upgradeXtoY methods from 1 to 8 audited to make sure their records use the old version of the classes. We need to load old Resources with the V1 installer for XForms, for example, right?

Good point though none of these methods seem to be loading Resources i.e. performing a read on Resource tables!

ctsims · 2018-02-22T15:49:41Z

app/src/org/commcare/models/database/app/AppDatabaseUpgrader.java

+    // Migrate records form FormProvider and InstanceProvider to new FormDefRecord and InstanceRecord respectively
+    private boolean upgradeEightNine(SQLiteDatabase db) {
+        db.beginTransaction();
+        android.database.Cursor cursor = null;


Strictly speaking no lines of code should occur after db.begin until the try starts.

ctsims · 2018-02-22T15:58:18Z

app/src/org/commcare/android/database/app/models/InstanceRecord.java

+        return sInstanceRecordStorage;
+    }
+
+    public static void setinstanceRecordStorage(SqlStorage<InstanceRecord> instanceRecordStorage) {


I'm not OK with managing this state statically. I know it's annoying, but I think we need to refactor to move all of the static functionality in this class to being called through some other pattern.

It would be OK in my mind for it to be linked through the platform through some context object, but it has only ever caused a lot of headache/tech debt to allow static maintenance of the storage sandboxes, so we should figure out how to make this something that comes from the platform.

will do, though I would be curious to know what kind of problems have we seen in past arising from storage sandboxes being singletons.

Two big things that have come up a lot:

We already have a lot of lifecycle objects around the code (java lifecycle, app lifecycle, user lifecycle) and it's a lot of overhead to maintain registering and unregistering data from them.

In many cases we are on a track to being able to decouple static state from apps entirely, which would give us pretty incredible capabilities like the ability to update apps in the background without interfering with seated applications. Adding new statically managed holders (rather than coupling them into another lifecycle object) significantly complicates those efforts by making a full refactor necessary and requiring us to inject the new lifecycle object in lots of places (this came up a ton with things like ReferenceManager, and it's a pattern we want to move away from)

Thanks! Currently we have different versions of getUserStorage and getAppStorage in Application class here. So If I just use these whenever I need instances of FormDefRecord and InstanceRecord instead of using the static instances, it should be fine right ? Or are we looking for something more here.

That should be fine. Passing the storage as a constructor to this (and/or wrapping that operation in a static getter from the Application) is a fine solution, since that puts us in the same state.

As a best-practice pattern I try to only call the singleton application methods from lifecycle objects (like the Activity classes), because it makes it easier to manage the outcome if the lifecycle is in a bad state (say, the user has been logged out).

shubham1g5 · 2018-02-22T20:54:30Z

@ctsims I did not think that we would like to merge the two tables. I was confused myself in utility of having them as separate tables though assumed that we might want to keep the separation as it is even in the new world. Will work on merging them now!

ctsims · 2018-02-22T21:19:27Z

@shubham1g5 The reason they were split was because for a while we were trying to assume we'd treat ODK as a separate service on the phone (when android looked like it was going to be able to work that way), so we had to add our metadata somewhere else (which is where we made FormRecord). Now that we're scrapping the form entry modularity I think it just adds overhead. Thanks for the merge!

shubham1g5 · 2018-02-26T11:58:49Z

@ctsims Is there a philosophy behind what things go to UserDb vs what goes to AppDb. When I created InstanceRecord and FormDefRecord, they both seemed app specific to me and so I added them to AppDB but now when I see the FormRecord it's part of UserDb. So now I am confused where should I put the merged version of FormRecord and InstanceRecord!

ctsims · 2018-02-26T16:31:42Z

@shubham1g5 FormRecords (and InstanceRecords) are UserDB Objects. They are encrypted by the user's keys and are protected assets for the user storage sandbox.

FormDefRecords are part of the App sandbox because they are a shared resource that is accessed by any user who is logged into the app and stored unencrypted.

shubham1g5 · 2018-02-27T10:08:19Z

@ctsims Thanks. Makes sense now!

'

shubham1g5 · 2018-03-19T09:35:16Z

@ctsims Ready to review again

ctsims

Thanks so much for the continued hard work on this!

This looks super super close to me. I made 2-3 types of comments.

Most critical - This code breaks legacy installs from the oldest version of our code. It may be that retaining that legacy code requires keeping around lots of the content provider code, which might not be worth it. We may need to discuss as a team
Transaction Semantics - Failures in certain parts of the code should I think be permitted given two bad alternatives which could result in busted states (one inevitable, one hypothetical)
General and minor error messaging and small feedback.

ctsims · 2018-04-09T21:51:18Z

app/src/org/commcare/activities/components/FormEntryInstanceState.java

+            instanceState.setFormDefPath(formDefRecord.getFilePath());
+            return new Pair<>(formDefRecord.getID(), isInstanceReadOnly);
+        } else if (formDefRecords.size() < 1) {
+            throw new FormEntryActivity.FormQueryException("Parent form does not exist");


I think "Parent" here is going to be a confusing word to whoever is reading this error.

It would be more helpful to describe "There was no XForm definition defined" and probably to also list the XMLNS of the form (although that may be more useful in a notification since it will be technobabble to most people)

ctsims · 2018-04-09T21:52:02Z

app/src/org/commcare/activities/components/FormEntryInstanceState.java

+        } else if (formDefRecords.size() < 1) {
+            throw new FormEntryActivity.FormQueryException("Parent form does not exist");
+        } else {
+            throw new FormEntryActivity.FormQueryException("More than one possible parent form");


For this error and the error above, we should also be soft-asserting (either here or where this is caught) very explicitly that this happened, along with the XMLNS

ctsims · 2018-04-09T21:54:27Z

app/src/org/commcare/android/database/app/models/FormDefRecord.java

+
+    public static final String STORAGE_KEY = "form_def";
+
+    public static final String META_DISPLAY_NAME = "displayName";


I'm surprised to hear that we aren't using "last status change date" anywhere, is that still true after Aliza's recent fix for form record ordering?

I don't believe we use any of the formdef fields, though.

ctsims · 2018-04-09T21:55:29Z

app/src/org/commcare/android/database/app/models/FormDefRecord.java

+
+    public static final String STORAGE_KEY = "form_def";
+
+    public static final String META_DISPLAY_NAME = "displayName";


The private key is also unused, right? that's some weird odk encryption stuff (we use our Symetric key, but not their public/private keypairs I thought)

ctsims · 2018-04-09T21:57:15Z

app/src/org/commcare/android/database/app/models/FormDefRecord.java

+    public int save(SqlStorage<FormDefRecord> formDefRecordStorage) {
+        // if we don't have a path to the file, the rest are irrelevant.
+        // it should fail anyway because you can't have a null file path.
+        if (StringUtils.isEmpty(mFormFilePath)) {


This seems like a bit of an odd time to fail on this to me. Can't we validate that this path isn't empty when it is being set, rather than when it is being used?

ctsims · 2018-04-09T22:01:21Z

app/src/org/commcare/android/database/user/models/FormRecord.java

    public static final String STATUS_JUST_DELETED = "just-deleted";
+    private static final String TAG = FormRecord.class.getName();
+
+    @StringDef({STATUS_UNSTARTED, STATUS_INCOMPLETE, STATUS_COMPLETE, STATUS_UNSENT, STATUS_SAVED, STATUS_QUARANTINED, STATUS_UNINDEXED, STATUS_JUST_DELETED})


This is a really cool pattern, thanks for sharing it!

Yeah. It also saves memory overhead caused by Enums as these annotations are compile time only.

ctsims · 2018-04-09T22:13:06Z

app/src/org/commcare/models/database/app/AppDatabaseUpgrader.java

+        }
+
+        // Delete migrated entries
+        context.getContentResolver().delete(FormsProviderAPI.FormsColumns.CONTENT_URI, null, null);


So one quick point:

Errors in this operation can't be rolled back, and also aren't material to the future functionality of the app, since this CR will be ignored moving forward.

I think for those reasons that moving this op outside of the v8->v9 Migrater makes a bit more sense.

Right now, it's:

transaction[ 1) add table 2) move data 3) delete old data ]

and if "delete old data" deletes half of the data before failing, the first two ops will be undone, but the delete old data op won't be, which will put the phone in an irrecoverable state. If instead #3 was treated as a "best effort" op

transaction[ 1) add table 2) move data ] try{ 3) delete old data } catch(){ //oops... do nothing. }

"Half of a failure" in step three doesn't result in an irrecoverable app.

Ahh never realised deletes are not atomic. I think one other way to do this is wrap the delete call in Conent Provider here in a transaction. What do you think ?

I think that would make everything here work reliably, as long as the transaction for this code is halted immediately after the delete succeeds. I'm still a bit hesitant to introduce semantics where the ContentProvider.delete() call can succeed (in whole or in part) while the migrate transaction is still open, just as a matter of most-importance.

Like, if the line of code that says "commit the ContentProvider DB delete transaction" executes and then the phone battery dies, the next time the app spins up, our DB will roll back the migrate transaction and the content provider data will be gone.

If we are super concerned with the ContentProvider delete occurring, I'd rather we separate those transactional semantics from the semantics of getting the data into this db. It's ok for the app to crash if the delete is failing (if that's a real bug that we should be catching and fixing), but I want to separate that condition from this transaction, since there's no way to use SQL transactions in a way which will Atomically link loading the new DB with the ContentProvider data, and deleting the data in the content provider DB. We'd need that to come from a separate source.

Hmm I see the concerns. Will make the changes accordingly.

ctsims · 2018-04-09T22:17:13Z

app/src/org/commcare/models/database/user/UserDbUpgradeUtils.java

+            // Merge Record's instance
+            String instanceUri = oldRecord.getInstanceURIString();
+            Cursor cursor = c.getContentResolver().query(Uri.parse(instanceUri), null, null, null, null);
+            if (cursor != null && cursor.getCount() > 0) {


quick note that technically cursor operations should be wrapped in try{}closes

Not sure what will that achieve us here since 1. This whole function is wrapped in try{} 2. We want to abort the whole thing if an error occurs in cursor operations.

We don't need to catch any exceptions, we just need a try{}finally{} block where the cursor gets closed.

ctsims · 2018-04-09T22:18:30Z

app/src/org/commcare/models/database/user/UserDbUpgradeUtils.java

+        }
+
+        // delete all instance provider entries
+        c.getContentResolver().delete(InstanceProviderAPI.InstanceColumns.CONTENT_URI, null, null);


Same note as in the other migration that I think it's a better practice for us to track "success" for the transaction of this operation before this delete and not after, since it inherently can't be a part of the transaction

ctsims · 2018-04-09T22:22:28Z

app/src/org/commcare/models/legacy/LegacyInstallUtils.java


        //We also need to tell the XForm Provider that any/all of its forms have been moved
-
-        ArrayList<Pair<Uri, String>> toReplace = new ArrayList<>();


Important Note: -This code will no longer work after this change. This legacy installer should continue assuming that a form provider exists (using hard-coded strings or whatever else is retained) or we should kill the legacy updater code.

Yeah. Didn't realize this block of code runs before DB update. We can defnitely keep the Content Provider code if it's worth it. What all versions of CommCare does legacy actually represent ?

Confirmed over slack, but the Legacy db represents CommCare 2.3.0 and below, so we can move forward with the assumption that we are removing legacy migration code.

…of the transaction

…in case of multiple apps installed on same device

amstone326 · 2018-04-12T21:14:19Z

@damagatchi retest this please

…/commcare-android into mergeFormInsanceProviders

shubham1g5 · 2018-04-16T13:58:32Z

@ctsims This should be now ready for another pass.

ctsims · 2018-04-16T17:14:48Z

@shubham1g5 I think this covers everything I was concerned about other than one lingering comment above ("This seems like a bit of an odd time to fail on this to me. Can't we validate that this path isn't empty when it is being set, rather than when it is being used?").

shubham1g5 · 2018-04-17T12:29:57Z

@ctsims I made a mistake in incuding that change as part of a merge commit which I am guessing is why that change is not getting reflected here. But if you go to this commit and search for FormDefRecord.java you will be able to see the required changes for pre-validating filePath.

ctsims · 2018-04-17T13:25:48Z

Great, I'm all set, then. Thanks for going through the 3-4 rounds of random testing for multiple apps, multiple users, etc! Ready to merge once we've cut 2.33

shubham1g5 · 2018-04-20T09:55:50Z

Merging this now in anticipation to find early bugs!

wpride · 2018-04-20T11:46:22Z

🎉 🎉 🎉 Congrats @shubham1g5 ! Looked like a slog

shubham1g5 · 2018-04-20T13:18:57Z

Yeah. I finally understood InstanceRecord before killing it in favour of FormRecord :D

wpride · 2018-04-20T13:23:58Z

To destroy your enemy you must know your enemy

shubham1g5 added 9 commits January 29, 2018 23:13

Migrates are uri based methods for Form and Instance Provider to use …

527add2

…new Form and Instance tables in app DB

Bug fixes

3e714f8

Fixes FormStorageTest

4d57db9

Cleans up old code

a2d191a

First Pass at migration

e257ccf

Migrates FormRecord

e64192f

Scripts migration for XFormAndroidInstaller

504d693

Fixes tests by re initializing Instance Record for breaking tests

fb993af

Cleanup

0cf18bf

shubham1g5 added cross requested product/invisible labels Jan 31, 2018

shubham1g5 self-assigned this Jan 31, 2018

More cleanup for tests

bca947d

shubham1g5 requested review from amstone326, ctsims and wpride January 31, 2018 13:59

ctsims reviewed Feb 22, 2018

View reviewed changes

ctsims requested changes Feb 22, 2018

View reviewed changes

Merge branch 'master' into mergeFormInsanceProviders

8b73bbb

shubham1g5 added 5 commits March 12, 2018 11:28

Merge branch 'master' into mergeFormInstanceRecord

edf5287

Merges InstanceProvider to FormRecord

8087d0a

removes static implementation for formDefReocrd'

5fd6319

'

Correction for FormInstanceXmlParser

57d6e09

Clean up

b829951

corrects comment

3cdc6ae

ctsims requested changes Apr 9, 2018

View reviewed changes

shubham1g5 added 2 commits April 10, 2018 13:04

better errors

c58e89d

Merge branch 'master' into mergeFormInsanceProviders

0bc948f

dimagi deleted a comment from shubham1g5 Apr 10, 2018

shubham1g5 and others added 5 commits April 11, 2018 13:21

Removes unused Encryption code

e6b4a21

Makes deletion from Forms and Instance Provider dependent on success …

bea51d0

…of the transaction

Makes Instance Provider Migration appId dependent to fix db migraion …

3f31ff4

…in case of multiple apps installed on same device

Logging errors happening while delete

5e56ec9

Merge branch 'master' into mergeFormInsanceProviders

0a1f625

amstone326 and others added 5 commits April 13, 2018 15:51

merge master fix conflicts

0f77ff5

Degrade support for upgrade from Legacy Commcare

c83780d

Logs last successful commcare run in prefs

20d9a9d

Merge branch 'mergeFormInsanceProviders' of https://github.com/dimagi…

cf949ca

…/commcare-android into mergeFormInsanceProviders

cleanup

e94237e

Update the deprecation comments to reflect right cc version

302c4ec

ctsims approved these changes Apr 16, 2018

View reviewed changes

amstone326 added this to the commcare_2.44 milestone Apr 18, 2018

shubham1g5 merged commit 13a36fb into master Apr 20, 2018

shubham1g5 deleted the mergeFormInsanceProviders branch April 20, 2018 09:56


		public static final String STORAGE_KEY = "form_def";

		public static final String META_DISPLAY_NAME = "displayName";


		//We also need to tell the XForm Provider that any/all of its forms have been moved

		ArrayList<Pair<Uri, String>> toReplace = new ArrayList<>();

Uh oh!

Merge Form Provider and Insance Provider to App DB #1940

Merge Form Provider and Insance Provider to App DB #1940

Uh oh!

Conversation

shubham1g5 commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctsims commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham1g5 commented Feb 22, 2018

Uh oh!

ctsims commented Feb 22, 2018

Uh oh!

shubham1g5 commented Feb 26, 2018

Uh oh!

ctsims commented Feb 26, 2018

Uh oh!

shubham1g5 commented Feb 27, 2018

Uh oh!

shubham1g5 commented Mar 19, 2018

Uh oh!

ctsims left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

shubham1g5 commented Jan 31, 2018 •

edited

Loading

shubham1g5 Apr 11, 2018 •

edited

Loading

shubham1g5 Mar 15, 2018 •

edited

Loading

shubham1g5 Feb 22, 2018 •

edited

Loading

shubham1g5 Feb 26, 2018 •

edited

Loading

shubham1g5 Apr 10, 2018 •

edited

Loading

shubham1g5 Apr 10, 2018 •

edited

Loading

ctsims commented Apr 17, 2018 •

edited

Loading