During development of a new feature, I noticed a bug occuring only after using
the new feature for several days. This was because the metadata only infrequently
changes in ways which cause certain code paths to be hit. By having the f-droid.org
metadata from several days apart in the test suite, it allows for testing more
of these cases. In the future, even later versions of the metadata can be added
to ensure that we can update happily from old to new metadata.
This ensures that all of the relevant joins are in place, so that when
the updater asks to `queryPackageName()` then it can assume that we have
already joined onto the `Schema.PackageTable`.
Renamed the `Schema.AppMetadata.Categories.CATEGORIES` constant into the
`Schema.AppMetadata.ForWriting.Categories.CATEGORIES` constant. This is
to make it very clear that it is not for reading from the database.
When updating existing apps or inserting new apps, instead of splatting
a comma separated list into a single sqlite3 column, we now put it into
several rows in the `CatJoinTable`. This is done after deleting existing
categories, to make sure that if the app has been removed from a category,
then this is reflected during the update.
It was hidden some time ago, and nobody seems to miss it.
Also, we will be redoing this view soon anyway. In the meantime,
this category stuff is changing and this view should be removed.
Be a little more concise about when we need to run migration for v64.
Before it is not apparant that the migration introduced for v64 is associated with that particular version. This is because the guard condition used to bail out from the upgrade is more closely related to a previous migration.
This is due to a flaw with the desigh of `resetTransient()`, whereby it always resets the database to the schema _of the current F-Droid version being run_, not of the tables as they stood at the time of the particular migration being introduced.
This clarifies the guard condition for v64 by allowing it to query whether the schema has been created fresh or not during this particular invocation of `onUpgrade()`
**Note:** this is less to do with the v64 migration, but rather I came across the exact same issue while working on category table related changes. At that point I realised I made a slight mistake with the `resetTransient()` method.
**Also:** I'm happy to entertain other designs if anybody is that interested. One other approach is to change the guard condition to:
```
if (version >= 64 || fieldExists(db, ApkTable.NAME, "...")) {
... add field ....
}
```
However that suffers a little bit when the migration is a little more complex and checking if a field exists may not be enough.
See merge request !408
For some reason, the existing approach of "select * and then see if the
column of interest is in the results set" didn't work as expected under
tests. Perhaps SQLite is caching the list of columns for the purpose of
`select *` even after running an `alter table add column` query?
Either way, I couldn't figure out why it wasn't working as expected.
This left us with two options:
* Try to `select columnToCheck` and see if it throws an exception
* Query columns using `PRAGMA table_info.
The exception thrown when a column doesn't exist is not specific enough
for our code to check that this is the exact exception that occured. It
is not possible to say: `try { ... } catch (SQLiteColumnNotFound e) { ...}`
unfotunately. Also, there is a cost associated with unwinding the stack
to process an exception, which means exceptions probably shouldn't be
used in unexceptional circumstances such as this.
This change instead uses `PRAGMA table_info(tableOfInterest)` and then
iterates over the cursor looking for the relevant column. Even if the
performance is worse than the stack unwinding of an exception, it is
more concise and less hacky.
The fact there are arbitrary migrations at the top of the file (between
`onCreate()` and `onUpdate()` makes it harder to scan this file.
This changeset moves these methods verbatim, without changing any of
the method bodies or signatures.
Before it is not apparant that the migration introduced for v64 is
associated with that particular version. This is because the guard
condition used to bail out from the upgrade is more closely related
to a previous migration.
This is due to a flaw with the desigh of `resetTransient()`, whereby
it always resets the database to the schema _of the current F-Droid
version being run_, not of the tables as they stood at the time of
the particular migration being introduced.
This clarifies the guard condition for v64 by instead explicitly asking
if the columns of interest exist yet in this particular invocation of
`onUpgrade()`.
Fix npe verifying perms
Fixed a NPE for apps with no permissions.
Here is an example of the logging output for a couple of apps too after my change:
```
D/ApkVerifier(10929): Checking permissions
D/ApkVerifier(10929): Actual:
D/ApkVerifier(10929): None
D/ApkVerifier(10929): Expected:
D/ApkVerifier(10929): None
```
and
```
D/ApkVerifier(10929): Checking permissions
D/ApkVerifier(10929): Actual:
D/ApkVerifier(10929): android.permission.READ_EXTERNAL_STORAGE
D/ApkVerifier(10929): android.permission.WRITE_EXTERNAL_STORAGE
D/ApkVerifier(10929): android.permission.SET_WALLPAPER
D/ApkVerifier(10929): android.permission.SET_WALLPAPER_HINTS
D/ApkVerifier(10929): android.permission.WRITE_SETTINGS
D/ApkVerifier(10929): Expected:
D/ApkVerifier(10929): android.permission.SET_WALLPAPER
D/ApkVerifier(10929): android.permission.READ_EXTERNAL_STORAGE
D/ApkVerifier(10929): android.permission.SET_WALLPAPER_HINTS
D/ApkVerifier(10929): android.permission.WRITE_SETTINGS
D/ApkVerifier(10929): android.permission.WRITE_EXTERNAL_STORAGE
```
See merge request !407
This wouldn'tve actually found the problem in the previous commit,
due to the null happening before checking permissions while logging perms.
However, still seems like a nice test to have so that the method itself
handles nulls correctly.
Improved category tests
In preparation for implementing the [new category UI](https://gitlab.com/fdroid/fdroidclient/uploads/01d865e65604c41b0a472f0d39e7f1a7/Categories.png) I will be refactoring the database so that categories get their own table. In preparation for that, this MR improves the categories tests so that they also test the ability to query apps based on their categories.
See merge request !406
The previous category tests only checked that certain categories
would indeed find their way into the database if certain app metadata
is saved. It didn't check the other direction, using these categories
in queries.
Support extended <uses-permissions/> tags
`<uses-permission-sdk-23>` is a new tag for requesting permissions on _android-23_ and above. `<uses-permission/>` and `<uses-permission-sdk-23>` both can have optional _maxSdkVersion_ values, and these need to be fully supported in order for the Privileged Extension to be able to check the APK permissions against the index.xml permissions. This is needed so that it can prompt the user once, then download and install the APK after that. Its also needed for transparent background updates to check if the permissions have changed in the update.
This gets closer to the complete support, really the full permissions should be stored in the database. Then the only
conversion would happen ewhen parsing the XML. Right now, it still stores the old F-Droid permissions names, i.e. without
`android.permission.`. Then those are converted when they are loaded from the DB into an Apk instance.
See merge request !402
<uses-permissions/> tags can have min and max SDK to take effect. This is
not supported currently, and it necessary especially with the privileged
installer so it can properly represent the permissions that an APK is
requesting.
For example:
<uses-permission
android:name="android.permission.MANAGE_ACCOUNTS"
android:maxSdkVersion="22" />
<uses-permission-sdk-23
android:name="android.permission.CAMERA" />
<uses-permission-sdk-23
android:name="android.permission.CALL_PHONE"
android:maxSdkVersion="23" />
android.content.pm.PackageInfo is the Android class for representing data
about an APK/package. Since Apk.permission is the same thing, we should
use the same name.
Android won't protect us from other apps sending other Intents to these
receivers, so at least check that the action string matches what its
looking for. This is based on a lint recommendation.
Test all database migrations from db-version/42 onwards
While a bit arbitrary choosing 42, it is from more than two years ago. We can use this as our new baseline, in that this test will always check upgrades from 42 onwards, with each database bump.
Once the test was up and running, it immediately identified a bug in the database migration for v50 and v57. These have been fixed in this branch too.
Fixes#778.
See merge request !403
The migration resulted in a query being run which was broken. The query
was broken because it was dynamically generated by Java code. This Java
code resulted in a valid migration when until very recently when the
query was refactored to deal with a new DB structure. Now the query is
no longer suitable to be run against a DB_VERSION 49 database.
To resolve this, the migration now hard codes the query to a string
which is executable when the DB_VERSION is 49.
It was a little arbitrary to choose this date. However it was when the database
looked quite close to what it looks like now and it is from well over two years
ago. Going into the future, this test may as well always start out at 42 forever
more to ensure that database migrations from that point continue to work for
all future database migrations.
For upgrades from DB version earlier than 63, the whole table is recreated
by resetTransient() in migrateToPackageTable() so the upgrade method for
the OBB tables only needs to run when the database is at exactly version 63
This was mistakenly added to cd9582c9902dd4ac9218acfd69872f3eebcd3d93 when
it was rebased on !375.
