Prepare dbs for upgrading

[grzesiek2010]

What has been done to verify that this works as intended?

I added automated tests and tested downgrading manually.

Why is this the best possible solution? Were any other approaches considered?

We agreed in #3444 that in order to take care of our users and backwards compatibility we should release a version with onDowngrade() methods that would be able to handle path changes first, then we can migrate them.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

In order to test this pr please:

1. Install the app with these changes
2. Open and save the attached form.
3. Copy instances.db and itemsets.db files from odk/metadata to your desktop
4. Open them using a browser like https://sqlitebrowser.org/
5. Bump database versions and change paths to start with ../ instead of /storage/emulated/0/odk/
6. Paste the edited databases to odk/metadata
7. Test the saved forms.

That would be enough when it comes to testing.

Do we need any specific form for testing your changes? If so, please attach one.

Please test a form with external itemsets like:
select_one_external.zip

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[lognaturel]

I didn't answer the question at #3444 (comment) about what the relative path prefixes should be because I wanted to think about it but I'm still really not sure.

This builds paths that start with ../ which makes sense given that the database files themselves are located at <odk root>/metadata/. That seems like a reasonable approach but I'm not feeling very confident about it. There's something that makes me uncomfortable about having a ../ in there but I can't put my finger on it.

My initial thought was to only store the unique part of the path so something like All widgets_2019-07-29_18-19-08/All widgets_2019-07-29_18-19-08.xml and let Collect (or whatever downstream app is using the db if someone copies it off the sdcard) prefix it as it wants.

@seadowg, any thoughts on what the paths should be relative to? Any best practices we should know of?

[lognaturel]

Wait a minute. Does this make sense? The end goal is to have files that won't be rooted at /sdcard/ anymore so they'll still be inaccessible even when going through this down migration, no? Is there any benefit to switching to relative paths before switching the root? This is hard to reason about. It would help to have an exhaustive list of the changes that need to be made, the order that they need to be in and what happens when upgrading and downgrading between versions.

[grzesiek2010]

Wait a minute. Does this make sense? The end goal is to have files that won't be rooted at /sdcard/ anymore so they'll still be inaccessible even when going through this down migration, no?

Yes but we wanted to migrate paths first and then in the future migrate files. Let's assume that we release:

• 1.25 with this pr
• 1.26 which would use only relative paths
• 1.27 which would use scoped storage (files won't be stored on sdcard)

downgrading from 1.26->1.25 will work thanks to this pr
downgrading from 1.27->1.26 - only new files added in 1.27 won't be available but we don't have to remove sdcard/odk dir after migrating file we can leave it so if someone install 1.27 and run into a big bug then they will be possible to downgrade and use files added by previous versions.
We can also prepare a doc how to copy newly added forms from the internal storage to sdcard in case of downgrading.

[seadowg]

@lognaturel I haven't been as involved in this stuff so potentially lacking context but yeah it doesn't feel right to have paths like ../ in the database. For me, Collect should mostly be in control of constructing the path to a file.

An example would be like storing an image in S3 in a web app. If I just store the filename the code gets to dictate where that file lives. That feels easier to change as I don't need to migrate a bunch of data in the database if I change services or structure.

[grzesiek2010]

@lognaturel

My idea I tried to explain during the meeting is to copy files to sd card in addition to what I've already done in this pr. So onDowngrade() would do:

1. Copy files from odk directory on internal storage to sd card
2. Convert relative paths to absolute paths (what this pr does)

The full scenario would be:

1. We release v1.25 with this pr + changes to copy files to sd card back
2. We release v1.26 with scoped storage (files from sd card are moved to internal storage), the directory on sd card is cleared.

downgrade v1.26->v1.25 the app will copy files from internal storage to sd card back and convert database paths.

Does that sound right?

[grzesiek2010]

@lognaturel @seadowg
can we make a final decision what to do with it?
We have to start using scope storage, that's for sure.
We need to migrate paths we store in databases, that's for sure as well.

The decision we need to make is whether we need to provide backward compatibility or it's not thta important.
My first opinion was yeah it would be good to do that but we would need to implement onDowngrade() methods to change paths again during downgrading (what I did in this pr) and also try to copy files from our app dir to sd card.
It will never be a perfect solution because we can do that now and ensure downgrading to v1.26 will work but we can't fix previous versions.
So the question is whether it's worth the effort at all.
I know we have a lot of time till August (when bumping targetSdkVersion will be required) but time flies it's been almost a month since I created this pr and we haven't made any decision.

[seadowg]

Ach sorry @grzesiek2010 this totally skipped my mind. So by dropping "backwards compatibility" you mean that someone wouldn't be able to downgrade a device from 1.26 to 1.25? I might have this wrong in my head.

[lognaturel]

The question for me has been more about when we put in the down migration. That is, do we put it in one version before the scoped storage change just to have a path out or do we try to get it in earlier (e.g. v1.26) so that users don't have to think about what version to downgrade to.

But now that I think about this again, another issue came to mind. Along with the scoped storage change, all data will be deleted on app uninstall. Downgrading will require manually installing an APK. Does this APK install lead to the previously-installed (newer) version being uninstalled and thus its data being deleted? If so, there's no point in having a down migration.

Another option we do have is documenting how to move the /sdcard/odk/ folder to the new location. If folks have to install an APK anyway to downgrade, they'd likely be able to do this.

[grzesiek2010]

Downgrading will require manually installing an APK. Does this APK install lead to the previously-installed (newer) version being uninstalled and thus its data being deleted?

You are right I forgot about it because on a daily basis I work with Android Studio and using it I can downgrade without any problems but yeahh if it's an apk file you need to uninstall the first version.

Ok we have one answer we don't need to handle copying files onDowngrad() (from the app dir on internal storage to sd card).

So by dropping "backwards compatibility" you mean that someone wouldn't be able to downgrade a device from 1.26 to 1.25?

downgrading will be possible but files won't be available because as @lognaturel pointed out they will be deleted (because you need to uninstall the existed version first) and new version will use relative paths so if you downgrade you will en up wit a database files with relative paths and old code not able to use them.Of course we can handle the second case (what I did in this pr) but taking into account we can't copy files back to sd card it doesn't make much sense.

[lognaturel]

if it's an apk file you need to uninstall the first version.

It is in fact possible to install without removing the files by doing e.g. adb install -r ODK-Collect-v1.23.3.apk. See http://adbcommand.com/adbshell/pm for documentation.

But someone would probably need documentation to find that out and the documentation we provide might as well tell them to copy files for safekeeping and then put them back in /sdcard if they need to downgrade.

I'm starting to feel like this almost warrants a major version bump to indicate that there isn't really any going back without files being cleared. I hope not too many users routinely downgrade production devices.

As far as this PR goes, I now think we do nothing with the files or database until we absolutely have to make the switch to scoped storage. Does that sound right?

[grzesiek2010]

I now think we do nothing with the files or database until we absolutely have to make the switch to scoped storage. Does that sound right?

it sound right if we don't want to provide any facilities with downgrading (downgrading databases, copying files back to sd card), because otherwise we need to do that earlier (in previous versions).
Now I'm more in favor of just migrating database paths and that's all.

[seadowg]

@lognaturel agreed with the major version bump. It does feel like it's good to create a little drama so people know to the files are now tied to their app install rather than just sitting on device.

[grzesiek2010]

Ok so we can close this pr and get #3444 ready (because we can use relative paths even now).
Then we need to warn our users about upcoming changes and switch to scope storage. Am I right?

[lognaturel]

because we can use relative paths even now

You mean updating the code to handle either relative or absolute paths in the db for now but not actually changing the database, right? That'd basically be #3444 without the db changes. We have determined that the db changes won't help because the db files will be removed on uninstall anyway once we move to the new storage location. Yes, as far as I can tell, that sounds right. Hopefully we've thought of most things now. 😫

[grzesiek2010]

You mean updating the code to handle either relative or absolute paths in the db for now but not actually changing the database, right?

I'm not sure now, I thought you prefer having the code that deals just with relative paths and updating existing paths in databases, to avoid complex code with if/else statements (checking if it's a relative or absolute path). Did you change your mind?

[lognaturel]

I see this. I think need to do the full analysis with all the information we now have so that hopefully we won't need more back and forth. Hoping I can do that next week.