Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to TaxonomyRestClient (safe) #2826

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Sep 4, 2023

Parent #2798

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

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 TaxonomyStore and its usages with our main clients suggests that this store is only being utilizing JP/WPAndroid. AFAIA WCAndroid is not using that store and as such can be safely ignored from testing.


PS: @irfano I added you as the main reviewer, randomly so, since I just wanted someone from the Jetpack/WordPress mobile team to be aware of and sign-off on that change for JP/WPAndroid. Feel free to merge this PR directly yourself if you deem so.


Nullability Annotation List:

  1. Add missing n-a to instantiate term model methods
  2. Add missing n-a to remote term payload and children
  3. Add missing n-a to fetch term on taxonomy rest client
  4. Add missing n-a to fetch terms on taxonomy rest client
  5. Add missing n-a to push term on taxonomy rest client
  6. Add missing n-a to delete term on taxonomy rest client
  7. Add missing n-a to term response to term model method on trc
  8. Add missing n-a to term model to params method on trc
  9. Add missing n-a to term wp com rest response fields

Nullability Checks List:

  1. Guard usages of instantiate term model methods

Warnings Resolutions List:

  1. Change type of request to include the missing generic type
  2. Make inner classes static on term wp com rest response

Refactor List:

  1. Reformat taxonomy rest client
  2. Replace anonymous classes with lambda
  3. Remove unused meta field from term wp com rest response

Associated Clients

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


To Test:

  • Smoke test the FluxC Example app via the TAXONOMIES screen. Verify everything is working as expected.
  • In addition to the above, using local-builds.gradle on JP/WPAndroid, smoke test any TaxonomyStore 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. Categories Settings Screen [CategoriesListFragment.kt]

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

  • Go to Menu sub-tab on the initial My Site screen and click on Site Settings under the
    Manage section.
  • Find the Categories under the Writing section and click on it.
  • Verify that the Categories screen is displayed and that everything works as expected.
2. Tags Settings Screen [SiteSettingsTagListActivity.kt]

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

  • Go to Menu sub-tab on the initial My Site screen and click on Site Settings under the
    Manage section.
  • Find the Tags under the Writing section and click on it.
  • Verify that the Tags screen is displayed and that everything works as expected.

Warning:
- "Raw use of parameterized class 'WPComGsonRequest'"
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI.1: 'n-a' stands for 'nullability annotations'.
FYI.2: 'trc' stands for 'taxonomy rest client'.
FYI.1: 'n-a' stands for 'nullability annotations'.
FYI.2: 'trc' stands for 'taxonomy rest client'.
Warnings:
- "Inner class 'TermsResponse' may be 'static'"
- "Inner class 'Meta' may be 'static'"
- "Inner class 'Links' 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.
Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for well structured commits and helpful PR descriptions. LGTM! 👍🏻
I've also tested FluxC example app and JP app and couldn't discover any issue. 🟢
I've added two questions just for my curious, not a blocker but haven't merged the PR just in case. You can merge it after answering my questions.

@ParaskP7 ParaskP7 merged commit 4cb0220 into trunk Sep 6, 2023
13 checks passed
@ParaskP7 ParaskP7 deleted the analysis/add-missing-nullability-annotations-to-taxonomyrestclient branch September 6, 2023 08:17
ParaskP7 added a commit to wordpress-mobile/WordPress-Android that referenced this pull request Sep 11, 2023
Compile Warnings:
- "Unnecessary safe call on a non-null receiver of type TermModel"
- "Condition 'event.term.name == null' is always 'false'"

------------------------------------------------------------------------

As per the removed comment, and although it specifies that sometimes the
API will return a success response with a null name, which then will be
treated as an error, because without a name, there is no category, after
some testing, it has been verified that this is not needed anymore. A
'409 duplicate' error is returned instead, that is, instead of a valid
response but with a 'null' name, which the deleted code was guarding
against.

This previous 'term.name == null' change is related to this JP/WPAndroid
#13256 PR below:

#13256

However, as per this newly merge FluxC #2826 PR below, the
'TermWPComRestResponse' response class and its 'name' field was marked
as '@nonnull'.

wordpress-mobile/WordPress-FluxC-Android#2826

PS: If and whenever the backend starts sending a 'term.name' as
'nullable', then this need to be addressed on the backend side, that is,
instead of making 'term.name' a nullable field on the FluxC side and
start guarding against such occurrences everywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants