Use relative paths to the storage

[grzesiek2010]

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

• reviewed the implemented changes
• ran tests on Firebase test lab
• performed manual test.

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

We will have to upgrade targetSdkVersion to 29 next year that means we will need to use scoped storage so that we won't be able to use sd card like we do now to store our files.
This pr is a first step towards that goal. I refactored the code in order to refer to sd card just in one place so that we will be able to replace that code easily and use our app directory in the feature.

Now Environment.getExternalStorageDirectory() is used just in one place (to be honest there is one method which uses it as well but it will be removed/replaced anyway) and we will be able to replace it with Context#getExternalFilesDir() easily.

@lognaturel we were talking about paths in databases but in fact I can't see anything that should be refactored there. Please let me know if I missed something.

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?

The changes are related to everything so it requires to be tested carefully. It's even difficult to tell what exactly should be tested because the risk is everywhere but the most important thing is to test databases:

• Forms db
• Instances db
• Itemsets db

Upgrading and downgrading is also very important here!

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

No.

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]

we were talking about paths in databases but in fact I can't see anything that should be refactored there

In the forms database, formMediaPath, formFilePath and jrcacheFilePath are absolute paths, right? For example, I see /storage/emulated/0/odk/forms/simple-media. This should be migrated to forms/simple-media. I think the migration should remove ODK_ROOT from the path prefix and detect if somehow it's not part of the path. I suppose it would be possible for an external application or the user to write other absolute paths into the DB. I think we would probably want to let the user know if any of those were detected that they won't work in future versions because of Android changes. Does that sound right?

[grzesiek2010]

Ahhh I completly forgot that we use paths there and was thinking about creating databases only.... ok sorry I'll fix it.

[grzesiek2010]

Hey, I migrated paths as we discussed but it's been bothering me and I wanted to talk with you.
The first problem is that changes in paths touch almost everything so it's a bit risky and would be good to test and merge it ASAP
The second problem (more important) is that we will save relative paths in databases, the code I implemented deals with it but if someone uses databases filled using that code (with relative paths) but with Collect v1.24 or lower it won't work, it's a case when for example we release v1.25 with my changes, users start using it but after some time it turns out that there is a big bug (somewhere else) and they will need to downgrade.
Another problem is that as long as we use sd card our forks use the same databases and then they won't be able to use forms saved by the original app - on the one hand maybe it's ok because in this way we could prepare users for scoped storage (they won't be able to use forks and the original app using the same files like now)
A solution for downgrading would be to release at least one version that just deals with both types of paths (full and relative) but saves full paths like now, and then in the future release another one that save relative paths to databases. Thanks to that even if downgrading is required it won't be a problem.
Do you think it make sense to be that careful?

[lognaturel]

changes in paths touch almost everything so it's a bit risky

Agreed. We also have the option to hold this for the next release too since it's not urgent. I'd rather get it in now if possible but depending on the answers below we may consider waiting.

and they will need to downgrade. [...] A solution for downgrading would be to release at least one version that just deals with both types of paths (full and relative) but saves full paths like now, and then in the future release another one that save relative paths to databases.

It seems to me that down migrations should be able to deal with the need to downgrade the app. We're not actually moving files with this step so it should be possible to add the absolute path prefix back again to the database contents on downgrade. Right?

as long as we use sd card our forks use the same databases and then they won't be able to use forms saved by the original app

This seems like it will be true when we actually move the files but not with this step. My sense is that we should delay actually moving the files until we have to. That way we can make sure to have communicated with users and received feedback about the impact. But I think that when it comes to forks this will largely be a positive change. I expect that the biggest disruption will be for folks who have companion apps and make assumptions about the locations of Collect files. They'll need to update their apps to go through the providers.

[grzesiek2010]

It seems to me that down migrations should be able to deal with the need to downgrade the app. We're not actually moving files with this step so it should be possible to add the absolute path prefix back again to the database contents on downgrade. Right?

I'm not removing absolute path prefixes in my approach, instead I add new rows with relative paths from now and make the code able to deal with both (new relative paths and those full saved before).
So my approach is a bit different than you thought I guess, but it doesn't matter because in both cases we would need to release at least one version which adds prefixes back or is able to handle both types of them.
If we release v1.25 with this pr you won't be able to downgrade to v1.24 because 1.24 doesn't contain a code to deal with such a downgrade. Does that seem right?

This seems like it will be true when we actually move the files but not with this step. My sense is that we should delay actually moving the files until we have to. That way we can make sure to have communicated with users and received feedback about the impact.

Yes I see, I'm just talking about shared db files. If someone has Collect v1.25 (let's assume with this pr) the saved paths will be relative what makes such rows unavailable from a fork which is based on an older version of collect.

[lognaturel]

If we release v1.25 with this pr you won't be able to downgrade to v1.24 because 1.24 doesn't contain a code to deal with such a downgrade.

Ah, of course. For some reason my brain has trouble dealing with the idea that the down migration code is in the old version, not in the new one. So we could introduce a down migration in v1.25 but not actually change paths yet. Then we could change paths and increment the db version in v1.26 or later. That way it would always be possible to downgrade to any version going through a downgrade to v1.25. I'm thinking that for that downgrade, instead of using a db version, we could use a separate USES_RELATIVE_PATHS boolean flag. That way we don't need to know exactly when we'll make the final switch and we can delay it until it's necessary.

I add new rows with relative paths from now

This essentially splits the migration to relative paths in two parts, right? The one-time migration would involve creating a new column, copying in all the relative paths, deleting the original column and renaming the new column. You're suggesting doing half of that work now and the other half later so that the v1.25 db will have redundant columns but will always work with older versions, right? That seems like a reasonable approach to me as long as you also have the down migrations for each db. Clever.

If someone has Collect v1.25 (let's assume with this pr) the saved paths will be relative what makes such rows unavailable from a fork which is based on an older version of collect.

Ah yes, I get you. Collect will write relative paths which will make the forms "invisible" to the fork. And the fork will write absolute paths which means the db will be in a weird hybrid state but your code will deal with both. It's annoying but your concept with the two columns mitigates that. Then when we do the full switch of the column we can start by recomputing the relative column.

When you get a chance, can you please update the PR description with the intended behavior in various interesting cases? There are lots of great details you've thought about that are not immediately obvious.

I haven't looked at the code in detail yet but I noticed that you have relative paths that start with /. If they're relative, they should not start with /.

[grzesiek2010]

You are talking about upgrading/downgrading databases and even adding a new column but that's not how I implemented it.
In instances.db and itemsets.db we have just one column which contains paths but in forms.db we have three it doesn't make sense to double them.
My solution is simple: once we merge it we will save relative paths to the same columns it doesn't matter whether they already contain full paths.
I added a code which deals with it, it writes relative paths only, but while reading it just checks it. If We need a full path because we need to read a file it adds the prefix if it's a relative path and does nothing otherwise (if it's a full path saved before we added those changes).

Sorry I should have described it above (will do).

I haven't looked at the code in detail yet but I noticed that you have relative paths that start with /. If they're relative, they should not start with /.

Now we use paths like:
/storage/emulated/0/odk/forms/All widgets-media
and my code saves path like:
/forms/All widgets-media
then adding the storage prefix if needed, is that bad?

[lognaturel]

In instances.db and itemsets.db we have just one column which contains paths but in forms.db we have three it doesn't make sense to double them.

The existing down migration for forms.db will just work, right? The contents of the forms database is always recoverable (minus the creation date but that's not a very big deal) so the down migration drops the table entirely and rebuilds it. So it's safe to make some or all paths in that db relative.

once we merge it we will save relative paths to the same columns it doesn't matter whether they already contain full paths.

Thanks for walking me through all of it. I guess are you thinking that we don't really need a migration to make all paths relative because we can continue to look for Environment.getExternalStorageDirectory() and strip it off forever? It feels odd to me to have to do this extra check and to have the database include a mix of relative and absolute paths. Since this creates a problem with downgrading anyway, it feels cleaner to me to have a single migration to relative paths and not deal with absolute paths ever again.

What do you think about, for this release, not making any changes to the actual paths that are written and only including new down migrations for the dbs with content that can't be thrown away? I'm imagining it doing something like check the first path in the column to see if it's absolute. If it's absolute, do nothing. If it's relative, copy path columns over, make all paths absolute, copy the column back to the original name. I don't actually think that needs to be wrapped in any conditional. It would ensure that all versions between 1.25 and whenever we actually migrate to relative paths can be downgraded to.

my code saves path like: /forms/All widgets-media

Given that the docs for isAbsolute() say "On Android, absolute paths start with the character '/'," I think relative paths in the DB should not start with '/'.

There's a lot going on here so if my proposal doesn't make sense or there are still subtleties to discuss, we should try to sync up on a quick call your Monday afternoon.

[grzesiek2010]

Ok I think it's a reasonable solution. We can implement onDowngarde() methods for instances.db and itemsets.db and leave forms.db as is since it can be just recreated.

when it comes to relative paths should I use ../forms/All widgets-media?

[grzesiek2010]

Ready #3460

[lognaturel]

Thanks. And so this will go back down to just the first commit and the rest will be saved for a future version, right?

[lognaturel]

Based on this discussion and exploration we have refined the strategy and it is now described in #3360.