[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to `Comment` Network Client Classes (`safe`) #2868

ParaskP7 · 2023-10-11T15:12:20Z

This PR adds any missing nullability annotation to CommentRestClient.java and CommentXMLRPCClient.java, all its related response classes and anything in between (ie. CommentStore). See response classes below:

CommentWPComRestResponse.java

FYI.1: This change is safe, meaning that there shouldn't be any (major) compile-time changes associated with this change. Having said that, testing is highly recommended since the changes in this PR can affect one or more clients (ie. JP/WPAndroid, WCAndroid, etc)

FYI.2: An analysis on CommentStore and its usages with our main clients suggests that this store is only being utilizing by JP/WPAndroid. AFAIA WCAndroid is not using that store and as such can be safely ignored from testing.

PS: Note that CommentStore.java and their respective network client classes, both REST and XMLRPC, seem to be deprecated. The CommentsStore.kt and their respective network client classes, both REST and XMLRPC, seem to be the ones that are actually being actively used atm. Also note that the CommentWPComRestResponse.java response is shared between those two stores. As such, it seems that all comment related API calls from JP/WPAndroid are currently flowing via the new CommentsStore.kt, while only FluxC is using the deprecated CommentStore.java. Be mindful of all that when testing this PR. 🤷

@wzieba let me know how I did on this PR, that is, in terms of updating these network client classes and adding nullability annotations on them and their main CommentWPComRestResponse response class, in order to utilize the nullability annotations added on its fields and thus make its use null proof. If you also feel that this is a good way to proceed with such client PRs, I'll then use this pattern across the board and follow a similar approach on every such PR.

Nullability Annotation List:

Nullability Checks List:

Remove unnecessary payload likes if condition on comment store

Warnings Resolutions List:

Warnings Suppression List:

Refactor List:

Test List:

Associated Clients

It is highly recommending that this change is being tested on the below associated clients:

JP/WPAndroid

To Test (`REST`):

Smoke test the FluxC Example app via the COMMENTS screen. Verify everything is working as expected.
In addition to the above, using local-builds.gradle on JP/WPAndroid, smoke test any CommentStore related functionality on both, the WordPress and Jetpack apps, and see if everything is working as expected. For a couple of examples, you can expand and follow the inner and more explicit test steps within:

To Test (`XMLRPC`):

Create a new self-hosted WordPress site for XMLRPC testing purposes (jurassic.ninja).
Smoke test the FluxC Example app via the COMMENTS screen. Verify everything is working as expected.
In addition to the above, using local-builds.gradle on JP/WPAndroid, smoke test any CommentStore related functionality on both, the WordPress and Jetpack apps, and see if everything is working as expected. For a couple of examples, you can expand and follow the inner and more explicit test steps within:

1. Comment Details Screen from Comments Screen [CommentDetailFragment.java]

ℹ️ This test applies to both, the Jetpack and WordPress app.

Go to Comments screen and tap on any comment.
Verify that the Comment Details screen is shown and functioning as expected.
For example, try replying, approving, marking as spam, liking, trashing, editing or copying a comment's link address. Additionally, make sure to verify that you can still swipe left-right to navigate between comments.

2. Comment Details Screen from Notification Tab [CommentDetailFragment.java]

ℹ️ This test applies to the Jetpack app only.

Go to Notifications tab, then its COMMENTS sub-tab, and tap on any comment.
Verify that the Comment Details screen is shown and functioning as expected.
For example, try replying to a comment. Additionally, make sure to verify that you can still swipe left-right to navigate between comments.

3. Edit Comment Screen from Comments Screen [UnifiedCommentsEditActivity.java]

ℹ️ This test applies to both, the Jetpack and WordPress app.

Go to Comments screen and tap on any comment.
From the More on the right side, at the bottom of the comment, select Edit.
Verify that the Edit Comment screen is shown and functioning as expected.
For example, try editing/adding the comment's Name, Web address, Email address and its Comment itself.

4. Edit Comment Screen from Notification Tab [UnifiedCommentsEditActivity.java]

ℹ️ This test applies to the Jetpack app only.

Go to Notifications tab, then its COMMENTS sub-tab, and tap on any comment.
From the More on the right side, at the bottom of the comment, select Edit.
Verify that the Edit Comment screen is shown and functioning as expected.
For example, try editing/adding the comment's Name, Web address, Email address and its Comment itself.

5. Reader Comments Screen [ReaderCommentListActivity.java]

ℹ️ This test applies to the Jetpack app only.

Go to Reader tab, then its DISCOVER sub-tab, and tap on any post.
Tap on the Comments button at the bottom of the post.
Verify that the Reader Comments screen is shown and functioning as expected.
For example, try (un)following, enabling/disabling in-app notifications, replying to post, and then sharing, replying or liking a specific comment.

Warning: "'switch' statement on enum type 'org.wordpress.android.fluxc.model.CommentStatus' misses case 'UNREPLIED'"

Warning: "Variable is already assigned to this value"

FYI: 'nn-a' stands for 'non-null annotation'.

FYI: 'n-a' stands for 'nullability annotations'.

FYI: 'n-a' stands for 'nullability annotations'. ------------------------------------------------------------------------ You will notice that I added extra conditions for when to trigger 'createNewComment(...)' versus 'createNewReply(...)' versus just throwing an illegal state exception for any other condition. if (payload.post != null && payload.reply == null) { ... } else if (payload.reply != null && payload.post == null) { ... } else { throw IllegalStateException(...) } I did that because the two 'RemoteCreateCommentPayload' constructors are also defined as such and thus the callers should also respect their contract in order to avoid any NPE or an illegal app state. I also chose to keep the 'payload.reply == null' condition and add the extra 'payload.post == null' condition because the 'payload.reply == null' condition was already present.

FYI: 'n-a' stands for 'nullability annotations'.

FYI: 'nl-a' stands for 'nullable annotation'.

Warning: "Explicit type argument CommentModel can be replaced with <>"

FYI: 'n-a' stands for 'nullability annotations'.

Warning: "'if' statement can be replaced with 'switch' statement"

FYI: 'n-a' stands for 'nullability annotations'.

Both those payload constructors are actually meant to and used by client apps (ie. JP/WPAndroid). They just seem unused as they are not being directly used via the example app or any unit/ui tests within this repo.

Warning: "Raw use of parameterized class 'Action'"

The 'instantiateCommentModel(...)' related 'CommentStore' methods is being in production and are only being used on a couple of connected tests. Thus, it is better to remove those and not have them exposed and available for clients like JP/WPAndroid to exploit. PS: This change was done to assist with a future refactor and in order to avoid extra testing.

Warning: "Condition 'payload.likes != null' is always 'true'"

Warning: "Explicit type argument LikeModel can be replaced with <>"

Warnings: "Exception 'java.lang.InterruptedException' is never thrown in the method"

Warning: "'assertEquals()' can be simplified to 'assertNull()'"

Warning: "'application' is deprecated" Explanation: Accessing this field directly is inherently incompatible with 'org.robolectric.annotation.experimental.LazyApplication' and Robolectric makes no guarantees if a test modifies this field during execution. Replacing 'application' with 'getApplication()' resolves this deprecation warning.

Warning: "'assertEquals()' can be simplified to 'assertNull()'"

Warning: "Variable 'commentResponse' is never used"

Warning: "Typo: In word 'Reponse'"

Warnings: - "Unchecked cast: Class<*>! to Class<XMLRPCRequest>" - "Unchecked cast: Any! to Response<Any>"

Warnings: "Typo: In word 'udpated'"

Warnings: - "Inner class 'CommentsWPComRestResponse' may be 'static'" - "Inner class 'Post' may be 'static'" - "Inner class 'Author' may be 'static'"

FYI: 'n-a' stands for 'nullability annotations'. PS: This 'NotNullFieldNotInitialized' warning got suppressed because a 'response' class can never have its fields initialized via a constructor initialization, or otherwise for that matter.

wzieba

I haven't finished review yet, leaving some comments to start discussion though.

wzieba · 2023-10-16T17:26:58Z

fluxc/src/main/java/org/wordpress/android/fluxc/store/CommentsStore.kt

+            throw IllegalStateException(
+                "Either post or reply must be not null and both can't be not null at the same time!"
+            )


This will likely crash client apps in production if this scenario happens - clients don't have possibility to handle this exception. Can we change it to a log and failed OnCommentChange event?

👋 @wzieba and thanks for this comment, this is indeed an interesting one, let me explain:

First, you can take a look at this commit's description: 53e14a0

From there, you will notice that I did that because the two RemoteCreateCommentPayload constructors are also defined as such and thus the callers should also respect their contract in order to avoid any NPE or an illegal app state. I also chose to keep the payload.reply == null condition and add the extra payload.post == null condition because the payload.reply == null condition was already present.

So, all in all, we should never reach this else case and thus if we do it is end-up being an illegal state of the app. Instead of trying to hide this state and somehow mute it I think it is better to throw an exception, that is, if and whenever someone changes how the RemoteCreateCommentPayload is constructed, because if they don't, as it is now, it will never happen anyway.

Does that makes sense? 🙏

the two RemoteCreateCommentPayload constructors are also defined as such and thus the callers should also respect their contract in order to avoid any NPE or an illegal app state

Thanks. When writing this comment, I was under the impression that RemoteCreateCommentPayload is a mapped response from the API. But then I looked at WordPress-Android codebase and understood, that it's a request created via constructor. Let's keep the exception, then!

wzieba · 2023-10-16T17:34:15Z

fluxc/src/main/java/org/wordpress/android/fluxc/store/CommentStore.java

+        }
+    }
+
+    private long getPrioritizedRemoteCommentId(@NonNull RemoteCommentPayload payload) {


What's the motivation behind this method? Are we sure it's valid to use remoteCommentId if comment is null? It seems to change a logic of many methods.

First all all, thanks for this comment @wzieba ! 💯

As discussed, this was done to avoid duplication on having the same logic with the REST and XMLRPC clients themselves, and also risking it be on one of the clients and not the other.

FYI:

First such commit, which creates this extra method: 9f6c87b

Second such commit, which reused this extra method: 12fe3cf

There are move such commits following the same pattern above.

ParaskP7 · 2023-10-17T08:13:32Z

👋 @wzieba and thanks for starting the review and any discussion on the changes, looking forward for us to finish the review! 🙇 ❤️

example/src/test/java/org/wordpress/android/fluxc/common/CommentsMapperTest.kt

wzieba · 2023-10-17T12:16:52Z

example/src/test/java/org/wordpress/android/fluxc/common/CommentsMapperTest.kt

-                authorProfileImageUrl = if (allowNulls) null else "https://gravatar.com/avatar/111222333",
-                postTitle = if (allowNulls) null else "again",
+                remoteSiteId = 100_000L,
+                authorUrl = if (allowNulls) "" else "https://test-debug-site.wordpress.com",


What's the motivation behind assigning empty string to these fields instead of null? null is a valid value for many fields in CommentEntity.

Good question @wzieba ! 👍

Briefly, this is done because otherwise this test suite will fail due to the fact that a some fields on the CommentWPComRestResponse are now defined as @NonNull. Let me explain:

You can try and change authorUrl = if (withEmpty) "" else "https://test-debug-site.wordpress.com", to authorUrl = if (withEmpty) null else "https://test-debug-site.wordpress.com", and see one test failing.

Now, in order to fix that you would need to update URL = entity.authorUrl ?: "" to URL = entity.authorUrl ?: null.

However, you will quickly notice that this is not compiling due to the fact that Author.URL is defined as @NonNull on CommentWPComRestResponse.

Does that make sense? 🙏

Also, this 'SameParameterValue' warning got suppressed for the 'getDefaultCommentList(...)' function as it is currently being used once and as 'getDefaultCommentList(false)'. How this doesn't mean it will not be used in the future, or wasn't used in the past. As such, keeping this method signature and suppressing this warning for the time being seems to be the better approach here. Related PR Comment: https://github.com/wordpress-mobile/ WordPress-FluxC-Android/pull/2868#discussion_r1362018044

wzieba

Thanks for answering the questions @ParaskP7 , looks good to me! I've tested the change on the FluxC example and Jetpack apps and haven't spotted any crashes/problems.

ParaskP7 · 2023-10-18T11:37:34Z

Awesome @wzieba , thanks for reviewing, testing and raising all those questions, you rock! 🙇 ❤️ 🚀

ParaskP7 added 7 commits October 10, 2023 15:35

Refactor: Reformat comment rest client

13840f1

Refactor: Reformat comment xmlrpc client

ae9066c

Refactor: Replace anonymous classes with lambda on comment rest client

a4c64ef

Refactor: Replace anonymous classes with lambda on comment xmlrpc client

c645894

Refactor: Create missing switch branch on comment xmlrpc client

35d0935

Warning: "'switch' statement on enum type 'org.wordpress.android.fluxc.model.CommentStatus' misses case 'UNREPLIED'"

Refactor: Remove complexity from get cmt status from xmlrpc status str

246005f

Warning: "Variable is already assigned to this value"

Analysis: Add missing nn-a to likes utils pvd field on cmt rest client

9fef411

FYI: 'nn-a' stands for 'non-null annotation'.

ParaskP7 added Core Tech Debt labels Oct 11, 2023

ParaskP7 self-assigned this Oct 11, 2023

ParaskP7 force-pushed the analysis/add-missing-nullability-annotations-to-comment-clients branch from 8219631 to 645d3a5 Compare October 11, 2023 15:21

ParaskP7 changed the title ~~[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Comment Client Classes (safe)~~ [Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Comment Network Client Classes (safe) Oct 12, 2023

ParaskP7 added 18 commits October 12, 2023 15:05

Analysis: Add missing n-a to fetch comments on comment clients

1c25861

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to push comment on comment clients

de7f769

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to fetch comment on comment clients

9f6c87b

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to fetch comment likes on comment rest client

14729c2

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to delete comment on comment clients

12fe3cf

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to like comment on comment rest client

d044e75

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to comments response to comment list

c9ccc72

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing nl-a to requested status field on payload

726ea21

FYI: 'nl-a' stands for 'nullable annotation'.

Analysis: Replace explicit type argument with <> on payload

a736e74

Warning: "Explicit type argument CommentModel can be replaced with <>"

Analysis: Add missing n-a to comment response to comment

057436b

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to get cmt status from xmlrpc status string

484463a

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Replace if with switch on comment xmlrpc client

41d27ca

Warning: "'if' statement can be replaced with 'switch' statement"

Analysis: Add missing n-a to comments response to comment list

c6d0db7

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Add missing n-a to comment response to comment

8b2e4ac

FYI: 'n-a' stands for 'nullability annotations'.

Analysis: Suppress long parameter list warning on comments clients

fd05328

Analysis: Suppress too many functions warning on comments xmlrpc client

d0df40f

Analysis: Suppress unused warnings on comment store

0bc6ba3

Both those payload constructors are actually meant to and used by client apps (ie. JP/WPAndroid). They just seem unused as they are not being directly used via the example app or any unit/ui tests within this repo.

ParaskP7 added 16 commits October 12, 2023 15:05

Analysis: Suppress raw types warning on comment store

f34249e

Warning: "Raw use of parameterized class 'Action'"

Analysis: Remove unnecessary payload likes if condition on comment store

b73fbb0

Warning: "Condition 'payload.likes != null' is always 'true'"

Analysis: Replace explicit type argument with <> on payload

816d1e3

Warning: "Explicit type argument LikeModel can be replaced with <>"

Test: Remove interrupted exception from test method's throws list

0ba2a4d

Warnings: "Exception 'java.lang.InterruptedException' is never thrown in the method"

Test: Simplify assert null assertion for connected tests

04d7934

Warning: "'assertEquals()' can be simplified to 'assertNull()'"

Test: Guard usages of get comment by site and remote id method

21ca96e

Test: Simplify assert null assertion for unit tests

048fe85

Warning: "'assertEquals()' can be simplified to 'assertNull()'"

Test: Remove unnecessary comment response local variable from unit test

015f664

Warning: "Variable 'commentResponse' is never used"

Test: Fix typo with comments response on unit test

2bab5c0

Warning: "Typo: In word 'Reponse'"

Test: Suppress unchecked cast on unit test

88d0a1e

Warnings: - "Unchecked cast: Class<*>! to Class<XMLRPCRequest>" - "Unchecked cast: Any! to Response<Any>"

Test: Fix typo with updated comment on unit test

dd40ab2

Warnings: "Typo: In word 'udpated'"

Analysis: Make inner classes static on comment wp com rest response

85fa7bf

Warnings: - "Inner class 'CommentsWPComRestResponse' may be 'static'" - "Inner class 'Post' may be 'static'" - "Inner class 'Author' may be 'static'"

ParaskP7 force-pushed the analysis/add-missing-nullability-annotations-to-comment-clients branch from 645d3a5 to 2e855cd Compare October 12, 2023 12:05

ParaskP7 requested a review from wzieba October 12, 2023 12:31

ParaskP7 marked this pull request as ready for review October 12, 2023 12:32

wzieba reviewed Oct 16, 2023

View reviewed changes

wzieba reviewed Oct 17, 2023

View reviewed changes

wzieba approved these changes Oct 18, 2023

View reviewed changes

ParaskP7 merged commit 6eee26f into trunk Oct 18, 2023
2 checks passed

ParaskP7 deleted the analysis/add-missing-nullability-annotations-to-comment-clients branch October 18, 2023 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to `Comment` Network Client Classes (`safe`) #2868

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to `Comment` Network Client Classes (`safe`) #2868

ParaskP7 commented Oct 11, 2023 •

edited

Loading

wzieba left a comment

wzieba Oct 16, 2023

ParaskP7 Oct 17, 2023

wzieba Oct 17, 2023

wzieba Oct 16, 2023

ParaskP7 Oct 17, 2023 •

edited

Loading

ParaskP7 commented Oct 17, 2023

wzieba Oct 17, 2023

ParaskP7 Oct 17, 2023

wzieba left a comment

ParaskP7 commented Oct 18, 2023

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Comment Network Client Classes (safe) #2868

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Comment Network Client Classes (safe) #2868

Conversation

ParaskP7 commented Oct 11, 2023 • edited Loading

Associated Clients

To Test (REST):

To Test (XMLRPC):

wzieba left a comment

Choose a reason for hiding this comment

wzieba Oct 16, 2023

Choose a reason for hiding this comment

ParaskP7 Oct 17, 2023

Choose a reason for hiding this comment

wzieba Oct 17, 2023

Choose a reason for hiding this comment

wzieba Oct 16, 2023

Choose a reason for hiding this comment

ParaskP7 Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

ParaskP7 commented Oct 17, 2023

wzieba Oct 17, 2023

Choose a reason for hiding this comment

ParaskP7 Oct 17, 2023

Choose a reason for hiding this comment

wzieba left a comment

Choose a reason for hiding this comment

ParaskP7 commented Oct 18, 2023

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to `Comment` Network Client Classes (`safe`) #2868

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to `Comment` Network Client Classes (`safe`) #2868

ParaskP7 commented Oct 11, 2023 •

edited

Loading

To Test (`REST`):

To Test (`XMLRPC`):

ParaskP7 Oct 17, 2023 •

edited

Loading