Migrations manager

[ezaquarii]

Migrations now

Currently there is no centrilized migration logic. We have 1 ad-hoc user model migration launched
in MainApp.java:264 that performs some account changes, but:

1. it does not block user interactions until the migration is completed
2. it ignores errors

Although we didn't have much problems with this simple solution, it is no longer scaling given the upcoming needs in WorkManager (#5482) migration and upcoming refactoring work.

Migrations manager

Changes

This PR provides a migration manager extracted from PR #5482 with few modifications done to satisfy androidTests environment. Changes are for the better anyway, so those changes will stay.

1. MigrationManager runs and keeps track of past migrations and failures
2. Migrations is a dumping ground for all sort of migration code for today and the future (Migrate contacts backup job to new background job manager API #5482) and supplies migraiton procedures for manager to run
3. Local tests are still using Robolectric, but androidTest are supplied as well so we can compare and make an informed decision about; see section below about problems discovered during implementation.
4. Added mockito-android for androidTests dependencies

Capabilities

• we can now run migrations only once
• we know when the migration failed and this fact can be used to force application re-installation
• UI can be blocked until all migrations are applied (using the infrastructure provided in Extract account logic from BaseActivity into a mixin #5064, this can be as easy as adding a mixin to base activity)
• migration status flag is observable

Issues with test environment

Things are not straightforward, unfortunately and I discovered some problems with androidTest environment.

• Contrary to local tests running in vanilla JVM, Mockito bytecode generator has some limitations on Android:
• spy() didn't work at all for me; this could be related to spy call real method for doReturn when mockito/mockito-kotlin#374
• Cannot mock invoke() (so can't use mocked functions)
• Cannot mock classes in Kotlin by default, only interfaces; Kotlin classes are final to discourage
  inheritance abuse and must be opened for mocking.
• For some rason, I cannot run all tests in IDE, only clicking them 1-by-1; instrumentation terminates unexpectedly; This could be just my IDE issue so I'd like to ask other devs for confirmation.
• Cannot run androidTests in QA/Dev build due to incompatible user ID (somehting related to screenshots); capability to run tests in non-GPlay variant is important for contributors who use their phone and don't want to overwrite their gplay daily driver app with development build.
• Cannot use backticks to name test functions as test titles and linter enforces camelCaseNames which are imo less readable as test titles (this is easily solvable by Detekt config).

TODO

• Decide what to do when migration fails - we still don't have a valid strategy for now, so the code only supplies a status flag and carries on - this is the current behaviour
• Decide if we still wan to use androidTest for it; in the context of mocking troubles, it's not so black-white anymore
• Linter cleanup for sure
• Replace MainApp.java:264 call with a call to migration manager
• Address migrations that can be re-run after failure ("optional")
• build.gradle cleanup
• Verify if user id migration can be applied only once or needs to be re-applied after new account creation

@AndyScherzinger @tobiasKaminsky review pls

Signed-off-by: Chris Narkiewicz hello@ezaquarii.com

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@b4a6ce9). Click here to learn what that means.
The diff coverage is 76%.

@@            Coverage Diff            @@
##             master    #5546   +/-   ##
=========================================
  Coverage          ?   24.52%           
  Complexity        ?        3           
=========================================
  Files             ?      402           
  Lines             ?    34032           
  Branches          ?     4731           
=========================================
  Hits              ?     8347           
  Misses            ?    24398           
  Partials          ?     1287

Impacted Files	Coverage Δ	Complexity Δ
...m/nextcloud/client/account/UserAccountManager.java	33.33% <ø> (ø)	0 <0> (?)
.../com/nextcloud/client/migrations/MigrationError.kt	100% <100%> (ø)	0 <0> (?)
src/main/java/com/owncloud/android/MainApp.java	49.84% <100%> (ø)	0 <0> (?)
...m/nextcloud/client/migrations/MigrationsManager.kt	100% <100%> (ø)	0 <0> (?)
...java/com/nextcloud/client/appinfo/AppInfoImpl.java	27.27% <100%> (ø)	0 <0> (?)
...c/main/java/com/nextcloud/client/di/AppModule.java	89.65% <100%> (ø)	0 <0> (?)
...xtcloud/client/account/UserAccountManagerImpl.java	57.53% <25%> (ø)	0 <0> (?)
...java/com/nextcloud/client/migrations/Migrations.kt	62.5% <62.5%> (ø)	0 <0> (?)
...xtcloud/client/migrations/MigrationsManagerImpl.kt	77.77% <77.77%> (ø)	0 <0> (?)

[nextcloud-android-bot]

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12843-Screenshot

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12844-IT

[nextcloud-android-bot]

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12845-Screenshot

[ezaquarii]

@tobiasKaminsky I looks like screenshots cannot be uploaded and the build fails because of it.

[nextcloud-android-bot]

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12867-Screenshot

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12877-IT

[tobiasKaminsky]

[ ] Decide what to do when migration fails - we still don't have a valid strategy for now, so the code only supplies a status flag and carries on - this is the current behaviour

When a migration fails it is a serious problem. We then should redirect to github for bug reporting , stop the app and recommend to reinstall it (after all only some settings are lost, but no data).

I know this is a rough step, but if we would allow to continue the app, it might lead to hard reproducable bugs.
(and every migration should be so bullet-proof that this does not really happen).

[tobiasKaminsky]

Regarding tests:
If Robolectric is not working, let us use emulator / instrumented tests.

[ezaquarii]

@tobiasKaminsky I added flag to mark migrations mandatory or not. Optional migrations will not cause application failure.

The use case is the exisitng account migration procedure that can fail, but will try again on subsequent run. This change will preserve existing app behaviour which has been deployed in the field for long time and seems to be working.

Could you please clarify what we do with the tests? I think we had some confusing email exchange and I'm not confident we're on the same page.

We have 2 options:

1. continue with the androidTest tests (phone/emulator tests)
2. continue using Robolectric in tests (local JVM tests, using platform emulation via robolectric)

If option 1 is desired, can we investigate the issues I found during the development, mentioned in the PR description?

[ezaquarii]

When a migration fails it is a serious problem. We then should redirect to github for bug reporting , stop the app and recommend to reinstall it (after all only some settings are lost, but no data).

As we currently do not have any hard-failing migrations (existing one is restarted and ignores failures), shall we address this separately, when such issue arises?

My #5482 PR will add a migration that is "bulletproof", so no need to over-implement at this stage as there will be no need for it now. On the other hand, having a complete solution up front has it's benefits as well. Let me know what strategy will be best here.

[ezaquarii]

@tobiasKaminsky @AndyScherzinger

1. removed robolectric
2. migrations are now called in MainApp

I guess it's shippable stage now let's do a final review with that intention.

[nextcloud-android-bot]

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12898-Screenshot

[ezaquarii]

@tobiasKaminsky User id migration returns error when any account migration fails. Previous implementation was returning succeess if any account succeeded. This was ok assuming migration being re-run on every start.

From now on, once migration succeeds for all accounts, we mark it as applied and no longer re-try.

This has an important implication. Could you tell me if the user id is set properly on account creation? If the purpose of this migration was to fix legacy accounts only - this implementation is correct.

But in case it was intentedd to continuously patch new accounts - we need a to fix authentication flow as well.

[nextcloud-android-bot]

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12905-Screenshot

[nextcloud-android-bot]

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12909-Screenshot

[tobiasKaminsky]

My #5482 PR will add a migration that is "bulletproof", so no need to over-implement at this stage as there will be no need for it now. On the other hand, having a complete solution up front has it's benefits as well. Let me know what strategy will be best here.

Let us tackle failing migrations, once this might occur.
Keep it simple right now :-)

[tobiasKaminsky]

This has an important implication. Could you tell me if the user id is set properly on account creation? If the purpose of this migration was to fix legacy accounts only - this implementation is correct.

Yes this is only for legacy accounts, as we save this for every new account:

android/src/main/java/com/owncloud/android/authentication/AuthenticatorActivity.java

Line 1777 in f8ff0e1

mAccountMgr.setUserData(mAccount, Constants.KEY_USER_ID, userInfo.getId());

[nextcloud-android-bot]

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12942.apk



To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

[nextcloud-android-bot]

Codacy

375

Lint

Type	master	PR
Warnings	92	94
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	70
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	77
Security Warnings	44
Dodgy code Warnings	137
Total	381

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	70
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	77
Security Warnings	44
Dodgy code Warnings	137
Total	381

[nextcloud-android-bot]

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[nextcloud-android-bot]

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12942-Screenshot