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.

Sign up for GitHub

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

Jump to bottom

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All Taxonomy & Term SqlUtils & Model Classes (breaking) #2851

Merged
merged 40 commits into from
Oct 4, 2023
Merged

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All Taxonomy & Term SqlUtils & Model Classes (breaking) #2851

merged 40 commits into from
Oct 4, 2023

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Sep 8, 2023 •
edited
Loading

Parent #2799
Closes #2839

This PR adds any missing nullability annotation to all Taxonomy & Term related sql-util & model classes.

FYI: This change is breaking, meaning that any client that depended on the Taxonomy & Term models should update to the new APIs (non-default constructors) as the existing API (default constructor) are now deprecated. As such, this change is inherently risky, meaning that there are compile-time changes associated with this change and thus needs testing to verify correctness, both on the library's and client's side.

PS: Any other changes on any other class are just some propagating missing nullability annotation changes, which stem from the main set of classes being updated, this PR's focus, plus, some other extra minor analysis, refactor and test related changes.

@mkevins @AjeshRPai let me know how I did on this PR, that is, in terms of adding non-default constructors to all the @Table related model, TaxonomyModel.java (actually unused) and TermModel.java (our main focus), in order to utilize the nullability annotations added on their fields and thus make them null proof. If you also feel that this is a good way to proceed with such PRs, I'll then use this pattern across the board and follow a similar approach on every such PR.

FYI: To understand why I proceeded with the non-default constructors, take a look at this WCTopPerformerProductModel.kt @Table related Kotlin data class. You will see that this pattern is actually already used to make it possible to use Kotlin with such models. This gave me a bit more confidence in term of proceeding with this change. Also, by applying this change now, will enable such Java models to be easily converted to Kotlin models in the future.

Nullability Annotations List (Model):

  1. Add missing n-a to taxonomy model
  2. Add missing n-a to term model
  3. Add missing n-a to terms model
  4. Add default and non-default constructors for taxonomy model (breaking)
  5. Add default and non-default constructors for term model (breaking)

Nullability Annotations List (SqlUtils):

  1. Add missing n-a to remove term sql utils methods
  2. Add missing n-a to insert term for result sql utils methods
  3. Add missing n-a to insert or update term sql utils methods
  4. Add missing n-a to clear taxonomy for site sql utils methods

Nullability Annotations List (Store):

  1. Add missing n-a to fetch terms payload
  2. Add missing n-a to fetch terms response payload
  3. Add missing n-a to on term uploaded store
  4. Add missing n-a to on term update/remove term store methods
  5. Add missing n-a to on get tags for post store method
  6. Add missing n-a to on get categories for post store method
  7. Add missing n-a to on get term by name store methods
  8. Add missing n-a to on get term by remote id store methods
  9. Add missing n-a to on get terms for site store methods
  10. Add missing n-a to handle fetch single term completed method
  11. Add missing n-a to handle delete/push term completed methods
  12. Add missing n-a to on taxonomy changed
  13. Add missing n-a to taxonomy error

Warnings Resolution List:

  1. Create missing switch cases for store actions
  2. Use inserted term on instantiate term model store method

Warnings Suppression List:

  1. Suppress condition covered by further condition warning
  2. Suppress unused warnings on get related store methods
  3. Suppress raw types warnings for store action method
  4. Suppress condition covered by further condition warning

Refactor List:

  1. Remove unnecessary taxonomy name field from taxonomy changed
  2. Remove test only taxonomy store methods

Test List:

  1. Add missing final keyword on taxonomy store unit test fields
  2. Resolve robolectric related application deprecation warning
  3. Create missing switch branches for on taxonomy changed
  4. Suppress new class naming convention for connected test
  5. Simplify assertion for check update existing term
  6. Assert for deleted term models on taxonomy store test
  7. Remove unnecessary interrupted exception from private test methods
  8. Guard usages of instantiate term model methods

Associated Clients

It is highly recommending that this change is being tested alongside the below associated clients and their accompanying PRs:

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 and TermModel related instantialization 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.
3. Prepublishing Screens [PrepublishingTagsFragment.kt + PrepublishingCategoriesFragment.kt]

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

  • Go to Posts screen and edit any post.
  • Click on the UPDATE button to have the bottom sheet appear (top-right).
  • Click on the Tags row and add, remove or edit any tag for this post. Click back.
  • Click on the Categories row and add, remove or create a new category for this post. Click back.
  • Click on the UPDATE NOW button to update this post (bottom).
  • Verify that this post's Tags/Categories are updated and that everything works as expected.

Merge instructions:

  1. Wait for the accompanying JP/WPAndroid#19158 to be reviewed and approved.
  2. Remove the [PR] Not Ready for Merge label.
  3. Merge this PR.
  4. Follow-up with the merge instructions on the accompanying JP/WPAndroid#19158 client PR.

@ParaskP7
Analysis: Add missing n-a to fetch terms payload
458ef95 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to taxonomy model
2882210 
FYI: 'n-a' stands for 'nullability annotations'.

This 'TaxonomyModel' is never being used in production, that is, at
least via the JP/WPAndroid app. As such, the nullability on the 'mName',
'mLabel' and 'mDescription' fields are being dictated by tests. As such,
and because every test that is using a 'TaxonomyModel' is also setting
its 'mName' right after, this is why this field is being annotated as
'@nonnull'. The same does not apply for the 'mLabel' and 'mDescription'
fields and as such, those field are being annotated as '@nullable'.

PS: This 'NotNullFieldNotInitialized' warning got suppressed because a
table related 'model' class can never have its fields initialized via a
constructor initialization, or otherwise for that matter.
@ParaskP7
Analysis: Suppress condition covered by further condition warning
a51d38b 
Warning: "Condition 'other == null' covered by subsequent condition
'!(other instanceof TaxonomyModel)'"

FYI: One could resolve this warning, simply by removing the unnecessary
'other == null' condition, but this would change the 'equals(...)' logic
and possible introduce some kind of a regression, most liked performance
wise. Thus, it is better to ignore this atm, just by suppressing it, and
call this out of scope.
@ParaskP7
Analysis: Add missing n-a to fetch terms response payload
4deacc9 
FYI: 'n-a' stands for 'nullability annotations'.

PS.1: This 'NotNullFieldNotInitialized' warning got suppressed because a
'payload' static class has both, a 'normal' and an 'error' payload.
Although a 'normal' payload could have its fields annotated with
'@nonnull', that is, when they are being instantiated as such via its
'normal' constructor, because of the 'error' payload, and its
constructor not needing most of those necessarily, this '@nonnull'
condition might no longer be valid for both those 'payload' classes.
PS.2: With this change, and because an 'error' payload needs to always
go via an 'payload.isError()' check, it is assumed that those
'@nonnull' fields will never be accessed, that is when the
'payload.isError()' check returns true. Thus, and in order to have a
better nullability throughput when on the 'normal' branch, it has been
decided in this change to suppress this 'NotNullFieldNotInitialized'
warning and ignore cases where those fields are incorrectly access, when
on the 'error' branch, which, unfortunately, could potentially cause a
NPE. To make this a bit more clear, an extra comment is added on those
fields that are '@nonnull' on both, the 'normal' and 'error' branches.
This is done in order for future authors to understand which field(s)
can be utilized when on the 'error' branch, and which can't/shouldn't.
@ParaskP7
Analysis: Add missing n-a to on term uploaded store
f37428a 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to on term update/remove term store methods
0b49e4c 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to on get tags for post store method
41c56a5 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to on get categories for post store method
a8d1f27 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to on get term by name store methods
8d75e7b 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to on get term by remote id store methods
a075f39 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to on get terms for site store methods
05a852f 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Test: Add missing final keyword on taxonomy store unit test fields
7de7470 
Warning: "Field 'mTaxonomyStore' may be 'final'"
@ParaskP7
Test: Resolve robolectric related application deprecation warning
c66f686 
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.
@ParaskP7
Test: Create missing switch branches for on taxonomy changed
25118ec 
Error: "'switch' statement on enum type
'org.wordpress.android.fluxc.action.TaxonomyAction' misses cases:
'FETCH_TERM', 'PUSH_TERM', 'DELETE_TERM', 'FETCHED_TERMS',
'FETCHED_TERM', 'PUSHED_TERM', 'DELETED_TERM' and 'REMOVE_ALL_TERMS'"
@ParaskP7
Test: Suppress new class naming convention for connected test
21e6424 
Warning: "Test class name 'ReleaseStack_TaxonomyTestWPCom' doesn't match
regex '[A-Z][A-Za-z\d]*Test(s|Case)?|Test[A-Z][A-Za-z\d]*|IT(.*)|(.*)
IT(Case)?'"
@ParaskP7
Test: Simplify assertion for check update existing term
646d2c6 
Warning: "'assertFalse()' can be simplified to 'assertNotEquals()'"
@ParaskP7
Analysis: Create missing switch cases for store actions
9547923 
Error: "'switch' statement on enum type
'org.wordpress.android.fluxc.action.TaxonomyAction' misses cases:
'UPDATE_TERM', and 'REMOVE_TERM'"
@ParaskP7
Analysis: Suppress unused warnings on get related store methods
30e6133 
All, some or none of those suppressed on 'unused' get related taxonomy
store methods, although seemingly unused, because they are 'public',
client apps like JP/WPAndroid can actually be using those. Thus, they
shouldn't be removed as 'unused'. Instead, and in order to avoid having
them removed, this 'unused' warning is being suppressed instead.
@ParaskP7
Analysis: Suppress raw types warnings for store action method
ab81837 
Warning: "Raw use of parameterized class 'Action'"
@ParaskP7
Analysis: Use inserted term on instantiate term model store method
4db457a 
Warning: "Variable is already assigned to this value"
@ParaskP7
Analysis: Add missing n-a to remove term sql utils methods
7d823c9 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to insert term for result sql utils methods
74fe47a 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to insert or update term sql utils methods
42dba85 
FYI: 'n-a' stands for 'nullability annotations'.

PS: As part of this change, the no longer valid 'testInsertNullTerm'
test, which tests for a nullable term being inserted, is being removed.
@ParaskP7
Test: Assert for deleted term models on taxonomy store test
1d5b073 
Warning: "Return value of the method is never used"

FYI: Not adding this extra assertion is causing the above warning for
the 'TaxonomySqlUtils.clearTaxonomyForSite(...)' method.
@ParaskP7
Analysis: Add missing n-a to clear taxonomy for site sql utils methods
8ff128c 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to handle fetch single term completed method
2fd3d9d 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to handle delete/push term completed methods
0849403 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to on taxonomy changed
42e8bec 
FYI: 'n-a' stands for 'nullability annotations'.

This change makes the 'causeOfChange' field on 'OnTaxonomyChanged'
non-null, which in its turn caused few updates in terms of how
'OnTaxonomyChanged' is being constructed. Otherwise, this
'causeOfChange' field couldn't have been non-null, but having that as
nullable is not justified as it can be easily become and used as
non-null.
@ParaskP7
Refactor: Remove unnecessary taxonomy name field from taxonomy changed
7b53e45 
This 'taxonomyName' was only used within the 'example' app, just for
logging purposes. As such, production wise it seems to not be holding
any real value. This is especially true since JP/WPAndroid is not using
this field anywhere. Thus, it is safe to remove this field and not
having to deal with a nullable 'taxonomyName' field for no particular
good reason, and thus making 'OnTaxonomyChanged' much more
straightforward as a class.
@ParaskP7
Test: Remove unnecessary interrupted exception from private test methods
8602356 
Warning: "Exception 'java.lang.InterruptedException' is never thrown in
the method "
@ParaskP7
Test: Guard usages of instantiate term model methods
30f1880
@ParaskP7
Analysis: Add missing n-a to taxonomy error
52cf542 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Analysis: Add missing n-a to term model
b34ed2f 
FYI: 'n-a' stands for 'nullability annotations'.

1) A 'TermModel' is currently used either for a 'category' or a 'tag'.
2) When a new 'category' is initialized, it always has a 'mTaxonomy',
which is actually a 'category', and also a 'mName', the category name
which is given by the user. However, it doesn't have a 'mSlug' or a
'mDescription'. The 'mSlug' gets added to 'TermModel' with the API
response. A 'category' has no description, thus 'mDescription' is either
'null' or being transformed to an empty string ("") with the API
response.
3) When a new 'tag' is initialized, it always has a 'mTaxonomy', which
is actually a 'tag', and also a 'nName', the tag name, which is given by
the user. In contrast to a 'category' type 'TermModel', a 'tag' does
have a 'mSlug' and can have a 'mDescription' too. The 'mSlug' gets added
during creation via 'mTerm.setSlug(sanitizeWithDashes(thisName))' and as
such getting its 'mSlug' from its 'mName'. The 'mDescription' is either
and empty string (""), if the user chooses to omit this step, or any
other string description, which is given by the user.

PS: This 'NotNullFieldNotInitialized' warning got suppressed because a
table related 'model' class can never have its fields initialized via a
constructor initialization, or otherwise for that matter.
@ParaskP7
Analysis: Suppress condition covered by further condition warning
2f9ad78 
Warning: "Condition 'other == null' covered by subsequent condition
'!(other instanceof TaxonomyModel)'"

FYI: One could resolve this warning, simply by removing the unnecessary
'other == null' condition, but this would change the 'equals(...)' logic
and possible introduce some kind of a regression, most liked performance
wise. Thus, it is better to ignore this atm, just by suppressing it, and
call this out of scope.
@ParaskP7
Analysis: Add missing n-a to terms model
f902b5e 
FYI: 'n-a' stands for 'nullability annotations'.
@ParaskP7
Refactor: Remove test only taxonomy store methods
0d408d3 
None of the 'instantiateTermModel(...)' related 'TaxonomyStore' methods
are being in production and are only being used either (unnecessarily)
on the demo 'example' app or 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.
@ParaskP7
Analysis: Add default and non-default constructors for taxonomy model
fdbcdc0 
This is done so that clients of that model, clients like JP/WPAndroid,
when trying to manually instantiating such a model, they will always
use the non-default constructor, and always provide a 'name' argument.

The empty default constructor was added because '@table' related models
need to have one such constructor. Otherwise, its 'Mapper' related
auto-generated class would fail to compile, in this case the
'TaxonomyModelMapper.java' class.

Also, a full-arguments constructor was added, which should be the
preferred and goto constructor for most cases.

FYI: The trick I used to first '@deprecated' this constructor is to help
clients avoid using this constructor in the future, that is, when they
manually construct one. Suppressing the 'DeprecatedIsStillUsed' warning
was necessary because otherwise, even if every occurrence in the
codebase is being switched to the non-default constructor, the
auto-generated 'Mapper' related class will anyway use that default
constructor, thus this warning will persist no matter what.

PS.1: Note that on the 1-argument constructor an extra Javadoc is added
to help clients understand when and why to use it. For all other cases,
using the full-arguments constructor should be the preferred and goto
constructor.
PS.2: Note that although this 'TaxonomyModel' model class is a '@table'
related model, there is no associated 'CREATE TABLE TaxonomyModel (...)'
WellSQL table related initialization happening within the
'WellSqlConfig.kt' class and more specifically during 'onUpgrade(...)'.
This means that this '@table' annotation on this 'TaxonomyModel' model
class could be removed. Actually, since this model class is only used
for testing purposes, it can be completely removed.
@ParaskP7 ParaskP7 self-assigned this Sep 8, 2023
@ParaskP7 ParaskP7 changed the title [Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All Taxonomy SqlUtils & Model Classes (breaking change) [Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All Taxonomy & Term SqlUtils & Model Classes (breaking change) Sep 8, 2023
@ParaskP7 ParaskP7 changed the title [Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All Taxonomy & Term SqlUtils & Model Classes (breaking change) [Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All Taxonomy & Term SqlUtils & Model Classes (breaking) Sep 8, 2023
@ParaskP7
Analysis: Add default and non-default constructors for term model
11b5817 
This is done so that clients of that model, clients like JP/WPAndroid,
when trying to manually instantiating such a model, they will always
use the non-default constructor, and thus always provide at least a
'taxonomy' argument.

The empty default constructor was added because '@table' related models
need to have one such constructor. Otherwise, its 'Mapper' related
auto-generated class would fail to compile, in this case the
'TermModelMapper.java' class.

Also, a full-arguments constructor was added, which should be the
preferred and goto constructor for most cases.

FYI: The trick I used to first '@deprecated' this constructor is to help
clients avoid using this constructor in the future, that is, when they
manually construct one. Suppressing the 'DeprecatedIsStillUsed' warning
was necessary because otherwise, even if every occurrence in the
codebase is being switched to the non-default constructor, the
auto-generated 'Mapper' related class will anyway use that default
constructor, thus this warning will persist no matter what.

PS: Note that on both, the 1-argument constructor and the 3-arguments
constructors, an extra Javadoc is added to help clients understand
when and why to use each. For all other cases, using the full-arguments
constructor should be the preferred and goto constructor.
@ParaskP7 ParaskP7 force-pushed the analysis/add-missing-nullability-annotations-to-all-taxonomy-sqlutils-model-classes branch from f98cc52 to 11b5817 Compare September 11, 2023 10:59
@ParaskP7 ParaskP7 mentioned this pull request Sep 11, 2023
Merged
16 tasks
@ParaskP7 ParaskP7 requested a review from mkevins September 11, 2023 12:14
@ParaskP7 ParaskP7 marked this pull request as ready for review September 11, 2023 12:14
@ParaskP7
Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-FluxC-A…
22e6a84 
…ndroid into analysis/add-missing-nullability-annotations-to-all-taxonomy-sqlutils-model-classes
@ParaskP7 ParaskP7 requested review from AjeshRPai and removed request for mkevins October 2, 2023 14:28
@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Oct 2, 2023

👋 @mkevins I've now unassigned you from this and assigned @AjeshRPai instead. It seems that you might be busy atm and I don't want to hold this PR any longer. 🙏

@ParaskP7
Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-FluxC-A…
a32ed9f 
…ndroid into analysis/add-missing-nullability-annotations-to-all-taxonomy-sqlutils-model-classes
AjeshRPai
AjeshRPai approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@AjeshRPai AjeshRPai left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @ParaskP7

I have gone through the code changes and smoke tested the app by the following user flows

  1. Category - Add default Category, Add under a category , Edit and Delete 🟢
  2. Tags - Add/Edit and Delete 🟢
  3. Adding category during Prepublishing a Post and setting tags during pre-publishing of story post 🟢

I am approving but not merging this PR since the fluxC PR has to be merged and then the version needs to be updated in the build.gradle file

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Oct 4, 2023

👋 @AjeshRPai and thank you so much for reviewing and testing this, you rock! 🙇 ❤️ 🚀

I am approving but not merging this PR since the fluxC PR has to be merged and then the version needs to be updated in the build.gradle file

Not to worry and thanks, I'll proceed doing the merge dance myself! 💯

@ParaskP7 ParaskP7 merged commit 55122d0 into trunk Oct 4, 2023
@ParaskP7 ParaskP7 deleted the analysis/add-missing-nullability-annotations-to-all-taxonomy-sqlutils-model-classes branch October 4, 2023 09:33
@ParaskP7 ParaskP7 mentioned this pull request Oct 16, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AjeshRPai AjeshRPai AjeshRPai approved these changes

Assignees

@ParaskP7 ParaskP7

Labels
Core Tech Debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nullability Annotations to Java Classes - SqlUtils & Model - Taxonomy & Term
2 participants
@ParaskP7 @AjeshRPai