Fix exception handling in NextcloudClient #394

ezaquarii · 2020-02-03T22:32:34Z

Fixes #393
Fixes nextcloud/android#5245
Fixes nextcloud/android#5305
Fixes nextcloud/android#5310
Fixes nextcloud/android#5331
Fixes nextcloud/android#5333
Fixes nextcloud/android#5390

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

ezaquarii · 2020-02-03T22:34:14Z

src/main/java/com/nextcloud/common/NextcloudClient.kt

        return method.execute(this)
    }

+    @Throws(IOException::class)


@tobiasKaminsky This will proably break clients, but since IOException is checked, it must be handled.
There is no API to return valid Response in case of error, as OkHttp3 use exceptions to signal abnormal condition. We should follow.

Hm. I would go instead with try/catch so that we can return our own error code.
After all, this is in our library and also should only be called within the library.

I wrapped the exception in a union-like object.

ezaquarii · 2020-02-03T22:36:23Z

src/main/java/com/nextcloud/common/NextcloudClient.kt


-class NextcloudClient(var baseUri: Uri, val context: Context) {
+class NextcloudClient(var baseUri: Uri, val context: Context, client: OkHttpClient) {
    lateinit var credentials: String


Misused lateinit. This should be initialized here. I'll open another ticket for this, tho.

This is fixed via #396

ezaquarii · 2020-02-03T22:38:54Z

@tobiasKaminsky Could you pealse check the method annotated with @Throws? I won't have bandwidth to do it now. I guess some client must be fixed to handle the IOException.

codecov · 2020-02-03T22:40:45Z

Codecov Report

Merging #394 into master will increase coverage by 0.24%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
+ Coverage      37%   37.24%   +0.24%     
==========================================
  Files         133      134       +1     
  Lines        5737     5751      +14     
  Branches      751      751              
==========================================
+ Hits         2123     2142      +19     
+ Misses       3289     3285       -4     
+ Partials      325      324       -1

Impacted Files	Coverage Δ
.../main/java/com/nextcloud/common/NextcloudClient.kt	`50% <100%> (+11.11%)`	⬆️
.../main/java/com/nextcloud/common/ResponseOrError.kt	`100% <100%> (ø)`
...main/java/com/nextcloud/common/OkHttpMethodBase.kt	`57.14% <62.5%> (+4.01%)`	⬆️
...d/lib/common/operations/RemoteOperationResult.java	`42.07% <0%> (+0.3%)`	⬆️
...wncloud/android/lib/resources/files/FileUtils.java	`85.71% <0%> (+14.28%)`	⬆️

tobiasKaminsky

Extract ResponseOrError

tobiasKaminsky · 2020-02-05T10:19:03Z

src/main/java/com/nextcloud/common/NextcloudClient.kt

-    val client: OkHttpClient
+    val client = client
+
+    data class ResponseOrError private constructor(val result: Response?, val error: Exception?) {


Nice approach 👍
To not have too many things in one class, can we extract this into its own class?

ezaquarii · 2020-02-06T06:35:40Z

@tobiasKaminsky I made the okhttp3 execute() request internal. Please confirm if this is what we can do, ie. we don't plan to use it outside the library.

Fixes #393 Fixes nextcloud/android#5245 Fixes nextcloud/android#5305 Fixes nextcloud/android#5310 Fixes nextcloud/android#5331 Fixes nextcloud/android#5333 Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

nextcloud-android-bot · 2020-02-06T17:53:16Z

Lint

Type	master	PR
Warnings	0	0
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	14
Correctness Warnings	39
Internationalization Warnings	6
Malicious code vulnerability Warnings	7
Multithreaded correctness Warnings	3
Performance Warnings	17
Security Warnings	1
Dodgy code Warnings	58
Total	145

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	14
Correctness Warnings	39
Internationalization Warnings	6
Malicious code vulnerability Warnings	7
Multithreaded correctness Warnings	3
Performance Warnings	17
Security Warnings	1
Dodgy code Warnings	58
Total	145

tobiasKaminsky · 2020-02-07T07:52:43Z

@tobiasKaminsky I made the okhttp3 execute() request internal. Please confirm if this is what we can do, ie. we don't plan to use it outside the library.

Should be fine, I currently cannot imagine why we need this outside our lib, but we can still change it later, if required.

ezaquarii requested a review from tobiasKaminsky February 3, 2020 22:32

ezaquarii commented Feb 3, 2020

View reviewed changes

ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 6276eef to 31a41df Compare February 3, 2020 22:35

ezaquarii commented Feb 3, 2020

View reviewed changes

tobiasKaminsky mentioned this pull request Feb 4, 2020

remove lateinit #396

Merged

ezaquarii added the 3. to review label Feb 4, 2020

ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 31a41df to 78ec45a Compare February 4, 2020 23:11

AndyScherzinger mentioned this pull request Feb 5, 2020

Nextcloud crashes on attempting to open a text file. nextcloud/android#5390

Closed

tobiasKaminsky requested changes Feb 5, 2020

View reviewed changes

nextcloud deleted a comment from ezaquarii Feb 5, 2020

ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 78ec45a to eb51e04 Compare February 5, 2020 22:14

ezaquarii requested a review from tobiasKaminsky February 5, 2020 22:14

ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from eb51e04 to 220afbb Compare February 6, 2020 06:31

Fix exception handling in NextcloudClient

803df96

Fixes #393 Fixes nextcloud/android#5245 Fixes nextcloud/android#5305 Fixes nextcloud/android#5310 Fixes nextcloud/android#5331 Fixes nextcloud/android#5333 Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 220afbb to 803df96 Compare February 6, 2020 16:18

tobiasKaminsky approved these changes Feb 7, 2020

View reviewed changes

tobiasKaminsky merged commit e8f50d6 into master Feb 7, 2020

delete-merged-branch bot deleted the ezaquarii/fix-nextcloud-client-exception-handling branch February 7, 2020 07:53

This was referenced Feb 9, 2020

Repeatedly logging out nextcloud/android#5438

Closed

Timeout exception is not catched nextcloud/android#5437

Closed

Fix exception handling in NextcloudClient #394

Fix exception handling in NextcloudClient #394

Uh oh!

Conversation

ezaquarii commented Feb 3, 2020 • edited by AndyScherzinger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezaquarii Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

ezaquarii Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

ezaquarii Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

ezaquarii commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tobiasKaminsky left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

ezaquarii Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

ezaquarii commented Feb 6, 2020

Uh oh!

nextcloud-android-bot commented Feb 6, 2020

Lint

SpotBugs (new)

SpotBugs (master)

Uh oh!

tobiasKaminsky commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ezaquarii commented Feb 3, 2020 •

edited by AndyScherzinger

Loading

ezaquarii commented Feb 3, 2020 •

edited

Loading

codecov bot commented Feb 3, 2020 •

edited

Loading