crash while account deletion

[Jane232]

Steps to reproduce

1. Have an inactive Nextcloud instance
2. Replace inactive Nextcloud with new one and create account with same user name but other password
3. Try to fetch activities in app
4. Remove account from app
5. Crash

Expected behaviour

Ask for new password
Or not crash if account is deleted

Actual behaviour

Crashed after account deletion

Comment

I dont know what caused that crash, I am aware of the unusual conditions but I wanted to include the circumstances

Report from crash

*********** CAUSE OF ERROR ************

java.lang.RuntimeException
	at com.owncloud.android.ui.activities.ActivitiesActivity$$ExternalSyntheticLambda0.get(Unknown Source)
	at com.nextcloud.java.util.Optional.orElseThrow(Optional.java:309)
	at com.owncloud.android.ui.activity.FileDisplayActivity$8.run(FileDisplayActivity.java:2061)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:148)
	at android.app.ActivityThread.main(ActivityThread.java:5525)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:730)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:620)

************ APP INFORMATION ************
ID: com.nextcloud.client
Version: 30190090
Build flavor: generic

************ DEVICE INFORMATION ************
Brand: lge
Device: p1
Model: LG-H815
Id: MRA58K
Product: p1_global_com

************ FIRMWARE ************
SDK: 23
Release: 6.0
Incremental: 18092193816c9

[ezaquarii]

@Jane232 Thank you for the report.

This stacktrace does not fit the latest source code, but it looks like it crashes when we're using getUser().getOrThrow().

A bit of historical context. Before we started the (slow) migration from Account to User, all activities were full of getAccount() calls returning nullable Account value. The code was assuming non-null value and not handling alternative, causing a lot of NPEs. Sadly, there were just too many weird paths to fix, especially around account deletion and switching workflows. There is simply too much accidental state spread around to cope with in any sensible way.

When we changed Account to User, we marked those code paths that were obviously null-unsafe with usage of Optional<User>, strongly encouraging callers to use isPresent(), instead of assuming valid values. Code that was unfixable in cost-effective manner was just marked by throwing orElseThrow(). This did not change the original behaviour - in old code, it would simply return null and crash in some other place.

The proper way of fixing those issues is to finish migration to User and get rid of nullable code paths. That means eliminating Optional<User> too, as User type is designed to be null-safe by using AnonymousUser. Monkey-patching incidental cases is not a cost-effective solution for this problem, IMO.

Unless it has detrimental effect of everyday app use, I'd mark it as wontfix.

[ezaquarii]

@tobiasKaminsky @AlvaroBrey FYI

[joshtrichards]

Fixed by #11518