Merge download broadcast receivers
Previously, for all 4 states broadcast receivers were registered separately. These have now been merged into one receiver. IMHO this makes the code more readable and structured.
See merge request !368
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.
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.
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.
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.
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.
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.
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.
SQLite has a very nice "EXPLAIN QUERY PLAN" command (https://sqlite.org/eqp.html).
It is not really meant to be used in production code, as per the docs, but it is
super helpful at diagnosing missing indexes or other performance problems with
databases. I find it much better than, for example, the MySQL alternative.
This commit routes queries from the `ApkProvider` and `AppProvider` through a
`LoggingQuery` which (in debug builds) times queries, and if they take longer
than a certain threshold, outputs them to logcat. In addition, it will then
ask sqlite to explain its query plan for that same query, and output that to
logcat too.
Right now, table names are in `DBHelper.TABLE_*` constants, and each tables fields are
in `*Provider.DataColumns.*` constants. This brings them all into a predictable location.
In addition, it makes it easier to statically import `Schema` so that instead of, e.g.,
* `AppProvider.DataColumns.PACKAGE_NAME`
We can choose one of the following, based on our current context:
* `Schema.AppTable.Cols.PACKAGE_NAME`
* `AppTable.Cols.PACKAGE_NAME`
* `Cols.PACKAGE_NAME`
In the worst case, it isa couple of chars shorter than now. In the best case, if we are
writing a class that primarily deals with Apps (e.g. App.java or AppProvider.java) then
we get a big win with just `Cols.PACKAGE_NAME`.
Having these things slightly shorter may seem like it is pointless, but the length of
each constant probably contributed to my lack of willingness to use constants instead
of string literals when constructing queries.
In the future, this should be moved towards something more akin to:
> http://openhms.sourceforge.net/sqlbuilder/
and I hope that extracting all the schema stuff into one interface may help that.
