Make response member in OkHttpMethodBase nullable #380

ezaquarii · 2020-01-11T20:21:38Z

The class uses non-null lateinit variable that
is not initialized. The member should be of
nullable type.

Fixes nextcloud/android#5073
Fixes nextcloud/android#5102
Fixes nextcloud/android#5100
Fixes nextcloud/android#5142
Fixes nextcloud/android#5156

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

ezaquarii · 2020-01-11T20:23:31Z

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

                                val useOcsApiRequestHeader: Boolean) {
-    lateinit var response: Response
+    private var response: Response? = null
    var queryMap: Map<String, String> = HashMap()


@tobiasKaminsky Is there a reason why those members are public?

No, I just forgot that the default visibility in Kotlin is different than in Java.

I wanted to avoid null, and thus I used lateinit, but seems to be wrong?

Yes, it is incorrect use. Keyword latetinit is a workaround for situations where you want to declare a non-nullable value, but it cannot be initialized immediately. Kotlin requires all variables to be initailized in a primary constructor (this eliminates whole class of bugs). When this is not possible (for example, when you can set it in onCreate(), you can annotate a variable lateinit. Kotlin will generate some code that checks if variable is initialized before first use and throw NPE if not, but the variable is still non-nullable.

This is not an alternative for java.util.Optional or nullable type.

ezaquarii · 2020-01-11T20:24:12Z

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

+    private var response: Response? = null
    var queryMap: Map<String, String> = HashMap()
    var requestHeaders: MutableMap<String, String> = HashMap()
    var requestBuilder: Request.Builder = Request.Builder()


those could be val

codecov · 2020-01-11T20:54:36Z

Codecov Report

Merging #380 into master will decrease coverage by <.01%.
The diff coverage is 29.41%.

@@            Coverage Diff             @@
##           master     #380      +/-   ##
==========================================
- Coverage   34.32%   34.32%   -0.01%     
==========================================
  Files         133      133              
  Lines        5724     5727       +3     
  Branches      751      751              
==========================================
+ Hits         1965     1966       +1     
- Misses       3486     3487       +1     
- Partials      273      274       +1

Impacted Files	Coverage Δ
.../main/java/com/nextcloud/common/NextcloudClient.kt	`30.43% <0%> (ø)`	⬆️
...urces/activities/GetActivitiesRemoteOperation.java	`36.84% <100%> (ø)`	⬆️
...main/java/com/nextcloud/common/OkHttpMethodBase.kt	`53.12% <28.57%> (-2.05%)`	⬇️

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

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

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

src/main/java/com/owncloud/android/lib/resources/activities/GetActivitiesRemoteOperation.java

tobiasKaminsky · 2020-01-13T18:03:57Z

Ready from my side :+1

AndyScherzinger · 2020-01-13T19:26:23Z

@tobiasKaminsky feel free to merge (from my pov)

The class uses non-null lateinit variable that is not initialized. The member should be of nullable type. Fixes nextcloud/android#5073 Fixes nextcloud/android#5102 Fixes nextcloud/android#5100 Fixes nextcloud/android#5142 Fixes nextcloud/android#5156 Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

ezaquarii · 2020-01-14T05:39:15Z

@tobiasKaminsky I squashed the branch, anticipating some tests updates, but I see no unit tests.
If this is all we shall do here, I'm done as well.

nextcloud-android-bot · 2020-01-14T07:08:38Z

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-01-14T08:50:39Z

/backport to stable-2.0

backportbot-nextcloud · 2020-01-14T08:50:56Z

backport to stable-2.0 in #381

ezaquarii requested review from AndyScherzinger and tobiasKaminsky January 11, 2020 20:21

ezaquarii commented Jan 11, 2020

View reviewed changes

ezaquarii force-pushed the ezaquarii/make-response-member-in-okhttpmethodbase-nullable branch from 7e0a6c2 to ee19f90 Compare January 11, 2020 20:38

ezaquarii self-assigned this Jan 11, 2020

ezaquarii force-pushed the ezaquarii/make-response-member-in-okhttpmethodbase-nullable branch 2 times, most recently from e367d17 to e23bc5e Compare January 11, 2020 20:50

ezaquarii added the 2. developing label Jan 11, 2020

AndyScherzinger reviewed Jan 13, 2020

View reviewed changes

src/main/java/com/nextcloud/common/OkHttpMethodBase.kt Outdated Show resolved Hide resolved

AndyScherzinger reviewed Jan 13, 2020

View reviewed changes

src/main/java/com/nextcloud/common/NextcloudClient.kt Outdated Show resolved Hide resolved

tobiasKaminsky requested changes Jan 13, 2020

View reviewed changes

tobiasKaminsky requested a review from AndyScherzinger January 13, 2020 18:03

AndyScherzinger previously approved these changes Jan 13, 2020

View reviewed changes

ezaquarii dismissed AndyScherzinger’s stale review via 2ab8e7e January 14, 2020 05:35

ezaquarii force-pushed the ezaquarii/make-response-member-in-okhttpmethodbase-nullable branch from 6529b8b to 2ab8e7e Compare January 14, 2020 05:35

ezaquarii requested a review from tobiasKaminsky January 14, 2020 05:39

ezaquarii added 3. to review and removed 2. developing labels Jan 14, 2020

tobiasKaminsky approved these changes Jan 14, 2020

View reviewed changes

tobiasKaminsky merged commit cb8e616 into master Jan 14, 2020

delete-merged-branch bot deleted the ezaquarii/make-response-member-in-okhttpmethodbase-nullable branch January 14, 2020 08:39

backportbot-nextcloud bot added the backport-request label Jan 14, 2020

backportbot-nextcloud bot mentioned this pull request Jan 14, 2020

[stable-2.0] Make response member in OkHttpMethodBase nullable #381

Merged

backportbot-nextcloud bot removed the backport-request label Jan 14, 2020

AndyScherzinger added this to the NC Android lib 2.0.0 milestone Jan 14, 2020

Make response member in OkHttpMethodBase nullable #380

Make response member in OkHttpMethodBase nullable #380

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ezaquarii Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

ezaquarii Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

ezaquarii Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasKaminsky commented Jan 13, 2020

Uh oh!

AndyScherzinger commented Jan 13, 2020

Uh oh!

ezaquarii commented Jan 14, 2020

Uh oh!

nextcloud-android-bot commented Jan 14, 2020

Lint

SpotBugs (new)

SpotBugs (master)

Uh oh!

tobiasKaminsky commented Jan 14, 2020

Uh oh!

backportbot-nextcloud bot commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ezaquarii commented Jan 11, 2020 •

edited

Loading

codecov bot commented Jan 11, 2020 •

edited

Loading