Ensure getCanBlaze is non nullable even when canBlaze is not set

[JorgeMucientes]

Fixes WOOMOB-1442

Description

I couldn't reproduce this crash, but the fix ensures mCanBlaze is never null.
I've attempted migrating the field from Boolean to primitive value boolean as suggested in Linear issue, but it does require migration. So the simplest approach was to handle nullability at getCanBlaze() function level.

Steps to reproduce

Couldn't reproduce this either.

Testing information

1. Switch back and forth between 2 sites, one where Blaze is enabled and the other where Blaze is disabled. Check that the feature is shown or hidden accordingly.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	27782cb
	Direct Download	woocommerce-wear-prototype-build-pr14730-27782cb.apk

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	27782cb
	Direct Download	woocommerce-prototype-build-pr14730-27782cb.apk

[wpmobilebot]

🤖 Test Failure Analysis

Your tests failed. Claude has analyzed the failures - check the annotation for details.

[hichamboushaba]

Thanks @JorgeMucientes, but the change doesn't fix the issue, please check my comment below.

I've attempted migrating the field from Boolean to primitive value boolean as suggested in Linear issue, but it does require migration

Just confirming here, what error did you encounter after updating the type?
On my side, I've tried changing the type to primitive, and it didn't seem to need a migration. After the change, I faced a crash:

java.lang.NoSuchMethodError: No virtual method getCanBlaze()Ljava/lang/Boolean; in class Lorg/wordpress/android/fluxc/model/SiteModel; or its super classes (declaration of 'org.wordpress.android.fluxc.model.SiteModel' appears in /data/data/com.woocommerce.android.dev/code_cache/.overlay/base.apk/classes3.dex)
                                                                                                    	at com.woocommerce.android.ui.blaze.IsBlazeEnabled.invoke(IsBlazeEnabled.kt:25)
...

But this was probably just because of a bad cache, because after using ./gradlew :WooCommerce:assembleWasabiDebug --rerun-tasks, the issue was fixed, and I was able to run the app without issues.

I'm just sharing this to discuss, using the primitive type is cleaner, but if we are worried about any side effects (I tried forcing NULL value for the field, and it seems WellSql handled the change gracefully, but I didn't check enough to be confident), applying the fix on getCanBlaze will work too.

[hichamboushaba]

This is not enough for the fix, we just moved the NPE exception here instead of IsBlazeEnabled.

When unboxing from the Boolean type to the primitive type, we'll get an NPE, and to reproduce it just comment the this.mCanBlaze = canBlaze; in the line 1117 to make sure we keep the default value (simulating what would happen if options is missing from /me/sites response), the app will crash with the following exception:

java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.Boolean.booleanValue()' on a null object reference
                                                                                                    	at org.wordpress.android.fluxc.model.SiteModel.getCanBlaze(SiteModel.java:1113)
                                                                                                    	at com.woocommerce.android.ui.blaze.IsBlazeEnabled.invoke(IsBlazeEnabled.kt:25)

If we want to keep the Boolean class as type, we need to improve this check to something like:

Suggested change

	return mCanBlaze == true;
	return mCanBlaze != null && mCanBlaze;

[hichamboushaba]

Another point @JorgeMucientes here, I think we might consider a beta fix with this given the number of crashes, WDYT?

[JorgeMucientes]

Thanks for reviewing this closely @hichamboushaba

When unboxing from the Boolean type to the primitive type, we'll get an NPE, and to reproduce it just comment the this.mCanBlaze = canBlaze;

Big 🤦🏼 . You are totally right. I wrongly assumed the unboxing would simply return the null value. But the unboxing was the root cause of the crash in the first place.

Just confirming here, what error did you encounter after updating the type?

The exact same crash you shared. And I wrongly concluded it was due to the type change instead of checking that the actual reason was outdated generated function signatures. I actually ran a clean build (which should've recompiled everything) and still got the error.

I'm just sharing this to discuss, using the primitive type is cleaner, but if we are worried about any side effects (I tried forcing NULL value for the field, and it seems WellSql handled the change gracefully, but I didn't check enough to be confident), applying the fix on getCanBlaze will work too.

Yes, I agree it's cleaner, and I think we should aim for that solution. Looks safe enough, worst case scenario, if we missed some corner case for users that currently have NULL persisted, then it will crash only for them, which is already crashing. So we won't make things worse, but we will not fix the problem for users already facing this crash.
Curious how you tested the forcing of NULL. Did you just edit the DB value from the AS app inspector? It's what I tested and worked well, DB value is always returned as long value 0 even when set to NULL. But I'm curious whether you tested this differently.

Another point @JorgeMucientes here, I think we might consider a beta fix with this given the number of crashes, WDYT?

Yeah, good point, given this started happening couple days ago in the latest release its probably worth fixing in beta. I'll move the changes to the release branch.

[JorgeMucientes]

Closing in favor of applying the fix to release/23.4 branch: #14733

[hichamboushaba]

Curious how you tested the forcing of NULL. Did you just edit the DB value from the AS app inspector? It's what I tested and worked well, DB value is always returned as long value 0 even when set to NULL. But I'm curious whether you tested this differently.

Yes, that's exactly how I tested it too.