NullPointerException: site.username must not be null

[AliSoftware]

Sentry Url: https://sentry.io/share/issue/5e66a98a823848749661aef1379e07c3/
User Count: 24
Count: 28
First Release: 18.5
First Seen: 2021-11-03T20:43:31.742000Z
Last Seen: 2021-11-08T15:00:57Z
24 Hours: 7
30 Days: 28

NullPointerException: site.username must not be null
     at org.wordpress.android.fluxc.network.xmlrpc.comment.CommentsXMLRPCClient.fetchComment(CommentsXMLRPCClient.kt:136)
     at org.wordpress.android.fluxc.store.CommentsStore.fetchComment(CommentsStore.kt:152)
     at org.wordpress.android.fluxc.store.CommentsStore.onFetchComment(CommentsStore.kt:564)
     at org.wordpress.android.fluxc.store.CommentsStore$onAction$2.invokeSuspend(CommentsStore.kt:497)
     at org.wordpress.android.fluxc.store.CommentsStore$onAction$2.invoke(null:10)
     at org.wordpress.android.fluxc.tools.CoroutineEngine$launch$1.invokeSuspend(CoroutineEngine.kt:62)
...
(3 additional frame(s) were not displayed)

Relatively low numbers but rising quickly and brand new in 18.5. so likely to continue rising.

[jd-alexander]

Thanks for reporting @AliSoftware I will check this out.

[jd-alexander]

Hi @develric 👋 Can you please take a look at this? Pinging you since you’ve worked on this in the last couple of months 🙇🏾

[peril-wordpress-mobile]

	Fails
🚫	Please add a feature label to this issue. e.g. 'Stats'

Generated by 🚫 dangerJS

[peril-wordpress-mobile]

	Fails
🚫	Please add a feature label to this issue. e.g. 'Stats'

Generated by 🚫 dangerJS

[develric]

Hey @jd-alexander , @AliSoftware 👋 ; I had a first pass on this but could not find any obvious path to replicate 🤔 . Also the CommentsXMLRPCClient is there from before the 18.5 so maybe there is something elsewhere that was introduced later that I'm not getting. I will try to get back to this a bit more tomorrow 🙇 .

[jd-alexander]

Hey @jd-alexander , @AliSoftware 👋 ; I had a first pass on this but could not find any obvious path to replicate 🤔 . Also the CommentsXMLRPCClient is there from before the 18.5 so maybe there is something elsewhere that was introduced later that I'm not getting. I will try to get back to this a bit more tomorrow 🙇 .

Thanks for chiming in @develric here's a PR I stumbled on that includes interactions with the CommentsStore but I am not seeing where it triggers fetchComment so your investigation into this will be helpful.

[develric]

Hey 👋

here's a PR I stumbled on that includes interactions with the CommentsStore but I am not seeing where it triggers fetchComment

Yep and that one was targeting 18.4 that is previous of the events we are seeing, so AFAIU it should not be directly related (unless sentry grouping is making fun of us of course 🙃).

A couple of notes I can share:

• AFAIU the username for a site is actually null for a .COM/JP site (looking into what we store in SiteModel fluxc table), but should (in principle) not be null for self-hosted sites (looking at that same table)
• The CommentsXMLRPCClient client should be used for self-hosted not JP connected sites and I was not able to get a null username in this scenario

One thing I was exploring (props to @khaykov for pointing out) is the case where a self-hosted site could use a not full JP connection but a minimum set based on Jetpack Connection Package. That case seems a bit in the middle between a .COM/JP and a self-hosted so not fully sure how the app logic would manage it. I'm not at all sure this is related in the end, but I was running out of time setting up a test server for that (not familiar with it so not sure how long that could take tbh).

@jd-alexander , @AliSoftware (and finger crossed while I say this with a super low voice 😅 🤞) since the numbers seem keeping low, I was wondering if it could make sense to add this to GK (maybe after monitoring some more days). Wdyt? Happy to follow up with you next week. Have a great we 🙇 !

[AliSoftware]

Yep wfm. Tbh I generally don't report Sentry issues with such low numbers anyway, just did it this time because I thought a NPE on something that got just introduced in latest release would have been an easy win… but apparently it's more tricky than expected, so fine by me to move to GK and keep an eye out.

Worth noting though that there are a couple of other site.username must not be null issues in Sentry, even if with a slightly different callstack (all related to comments fetching though). Worth keeping an eye on them too.

• For example this one happening on fetchCommentsPage is at 74 events / 39 users
• For reference, Sentry numbers on the one originally posted in this GH issue is now at 66 events / 46 users.

---

PS / random idea: Could it be that the backend API changed recently and introduced the bug from the backend response format – now sometimes returning nil for that field while it didn't before – rather than a change in our frontend code? 🤔

[jd-alexander]

Jetpack Connection Package

Hey 👋

here's a PR I stumbled on that includes interactions with the CommentsStore but I am not seeing where it triggers fetchComment

Yep and that one was targeting 18.4 that is previous of the events we are seeing, so AFAIU it should not be directly related (unless sentry grouping is making fun of us of course 🙃).

Ah, thanks for sharing!

A couple of notes I can share:

• AFAIU the username for a site is actually null for a .COM/JP site (looking into what we store in SiteModel fluxc table), but should (in principle) not be null for self-hosted sites (looking at that same table)
  Understandable.

• The CommentsXMLRPCClient client should be used for self-hosted not JP connected sites and I was not able to get a null username in this scenario

Makes sense!

One thing I was exploring (props to @khaykov for pointing out) is the case where a self-hosted site could use a not full JP connection but a minimum set based on Jetpack Connection Package. That case seems a bit in the middle between a .COM/JP and a self-hosted so not fully sure how the app logic would manage it. I'm not at all sure this is related in the end, but I was running out of time setting up a test server for that (not familiar with it so not sure how long that could take tbh).

No problem. These insights will help in getting the issue resolved.

@jd-alexander , @AliSoftware (and finger crossed while I say this with a super low voice 😅 🤞) since the numbers seem keeping low, I was wondering if it could make sense to add this to GK (maybe after monitoring some more days). Wdyt? Happy to follow up with you next week. Have a great we 🙇 !

Indeed, I agree that we can add it to groundskeeping since some digging is needed to verify the user flow that is actually causing the issue. I agree with @AliSoftware that we should keep an eye on it, so if necessary we can prioritize and address it sooner.

[khaykov]

I think I managed to get to the bottom of this one but unfortunately run out of time to fix it.
The crash happens when opening a Comments List on self-hosted sites that are using the Jetpack Connection Plugin and never had Jetpack installed (this is important, pressable sites will not work!). Sites have a Jetpack CP when they have certain plugins installed that require connection to wpcom (WCPay or Jetpack Backup).

Sites with Jetpack CP are returned with the list of wpcom sites, but they are not actually wpcom sites, and since they don't have credentials you can't use them with XML-RPC endpoints either (that's why the crash is happening).

While you can change the logic in SiteMode so the isUsingWpComRestApi() will return true for Jetpack CP sites, most of the functionality of the app will be broken (interestingly, comments will work!).

I think that the appropriate course of action (at least for now) will be to exclude Jetpack CP sites from the fetch sites response of SiteRestClient. They should be added as self-hosted sites.

Thanks to @hichamboushaba for confirming the issue and for the help with the investigation.

[develric]

Just noticed this PR was automatically closed by this Fluxc PR. I'm re-opening it since the full fix needs also this WPAndroid PR and this Login lib PR merged. Also, I'm asking to consider a Woo PR update before to merge everything since the login lib changes can be relevant to Woo as well (see notes in the linked login lib PR).