New user model by ezaquarii · Pull Request #4835 · nextcloud/android

ezaquarii · 2019-11-16T22:18:22Z

Here is the initial proposal of a new user model. There is very little there so far in terms of features, as I migrated few small portions of code to see what happens. There is a POJO and some trivial API changes.

The User object offers 2 methods to obtain legacy account formats:

Account User.toPlatformAccount()
OwnCloudAccount User.toOwnCloudAccount()

Migration plan:

Start from CurrentAccountProvider.getCurrentAccount() and replace a call with getUser() or try eliminating any existing User.to*Account()
Provide User instance in place of Account / OwnCloudAccount
Migrate code to User and plug any potential rabbit holes with User.toPlatformAccount() or User.toOwnCloudAccount()
Rinse and repeat until code compiles and runs
while(!done) goto 1

This means that temporarily we will be deadling with 5 account formats, but the remaining formats should be phased out interatively.

The use of User interface is not about anectodal Java baclava abstractions. One of my goals is to eliminate "no user" if case by providing a default, semantically correct user value, eliminating if-ology around null values, for example from DB queries.

I think that down the road we'll probably leave only one POJO type, but for the time being I decided to have 2 independent implementations behind an interface. This makes unit testing a bit more complex with mocking (instead of using a value), but I'm not yet sure if we'll be able to collapse both cases into 1 type and it's just easier to run development with 2 independent implementations. If we change decision later - client code won't be affected, so this is a safest route.

One of the things that come to my mind when looking at the code was that the accidental CurrentAccountProvider interface[1] can be turned into something like Session and possibly
enable multiple parallel operations that currently depend on selected user, such as background sync.

2 simple cases were migrated as a proof of concept:

ConnectivityService - somehow I started here as it had pretty complete test suite
UploadsStorageManager - you can see how the default "empty user" simplified if guards around DB queries

Also, I had to backport Java 8 Optional<T> as it is not available on all Android API levels. Since this backport is semantically identical with the original, once Java 8 features become available (either by bumping min API level or using a support library), the change will be as easy as changing the package. Please bear in mind that the code is copyrighted by Oracle (still GNU GPL 2, so should be ok).

I think I'll stop it here and allow us to review and merge this before starting migrating other areas of code, as this migrations - although rather trivial - needs incremental manual testing and I'm not very keen on dumping big diffs for review.

[1] - we created it as a temporary measure to break dependency loops in the early days, where everything depended on AccountManager for exactly 1 method.

nextcloud-android-bot · 2019-11-17T00:57:05Z

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

nextcloud-android-bot · 2019-11-17T01:08:07Z

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

nextcloud-android-bot · 2019-11-17T19:58:48Z

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

nextcloud-android-bot · 2019-11-17T22:10:47Z

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

nextcloud-android-bot · 2019-11-17T22:20:30Z

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

ezaquarii · 2019-11-17T22:26:57Z

@tobiasKaminsky @AndyScherzinger Can you tell me if the account name in form of username@fqdn is written in stone? I'm keen on moving away from free-form string and create a special type for it that will ensure account ID validity. Implementing CharSequence will ensure semantic compatibility with old String flavor.

ezaquarii · 2019-11-17T22:32:28Z

build failure has something to do with IT test - not sure if this is my fault, but I doubt. Logs show that the emulator can't find tests for some reason.
Codacy flags some issues with code backported from java.util - I fixed some of them, but some fixes would break the interface, so I'm not keen on doing it.

nextcloud-android-bot · 2019-11-17T22:57:33Z

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

nextcloud-android-bot · 2019-11-17T23:10:13Z

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

nextcloud-android-bot · 2019-11-17T23:44:04Z

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

nextcloud-android-bot · 2019-11-18T00:01:22Z

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

nextcloud-android-bot · 2019-11-18T16:23:24Z

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

nextcloud-android-bot · 2019-11-18T21:01:21Z

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

ezaquarii · 2019-11-18T21:25:03Z

GPlay tests are still failing to run:

09:01:15 I/InstrumentationResultParser: test run failed: 'Instrumentation run failed due to 'Process crashed.''

ezaquarii · 2019-11-19T21:58:55Z

Maybe you have a better idea?

I addressed both concerns:

Anonymous user is now creaed using account type retrieved from Context
ConnectivityService detects empty URL and finishes fast with "walled" status true + unit test

tobiasKaminsky · 2019-11-20T15:01:41Z

Nice @ezaquarii thank you for the fast fixes 👍

I will resolve kotlin lint checks and then merge it.

ezaquarii · 2019-11-20T21:03:38Z

@tobiasKaminsky I squashed and rebased on top of master.

tobiasKaminsky · 2019-11-21T09:05:42Z

CI was cancelled, I restarted it and will merge afterwards.

New non-nullable user model abstracts away all complexities of Nextcloud user account and provides. - backported java.util.Optional - extended UserAccountManager with User getters - deprecated CurrentAccountProvider.getCurrentAccount() method - migrated connectivity service to new user model - migrated UploadsStorageManager to new user model - migrated OCFileListAdapter Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

ezaquarii · 2019-11-21T20:02:53Z

CurrentAccountProvider interface is no longer functional and test could not be compiled as it used lambda. I added default implementation to fix it.

nextcloud-android-bot · 2019-11-21T20:04:28Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11698.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 · 2019-11-21T20:05:01Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
- Added 5
           

Complexity increasing per file
==============================
- src/main/java/com/nextcloud/java/util/Optional.java  6
         

Complexity decreasing per file
==============================
+ src/main/java/com/nextcloud/client/network/ConnectivityServiceImpl.java  -1

See the complete overview on Codacy

nextcloud-android-bot · 2019-11-21T20:05:02Z

src/main/java/com/owncloud/android/ui/adapter/OCFileListAdapter.java


                // get ocFile from Server to have an up-to-date copy
-                RemoteOperationResult result = new ReadFileRemoteOperation(ocShare.getPath()).execute(account,
+                RemoteOperationResult result = new ReadFileRemoteOperation(ocShare.getPath()).execute(user.toPlatformAccount(),


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2019-11-21T20:05:03Z

src/main/java/com/nextcloud/java/util/function/Predicate.java

+     */
+    default Predicate<T> or(Predicate<? super T> other) {
+        if (other == null) {
+            throw new NullPointerException();


Issue found: Avoid throwing null pointer exceptions.

nextcloud-android-bot · 2019-11-21T20:05:04Z

src/main/java/com/nextcloud/java/util/Optional.java

+    /**
+     * Common instance for {@code empty()}.
+     */
+    private static final Optional<?> EMPTY = new Optional<>();


Issue found: Field EMPTY has the same name as a method

nextcloud-android-bot · 2019-11-21T20:05:05Z

src/main/java/com/nextcloud/java/util/Optional.java

+        } else {
+            Optional<U> u = mapper.invoke(value);
+            if (u == null) {
+                throw new NullPointerException();


Issue found: Avoid throwing null pointer exceptions.

nextcloud-android-bot · 2019-11-21T20:05:06Z

src/main/java/com/nextcloud/java/util/Optional.java

+     * should exist per VM.
+     */
+    private Optional() {
+        this.value = null;


Issue found: Assigning an Object to null is a code smell. Consider refactoring.

nextcloud-android-bot · 2019-11-21T20:10:00Z

Codacy

567

Lint

Type	master	PR
Warnings	59	59
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	119
Security Warnings	44
Dodgy code Warnings	136
Total	422

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	119
Security Warnings	44
Dodgy code Warnings	136
Total	422

codecov · 2019-11-21T20:12:48Z

Codecov Report

Merging #4835 into master will decrease coverage by 0.04%.
The diff coverage is 28.26%.

@@             Coverage Diff              @@
##             master    #4835      +/-   ##
============================================
- Coverage     17.68%   17.63%   -0.05%     
  Complexity        3        3              
============================================
  Files           377      383       +6     
  Lines         32403    32509     +106     
  Branches       4580     4587       +7     
============================================
+ Hits           5729     5734       +5     
- Misses        25753    25859     +106     
+ Partials        921      916       -5

Impacted Files	Coverage Δ	Complexity Δ
...va/com/nextcloud/client/network/NetworkModule.java	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
...m/nextcloud/client/account/UserAccountManager.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...xtcloud/client/account/CurrentAccountProvider.java	`0% <0%> (ø)`	`0 <0> (?)`
...va/com/nextcloud/java/util/function/Predicate.java	`0% <0%> (ø)`	`0 <0> (?)`
...rc/main/java/com/nextcloud/java/util/Optional.java	`0% <0%> (ø)`	`0 <0> (?)`
...cloud/android/datamodel/UploadsStorageManager.java	`25% <0%> (+0.63%)`	`0 <0> (ø)`	⬇️
...c/main/java/com/nextcloud/client/account/Server.kt	`100% <100%> (ø)`	`0 <0> (?)`
...ncloud/android/ui/fragment/OCFileListFragment.java	`25.57% <100%> (ø)`	`0 <0> (ø)`	⬇️
...owncloud/android/ui/adapter/OCFileListAdapter.java	`30.05% <40.9%> (+0.28%)`	`0 <0> (ø)`	⬇️
...xtcloud/client/account/UserAccountManagerImpl.java	`46.76% <44.82%> (-3.24%)`	`0 <0> (ø)`
... and 16 more

ezaquarii · 2019-11-21T20:23:30Z

@tobiasKaminsky Finally CI is green. Ready. Phew...

c8eba1d Merge pull request #4835 from nextcloud/ezaquarii/new-user-model 609b996 New user model 44220d7 Merge pull request #4867 from nextcloud/dependabot/gradle/androidx.exifinterface-exifinterface-1.1.0 6e5d7a8 daily dev 20191121

…ovider Remove getCurrentAccount() method from CurrentAccountProvider interface. The method was deprecated in favor of getUser() and had only one external caller (DrawerActivity), which has been migrated to use getUser() instead. The implementation in UserAccountManagerImpl remains as it's still used internally by getUser(), but removing it from the interface prevents new code from depending on the deprecated Account-based API. Refs: #4853, #4835 Signed-off-by: Josh <josh.t.richards@gmail.com>

ezaquarii requested review from AndyScherzinger and tobiasKaminsky November 16, 2019 22:18

ezaquarii force-pushed the ezaquarii/new-user-model branch 3 times, most recently from 2c5b6dd to 318e80a Compare November 17, 2019 00:50

ezaquarii force-pushed the ezaquarii/new-user-model branch from bf201fe to 1c44424 Compare November 17, 2019 22:07

ezaquarii force-pushed the ezaquarii/new-user-model branch from 1c44424 to 0098d14 Compare November 17, 2019 22:14

ezaquarii added the 3. to review label Nov 17, 2019

ezaquarii force-pushed the ezaquarii/new-user-model branch from 0098d14 to 84d42ff Compare November 17, 2019 22:52

nextcloud deleted a comment from ezaquarii Nov 17, 2019

ezaquarii force-pushed the ezaquarii/new-user-model branch from 84d42ff to e3151d8 Compare November 17, 2019 23:01

ezaquarii force-pushed the ezaquarii/new-user-model branch from e3151d8 to e3b5eed Compare November 17, 2019 23:40

ezaquarii force-pushed the ezaquarii/new-user-model branch from e3b5eed to f847f76 Compare November 17, 2019 23:53

ezaquarii force-pushed the ezaquarii/new-user-model branch from f847f76 to 8f26d24 Compare November 18, 2019 16:12

nextcloud deleted a comment from ezaquarii Nov 18, 2019

ezaquarii force-pushed the ezaquarii/new-user-model branch from 8f26d24 to d03f723 Compare November 19, 2019 21:56

nextcloud deleted a comment from ezaquarii Nov 20, 2019

ezaquarii force-pushed the ezaquarii/new-user-model branch 2 times, most recently from 61c2e72 to 85ffb1c Compare November 20, 2019 21:03

nextcloud deleted a comment from ezaquarii Nov 20, 2019

tobiasKaminsky approved these changes Nov 21, 2019

View reviewed changes

ezaquarii force-pushed the ezaquarii/new-user-model branch from 85ffb1c to 609b996 Compare November 21, 2019 20:01

nextcloud-android-bot reviewed Nov 21, 2019

View reviewed changes

tobiasKaminsky merged commit c8eba1d into master Nov 22, 2019

delete-merged-branch bot deleted the ezaquarii/new-user-model branch November 22, 2019 06:57

AndyScherzinger added this to the Nextcloud App 3.10.0 milestone Nov 22, 2019

joshtrichards mentioned this pull request Feb 8, 2026

refactor: remove deprecated getCurrentAccount() from interface #16484

Closed

1 task

Uh oh!

Conversation

ezaquarii commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

ezaquarii commented Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezaquarii commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 17, 2019

Uh oh!

nextcloud-android-bot commented Nov 18, 2019

Uh oh!

nextcloud-android-bot commented Nov 18, 2019

Uh oh!

nextcloud-android-bot commented Nov 18, 2019

Uh oh!

ezaquarii commented Nov 18, 2019

Uh oh!

ezaquarii commented Nov 19, 2019

Uh oh!

tobiasKaminsky commented Nov 20, 2019

Uh oh!

ezaquarii commented Nov 20, 2019

Uh oh!

tobiasKaminsky commented Nov 21, 2019

Uh oh!

ezaquarii commented Nov 21, 2019

Uh oh!

nextcloud-android-bot commented Nov 21, 2019

Uh oh!

nextcloud-android-bot commented Nov 21, 2019

Uh oh!

nextcloud-android-bot Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

nextcloud-android-bot Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

nextcloud-android-bot Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

nextcloud-android-bot Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

nextcloud-android-bot Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

nextcloud-android-bot commented Nov 21, 2019

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

Uh oh!

codecov bot commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ezaquarii commented Nov 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ezaquarii commented Nov 16, 2019 •

edited

Loading

ezaquarii commented Nov 17, 2019 •

edited

Loading

codecov bot commented Nov 21, 2019 •

edited

Loading