NPE empty attributes #2752
NPE empty attributes #2752
Conversation
| * Returns the list of products attributes. The function returns an empty list | ||
| * when the attributes json deserialization fails. | ||
| */ | ||
| @Suppress("SwallowedException", "TooGenericExceptionCaught") |
There was a problem hiding this comment.
np: I think you don't need the "SwallowedException" supression here.
| @@ -347,7 +352,7 @@ data class WCProductModel(@PrimaryKey @Column private var id: Int = 0) : Identif | |||
| ) | |||
| } | |||
| } | |||
| } catch (e: JsonParseException) { | |||
| } catch (e: Exception) { | |||
There was a problem hiding this comment.
There's a way to refactor this while keeping the generic catch, avoiding the TooGenericExceptionCaught supression, and improve the overall performance of the operation while keeping it idiomatic:
return kotlin.runCatching {
Gson().fromJson(attributes, JsonElement::class.java)
.asJsonArray.asSequence()
.map { it.asJsonObject }
.map { json ->
ProductAttribute(
id = json.getLong("id"),
name = json.getString("name") ?: "",
variation = json.getBoolean("variation", true),
visible = json.getBoolean("visible", true),
options = getAttributeOptions(json.getAsJsonArray("options"))
)
}.toList()
}.fold(
onSuccess = { it },
onFailure = { emptyList() }
)
By using the runCatching, we make sure that every single Throwable coming from that block will trigger the onFailure, allowing us to remove all suppression from the top of the function and making it clear what to do on each scenario through the result folding.
WDYT? Makes sense?
| import org.assertj.core.api.Assertions | ||
| import org.junit.Test | ||
|
|
||
| class WCProductModelTest { |
There was a problem hiding this comment.
Great work adding unit tests!
|
Thank you for moving forward with my suggestion, @atorresveiga! Looks good! |
Closes: woocommerce/woocommerce-android#9160
Closes: woocommerce/woocommerce-android#9162
Description
This PR solves the NPE crash in the app when receiving unexpected values in the attributes field. We previously caught only JsonParseException parsing exceptions, and now we are swallowing all types of exceptions. Now, we return an empty list whenever we fail to parse the product attributes (the same behavior we had before but only for JsonParseException).
Here p1686673876637129-slack-CGPNUU63E, we discussed about the WCProductModel refactor to be able to unit tests the attributes deserialization.
Testing
Check that exceptions deserializing product's attributes don't crash the app