Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Overhaul of DiagnosisKey related Download and Caching Mechanisms (EXPOSUREAPP-2469) #1136

Merged
merged 29 commits into from
Sep 11, 2020
Merged

Overhaul of DiagnosisKey related Download and Caching Mechanisms (EXPOSUREAPP-2469) #1136

merged 29 commits into from
Sep 11, 2020

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Sep 9, 2020 •
edited
Loading

This PR is an overhaul of the existing logic for downloading and caching diagnosis keyfiles.

  • Supports multiple countries (interop).
  • Removes any singleton use in this scope and replaces it with dependency injection.
  • Adds tests for all classes in this scope (~100 new tests).
  • Migrates old cache files to the new logic to preserve bandwitdth.
  • Download API, key fetching and caching is now strongly typed (no longer using raw strings) to possibly catch errors at compile time.
  • The download API is now encoded in a DownloadAPIV1 and no longer using string constants. If there is a V2 api, we will make a new interface.
  • The KeyCacheRespoitory` hides all file/storage details and only provides direct access to paths on demand (to prevent hardcoding paths that may change, i.e. move to sdcard). It also uses an unencrypted database for storing the keyfile metadata to offer less bug attack surface encryption issues (i.e. error 9002). We don't need encryption for information that is public anyways all files + meta data comes from a public CDN that everyone has access to.
  • The KeyFileDownloader is now just responsible for determining the missing days and downloading them, i.e. syncing our cache with the server. The internal "fetch last 3 hours" is no longer "TODO remove" and has been rewritten to potentially support future hour based key file download.

TODO:

  • Complete tests for KeyFileDownloader
  • Wait for architecture decision if we should continue if any file download fails (which it now does due to higher chance for download failures when downloading countries*days files).
  • Replace FileStorageHelper with DeviceStorage

How to test

  • Use the app and trigger key downloads in various situations
  • Enter the test menu and trigger repeated downloads
  • Open logcat and filter for Download
  • Install an old version (dev) fetch keys, upgrade to this PR, install, fetch keys, check our apps cache folder, the old keys should be gone and the log should show that we migrated the old files, and we didn't download the whole again (it just get's the files checksum from the headers and compares it with our old keys).

@d4rken
Rework of keyfile download and caching.
b754d07 
* Supports interop
* More modular for better testing and build flavour based behavior adjustments
* More resilient handling of failed downloads
* Preperations for future hourly download and serverside checksums

TODO: Finish unit tests, keycache migration and cache health check
@d4rken
First batch of unit tests and some fixes for incorrect behavior that …
c67eaf9 
…the tests surfaced.
@d4rken
Added unit tests for the KeyCacheRepository
3000bdc 
TODO: Tests for downloader and migration.
@d4rken
Implemented POC for migration old key files.
f256fef
@d4rken
Fixed legacy file migration and cleanup, improved logging.
5671e66
@d4rken
Added unit tests for legacy key file migration.
0a1ffb3
@d4rken
Add fallback for different file hashes in the header.
8905532
@d4rken d4rken added enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers sprint5 1.5.0 labels Sep 9, 2020
@d4rken d4rken added 1.5.0 and removed 1.4.0 labels Sep 10, 2020
@d4rken d4rken requested a review from chris-cwa September 10, 2020 09:41
@d4rken d4rken changed the title [Draft] Overhaul of DiagnosisKey related Download and Caching Mechanisms (EXPOSUREAPP-2469) Overhaul of DiagnosisKey related Download and Caching Mechanisms (EXPOSUREAPP-2469) Sep 10, 2020
@d4rken d4rken marked this pull request as ready for review September 10, 2020 10:19
@d4rken d4rken requested a review from a team September 10, 2020 10:19
@d4rken
Copy link
Member Author

d4rken commented Sep 10, 2020

While we are still waiting on feedback if it's okay to continue if a few file downloads failed, but others succeed, changing that is a minor code change. There are no more major changes planned for this PR and it is ready to be reviewed. ❤️

jakobmoellerdev
jakobmoellerdev suggested changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple Style Questions and a few structural points tbd, general Implementation looks good.

...-Warn-App/src/main/java/de/rki/coronawarnapp/transaction/RetrieveDiagnosisKeysTransaction.kt Outdated Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/download/KeyFileDownloader.kt Outdated Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/download/KeyFileDownloader.kt Outdated Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/download/KeyFileDownloader.kt Outdated Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/download/KeyFileDownloader.kt Outdated Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/storage/KeyCacheRepository.kt Outdated Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/storage/KeyCacheRepository.kt Show resolved Hide resolved
...p/src/main/java/de/rki/coronawarnapp/diagnosiskeys/storage/legacy/LegacyKeyCacheMigration.kt Show resolved Hide resolved
...p/src/main/java/de/rki/coronawarnapp/diagnosiskeys/storage/legacy/LegacyKeyCacheMigration.kt Outdated Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/TimeStamper.kt Show resolved Hide resolved
d4rken added 12 commits September 10, 2020 22:29
@d4rken
Code changes based on PR comment, part #1.
3143c09
@d4rken
Code fluff, formatting.
949f8f2
@d4rken
Handle download errors correctly.
fa196fc
@d4rken
Refactoring:
bcc6418 
* Remove unnecessary `currentDate` we always start with the newest date from the servers index.
* Make a specialised class for header validation
@d4rken
Let legacy cache migration abort early, depending on whether the key …
7fd919b 
…dir exists.
@d4rken
If we can't create the base directory for the key repo, throw an exce…
4fa414a 
…ption.
@d4rken
Delete cache entry if a download fails.
610aa6e
@d4rken
Fixed test regression due to refactoring.
d694bac
@d4rken
Consolidate staleness check into getStale
e16e6b4
@d4rken
Consolidate clean up for failed downloads into the download method.
8c4e312 
Added tests to check that we delete the keycache entry if the download fails (which we didn't for hours :O!)
@d4rken
Because the hour-mode uses caching too, we add an explicit button to …
10407cf 
…the test menu that clears the cache.
@d4rken
Add comment with reference to ticket regarding follow up on the other…
5b87f7f 
… headers.
chris-cwa
chris-cwa approved these changes Sep 11, 2020
View reviewed changes
Copy link
Contributor

@chris-cwa chris-cwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐️⭐️⭐️⭐️⭐️

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/DiagnosisKeysModule.kt Show resolved Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/download/KeyFileDownloader.kt Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Sep 11, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 8 Code Smells

79.4% 79.4% Coverage
0.0% 0.0% Duplication

@d4rken d4rken merged commit f7f185a into feature/interoperability Sep 11, 2020
@d4rken d4rken deleted the feature/interop-caching-logic-2469 branch September 11, 2020 13:46
d4rken added a commit that referenced this pull request Sep 18, 2020
@jakobmoellerdev @kolyaopahle
@apopovsap @mertsafter @d4rken
Interoperability to 1.5.X (#1173)
6054bf5 
* Added first set of country flags

Signed-off-by: Kolya Opahle <k.opahle@sap.com>

* Added first implementation of Submission Country Selection Screen. (Currently not part of nav graph).
Moved stuff inside of submission package into subpackages

Signed-off-by: Kolya Opahle <k.opahle@sap.com>

* Added missing changes to nav_graph file

* Test Menu options for Country selection and Measurement (EXPOSUREAPP-2229) (#1086)

* Implement country filter (#2229)

* Implement measure for Risk calculation and Key Retrieval (#2229)

* Some code cleanup for measureRiskLevelAndKeyRetrieval

* Some code cleanup (#2229)

* Implement usage of AppConfig usage for country codes (#2229)

* change CURRENT_COUNTRY val to var

* Suppress "LongMethod" warning (#2229)

* Adjust unit tests to respect new country filter

* Align with backend for app config country property name

* Display total file size of keys in results (#2229)

* Fix linting issue

* Update strings.xml files

* Do refactoring and some code clean up

* Move RiskLevel and Key retrieval measurement in own class

* Move code in logical classes. Remove translation in test fragment

* Fix linting and Unit tests

* Remove 3hour boolean check from debug class.

* Refactor CountryWrapper and missing dates check.

* Fix linting and Unit tests

* Add Unit test for getMissingDates

* Code cleanup

* Hide keyboard on action

* Extend statistics for api submission

* Set callback to null after invoked

Co-authored-by: Mert Safter <mert.safter@sap.com>
Co-authored-by: Matthias Urhahn <matthias.urhahn@sap.com>

* Overhaul of DiagnosisKey related Download and Caching Mechanisms (EXPOSUREAPP-2469) (#1136)

* Rework of keyfile download and caching.
* Supports interop
* More modular for better testing and build flavour based behavior adjustments
* More resilient handling of failed downloads
* Preperations for future hourly download and serverside checksums

TODO: Finish unit tests, keycache migration and cache health check

* First batch of unit tests and some fixes for incorrect behavior that the tests surfaced.

* Added unit tests for the KeyCacheRepository

TODO: Tests for downloader and migration.

* Implemented POC for migration old key files.

* Fixed legacy file migration and cleanup, improved logging.

* Added unit tests for legacy key file migration.

* Add fallback for different file hashes in the header.

* Yes kLint, we know it's a long method,
but for this it's better to read it in one block vs jumping to extra methods.

* More linting issues, adjusting project code style prevent a few of these in the future.

* Added missing unit tests for `KeyFileDownloader` and fixed faulty behavior that was noticed during testing.

* CRUD (instrumentation) test for `KeyCacheDatabase`

* Remove unused `FileStorageHelper` and related constants+tests.

* Fix last3Hours unit test in deviceRelease mode, we need to explicitly enable debug for these tests.

* Until we have more information about the hashsum's format in the header, default to `ETag

* Split app config server API from diagnosis key download API,
and reintroduce caching for the app config download.

* Add test to check that the cache is used on flaky connections.

* Code changes based on PR comment, part #1.

* Code fluff, formatting.

* Handle download errors correctly.

* Refactoring:
* Remove unnecessary `currentDate` we always start with the newest date from the servers index.
* Make a specialised class for header validation

* Let legacy cache migration abort early, depending on whether the key dir exists.

* If we can't create the base directory for the key repo, throw an exception.

* Delete cache entry if a download fails.

* Fixed test regression due to refactoring.

* Consolidate staleness check into `getStale`

* Consolidate clean up for failed downloads into the download method.
Added tests to check that we delete the keycache entry if the download fails (which we didn't for hours :O!)

* Because the hour-mode uses caching too, we add an explicit button to the test menu that clears the cache.

* Add comment with reference to ticket regarding follow up on the other headers.

* Move expected storage size per country into a named constant.

Co-authored-by: Matthias Urhahn <matthias.urhahn@sap.com>

* Implementation of tests for the interoperability countries selection screen (EXPOSUREAPP-2439) (#1140)

* Added initial set of viewModel tests

Signed-off-by: Kolya Opahle <k.opahle@sap.com>

* Fail tests on unexpected null.

Co-authored-by: Matthias Urhahn <matthias.urhahn@sap.com>

Co-authored-by: Kolya Opahle <k.opahle@sap.com>
Co-authored-by: apopovsap <66370584+apopovsap@users.noreply.github.com>
Co-authored-by: Mert Safter <69803294+MertSafter@users.noreply.github.com>
Co-authored-by: Mert Safter <mert.safter@sap.com>
Co-authored-by: Matthias Urhahn <matthias.urhahn@sap.com>
Co-authored-by: Matthias Urhahn <darken@darken.eu>
@harambasicluka harambasicluka added this to the 1.5.0 milestone Oct 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chris-cwa chris-cwa chris-cwa approved these changes

@jakobmoellerdev jakobmoellerdev jakobmoellerdev approved these changes

Assignees
No one assigned
Labels
enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers sprint5
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@d4rken @jakobmoellerdev @chris-cwa @harambasicluka