Make App and Apk parcelable and fix related installer NPEs
Installations fails currently due to
```
885 ACRA E Caused by: java.lang.NullPointerException
885 ACRA E at org.fdroid.fdroid.installer.InstallManagerService.getAppName(InstallManagerService.java:327)
885 ACRA E at org.fdroid.fdroid.installer.InstallManagerService.createNotificationBuilder(InstallManagerService.java:318)
885 ACRA E at org.fdroid.fdroid.installer.InstallManagerService.onStartCommand(InstallManagerService.java:158)
885 ACRA E at android.app.ActivityThread.handleServiceArgs(ActivityThread.java:2039)
885 ACRA E ... 10 more
```
This bug has been introduced in https://gitlab.com/fdroid/fdroidclient/merge_requests/359 where the packageName has been removed from the toContentValues() method.
The usage of ContentValues to send App/Apk objects to services was an hack in my opinion.
Thus, this PRs introduces proper parceling of App and Apk classes.
@pserwylo @eighthave @mvdan
See merge request !362
The usage of ContentValues to send App/Apk objects
to services was an hack in my opinion.
This hack broke in https://gitlab.com/fdroid/fdroidclient/merge_requests/359
where the packageName has been removed from the
toContentValues() method, which leads to NPEs in
the services.
Remove now unused package name from apk table
The package name is only stored in the `fdroid_app` table now, so we need to remove it form the `fdroid_apk` table. Under normal circumstances, I'd normally just leave unused fields in the DDL (the SQL which defines the tables) and never use it from within the Java code. However in this case, the package name formed part of the primary key of this table. Seeing as we are not inserting into that column any more, it isn't okay to leave it there but instead it must be removed so that we can put a more appropriate primary key on the table. In this case, the new primary key is `appId` + `vercode` + `repoId`.
I think this is the final merge request before I submit a MR with repo priorities.
See merge request !359
Wherever the "package name" of an apk is required, it can be requested by
asking for `Schema.ApkTable.Cols.App.PACKAGE_NAME`. Note the `App` which
indicates that it is in fact pulling this data from the `fdroid_app` table rather
than the `fdroid_apk` table.
Correctly identify the repo for a given URL to fix HTTP Auth.
**NOTE: Based on !355 (If that one gets merged first, I can rebase this, or else we can merge this one for both commits)**
When downloading arbitrary URLs using F-Droid (e.g. icons, .apk files, indexes) then it may be the case that the repo requires authentication. As such, we try to infer the repository based purely on the URL.
The old code took the basename of the URL, which means remove the last fragment (e.g. "index.jar") and use the remaining portion of the URL to lookup the repo.
This is broken for many reasons, partly because of the presence of a query string, partly because there are other things which are not just in the root directory of the repo (e.g. "/icons/*.png").
This new method iteratively peels off the right most segment of the URLs path, then looks to see if a repo exists at that address.
Note that this breaks down if you have nested repositories on a server, where one of the repositories is nested inside a directory that F-Droid knows about, such as "icons". In such a case, the following repositories:
* https://f-droid.org/repo (requires auth)
* https://f-droid.org/repo/icons (doesn't require auth)
will break down. If requesting something from the repo requiring auth:
* https://f-droid.org/repo/icons/org.fdroid.fdroid.png
Then it will lookup the database and find the repo which lives in "/icons" and doesn't require auth (or requires a different auth username/password). Not sure there is a lot that can be done about this without major refactoring. Such refactoring would require making sure a `Repo` is always given to a downloader for any HTTP request, and is probably a bit out of scope of this bug.
Also added tests for this behaviour.
Fixes#711.
See merge request !357
Firstly, this causes #721, possibly due to a bug in "Barcode
Scanner" whereby it seems to ignore the scheme when in caps,
assuming it is "http".
The relevant RFC is:
> RFC3986 (Uniform Resource Identifier (URI): Generic Syntax
In section 3.1, it describes the scheme:
> Although schemes are case-insensitive, the canonical form is
> lowercase and documents that specify schemes must do so with
> lowercase letters. An implementation should accept uppercase
> letters as equivalent to lowercase in scheme names (e.g., allow
> "HTTP" as well as "http") for the sake of robustness but should
> only produce lowercase scheme names for consistency.
Secondly, it is not valid to uppercase URLs at will. Although it
seems that there is some sort of more-compact-QR-generating-logic
that doesn't justify this. Funnily enough, I can't find anything
in RFC3986 about the case-insensitivity of URI paths. However
consider the following:
* https://i.imgur.com/fn33EcW.jpg
That is a valid path to an image. If we upper case it:
* HTTPS://I.IMGUR.COM/FN33ECW.JPG
or lower case it:
* https://i.imgur.com/fn33ecw.jpg
Then the server is entitled to treat it differently and indeed
it does. Both the upper case and lower case are no both 404's.
When downloading arbitrary URLs using F-Droid (e.g. icons, .apk files, indexes)
then it may be the case that the repo requires authentication. As such, we try
to infer the repository based purely on the URL.
The old code took the basename of the URL, which means remove the last fragment
(e.g. "index.jar") and use the remaining portion of the URL to lookup the repo.
This is broken for many reasons, partly because of the presence of a query string,
partly because there are other things which are not just in the root directory
of the repo (e.g. "/icons/*.png").
This new method iteratively peels off the right most segment of the URLs path,
then looks to see if a repo exists at that address.
Note that this breaks down if you have nested repositories on a server, where
one of the repositories is nested inside a directory that F-Droid knows about,
such as "icons". In such a case, the following repositories:
* https://f-droid.org/repo (requires auth)
* https://f-droid.org/repo/icons (doesn't require auth)
will break down. If requesting something from the repo requiring auth:
* https://f-droid.org/repo/icons/org.fdroid.fdroid.png
Then it will lookup the database and find the repo which lives in "/icons"
and doesn't require auth (or requires a different auth username/password).
Not sure there is a lot that can be done about this without major refactoring.
Such refactoring would require making sure a `Repo` is always given to a downloader
for any HTTP request, and is probably a bit out of scope of this bug.
Also added tests for this behaviour.
Fixes#711.
Prevent crash for servers that don't send etags with repo indexes
While working on #711, I noticed this bug using my [Mock HTTP Auth server](https://f-droid.org/wiki/page/PHP_Mock_Repository).
This fix always captures timestamps during repo updates, even if it is the same as last time. This is because we are dependent on it later on in the repo update process. Specifically, when updating from a HTTP server that doesn't send out etags with its responses, it will trigger a full blown repo update every time, even if all the values in the index are the same (name, description, etc). This is as distinct from better behaving servers that send etags, in which case we will only do a partial update (i.e. persist the "last updated time"). In such a case, the remainder of the update process will proceed, and ask for this timestamp.
See merge request !356
Always capture timestamps, even if it is the same. This is because we are dependent
on it later on in the repo update process. Specifically, when updating from a HTTP
server that doesn't send out etags with its responses, it will trigger a full blown
repo update every time, even if all the values in the index are the same (name,
description, etc). This is as distinct from better behaving servers that send etags,
in which case we will only do a partial update (i.e. persist the "last updated time").
In such a case, the remainder of the update process will proceed, and ask for this
timestamp.
Correctly delete single repo, not all repos.
In a recent commit, I cleaned up the code which deletes repo. At that point, instead of maybe concatenating strings together, sometimes with an `AND` statement,
it was changed to use the slightly better `QuerySelection`. This class is preferable because it doesn't need the developer to know whether there was
any previous constraints, and thus it knows whether to prepend an `AND`.
The problem arose because `QuerySelection` is effectively an immutable class. Calling `add()` on it returns a new copy with a different set of constraints.
The code which deleted the repo did not use this copy, and thus the resulting query had zero constraints.
The fix is to use the return value of `add()` correctly. It would've been easier to identify this bug if we had a lint check for "unused return values", though it is likely that that would get annoying very quickly.
Fixes#717.
See merge request !354
In a recent commit, I cleaned up the code which deletes repo. At that point,
instead of maybe concatenating strings together, sometimes with an `AND` statement,
it was changed to use the slightly better `QuerySelection`. This class is
preferable because it doesn't need the developer to know whether there was
any previous constraints, and thus it knows whether to prepend an `AND`.
The problem arose because `QuerySelection` is effectively an immutable class.
Calling `add()` on it returns a new copy with a different set of constraints.
The code which deleted the repo did not use this copy, and thus the resulting
query had zero constraints.
The fix is to use the return value of `add()` correctly. It would've been
easier to identify this bug if we had a lint check for "unused return values",
though it is likely that that would get annoying very quickly.
Fixes#717.
Use an integer primary key to join `fdroid_app` and `fdroid_apk` rather than the apps package name.
**Disclaimer:**
I realise this is a big change, but it needs to be done at some point, and it is not amenable to smaller changes, due to the fact that the app/apk relationship is so ingrained throughout F-Droid. Luckily, we have really quite comprehensive test coverage of the F-Droid `ContentProvider`s which helps to confirm that nothing should be majorly broken here.
**Some points of note:**
This is the first part of implementing #511, whereby the DB is refactored to better support multiple repositories.
Instead of joining `fdroid_app` and `fdroid_apk` tables using the package name, join based on an integer id autogenerated by sqlite. By default sqlite calls this `rowid` and it exists for every table, unless you've specified your own `NUMBER AUTO INCREMENT PRIMARY KEY` field. We have not done this for `fdroid_app`, so `rowid` is indeed the key we use in this MR. The package name was previously `id` in both the app and apk tables. Now `fdroid_app` makes use of `rowid` and `fdroid_apk` has a foreign key called `appId`.
The `ApkProvider` used to get away with only really querying the `fdroid_apk` table, and thus it didn't have to prefix any of the field names in the query with the table name. However now it always joins onto the `fdroid_app` table also, and as such, there are many places where field names needed to be prefixed with the table name (e.g. the `apk` alias or the `app` alias) to ensure the SQL is unambiguous when fields with the same name exist in both tables. The catch is, we want to reuse helper functions that build fragments of SQL, such as "Query based on package name". These helper functions are used both when updating and deleting apks (where field table prefixes are not allowed) and also in select statements (where they are required). Thus this changes comes with an `includeTableAlias` argument added to many of these methods (e.g. `ApkProvider.queryApp`).
There is still a package name column in the `fdroid_apk` table (the `id` field). This will be removed in future MRs and replaced with the package name from the joined `fdroid_app` table.
The `RepoPersister` used to dump apps in the db, then dump apks into the db. Now it needs to be a bit more nuanced, and dump apps into the db, _then ask the db what `rowid` was assigned to the apps_. This is then used when dumping the apks into the db. This also required some changes to how the `TempAppProvider` and `AppProvider` interact. In the interests of reusing code, both of these are able to provide operations on a similarly structured table but one is an in memory table (`temp_fdroid_app`) and the other is on disk (`fdroid_app`). In the past this was simpler, because the only interaction with the `TempAppProvider` was by using lists of `ContentOperation`s. Whereas now that we need to ask more substantial questions of the `TempAppProvider` other than "Insert this thing" or "update that thing", we needed to implement the `query` method in `TempAppProvider` similar to how it is in the base class `AppProvider`. As such, the common code for the base class and subclass `query` methods was extracted into `AppProvider.runQuery()`.
I tried to minimize the changes to the test suite as much as possible, so that it is possible to verify that they pass under the same conditions as before this change. However some changes were required to support the notion that apks depend on an app and its rowid, whereas this was not the case before. Thus there is some more boilerplate in the tests to ensure that inserting an apk ensures an app entry is present in the db too.
See merge request !345
Right now there is only one test in there anyway, so hopefully this is
a good tradeoff in terms of our time wasted vs not being able to run
those tests.
Moved index adding to a helper function, so that the same mistake isn't made
again. That is, indexes should be the same whether upgrading or creating a
database. Thus, the code to add indexes should always be the same regardless
of the reason for an index being added. The `IF NOT EXISTS` syntax helps
to allow the same queries to add during creatin and migration of database.
The fact that sqlite chose to do a "FULL TABLE SCAN" right off the bat when showing a list
of apps results in it having to do extra work at the end of the query to sort. If the scan
can be utilise an index, then the sorting is already done and a b-tree need not be constructed.
Thus, this introduces indexes for both the `name` column (used to sort most lists of apps)
and the `added` column (used to figure out the `Whats New` apps).
This is required because SQLite "rowids" are 64 bit integers:
> all rows within SQLite tables have a 64-bit signed integer
> key that uniquely identifies the row within its table."
https://sqlite.org/lang_createtable.html#rowid
This is important for the ability to refactor the database for better performance in the future.
See #511 for details.
For those interested in the details, here is a query plan of selecting the "All" category of apps before
this commit:
* `SCAN TABLE fdroid_app USING INDEX app_id`
* `SEARCH TABLE fdroid_apk USING INDEX apk_id (id=?)`
* `SEARCH TABLE fdroid_repo USING INTEGER PRIMARY KEY (rowid=?)`
* `SEARCH TABLE fdroid_installedApp AS installed USING INDEX sqlite_autoindex_fdroid_installedApp_1 (appId=?)`
* `SEARCH TABLE fdroid_apk AS suggestedApk USING INDEX sqlite_autoindex_fdroid_apk_1 (id=? AND vercode=?)`
* `USE TEMP B-TREE FOR ORDER BY`
And here is a query plan of afterwards:
* `SCAN TABLE fdroid_app`
* `SEARCH TABLE fdroid_apk USING INDEX apk_appId (appId=?)`
* `SEARCH TABLE fdroid_repo USING INTEGER PRIMARY KEY (rowid=?)`
* `SEARCH TABLE fdroid_installedApp AS installed USING INDEX sqlite_autoindex_fdroid_installedApp_1 (appId=?)`
* `SEARCH TABLE fdroid_apk AS suggestedApk USING INDEX apk_appId (appId=?)`
* `USE TEMP B-TREE FOR ORDER BY`
The things of note are:
* `SCAN TABLE` doesn't use an index, which means that it is really using the rowid index. Shouldn't behave much differently.
* The second item now uses an integer primary key index rather than a package name index. Should increase search speed marginally which was the goal of this commit. As more apks exist, the speed improvement will also increase.
Removed `TargetApi` annotation from class, pushing it to methods, and fix API problems.
Fixes#708.
On gingerbread (in my case, 2.3.4) F-Droid will reliably crash whenever it views `AppDetails` with the following error:
```
E java.lang.NoSuchMethodError: android.content.Context.getDrawable
E at org.fdroid.fdroid.privileged.views.AppSecurityPermissions$MyPermissionGroupInfo.loadGroupIcon(AppSecurityPermissions.java:111)
E at org.fdroid.fdroid.privileged.views.AppSecurityPermissions$PermissionItemView.setPermission(AppSecurityPermissions.java:158)
E at org.fdroid.fdroid.privileged.views.AppSecurityPermissions.getPermissionItemView(AppSecurityPermissions.java:396)
E at org.fdroid.fdroid.privileged.views.AppSecurityPermissions.displayPermissions(AppSecurityPermissions.java:370)
E at org.fdroid.fdroid.privileged.views.AppSecurityPermissions.getPermissionsView(AppSecurityPermissions.java:348)
E at org.fdroid.fdroid.AppDetails$AppDetailsSummaryFragment.buildPermissionInfo(AppDetails.java:1381)
E at org.fdroid.fdroid.AppDetails$AppDetailsSummaryFragment.setupView(AppDetails.java:1348)
E at org.fdroid.fdroid.AppDetails$AppDetailsSummaryFragment.onCreateView(AppDetails.java:1095)
E at android.support.v4.app.Fragment.performCreateView(Fragment.java:2074)
E at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1104)
E at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1286)
E at android.support.v4.app.BackStackRecord.run(BackStackRecord.java:758)
E at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:1671)
E at android.support.v4.app.Fragment.performStart(Fragment.java:2096)
E at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1142)
E at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1286)
E at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1268)
E at android.support.v4.app.FragmentManagerImpl.dispatchStart(FragmentManager.java:2148)
E at android.support.v4.app.FragmentController.dispatchStart(FragmentController.java:212)
E at android.support.v4.app.FragmentActivity.onStart(FragmentActivity.java:625)
E at org.fdroid.fdroid.AppDetails.onStart(AppDetails.java:424)
E at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1129)
E at android.app.Activity.performStart(Activity.java:3791)
E at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:1620)
E at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:1663)
E at android.app.ActivityThread.access$1500(ActivityThread.java:117)
E at android.app.ActivityThread$H.handleMessage(ActivityThread.java:931)
E at android.os.Handler.dispatchMessage(Handler.java:99)
E at android.os.Looper.loop(Looper.java:130)
E at android.app.ActivityThread.main(ActivityThread.java:3683)
E at java.lang.reflect.Method.invokeNative(Native Method)
E at java.lang.reflect.Method.invoke(Method.java:507)
E at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:864)
E at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:622)
E at dalvik.system.NativeStart.main(Native Method)
```
This is because `AppSecurityPermissions` uses a method that was not added until a later API. This class is used by `AppDetails` without an API verison check around its access. Thus, we call methods on this that are unsupported when on gingerbread.
By removing `TargetApi` from the class, it unearthed a couple of locations where methods were being invoked without first guarding against the build version correctly. These two locations have been fixed and a `TargetApi` attached to the more narrowly defined method which requires it.
See merge request !353
This class is used by `AppDetails` without an API verison check around its access.
Thus, we call methods on this that are unsupported when on gingerbread.
By removing `TargetApi` from the class, it unearthed a couple of locations
where methods were being invoked without first guarding against the build version
correctly. These two locations have been fixed and a `TargetApi` attached to the
more narrowly defined method which requires it.
