Strip product metadata #2756
Strip product metadata #2756
Conversation
| operator fun invoke(metadata: String?): String { | ||
| if (metadata.isNullOrEmpty()) return EMPTY_JSON_ARRAY | ||
|
|
||
| return gson.fromJson(metadata, JsonArray::class.java) | ||
| .mapNotNull { it as? JsonObject } | ||
| .asSequence() | ||
| .filter { jsonObject -> | ||
| val isNullOrEmpty = jsonObject[WCMetaData.VALUE].isElementNullOrEmpty() | ||
| jsonObject[WCMetaData.KEY]?.asString.orEmpty() in SUPPORTED_KEYS && isNullOrEmpty.not() | ||
| }.toList() | ||
| .takeIf { it.isNotEmpty() } | ||
| ?.let { gson.toJson(it) } ?: EMPTY_JSON_ARRAY | ||
| } |
There was a problem hiding this comment.
Just out of curiosity, I remember reviewing a code like this in the past. Did we implement a metadata strip before that got removed? I'm not finding anything like this in the trunk code base 😅
There was a problem hiding this comment.
I remember reviewing a code like this in the past.
Yes, but it was for product variations, not products. This is the strip use case class StripProductVariationMetaData
| @@ -1486,6 +1497,7 @@ class ProductRestClient @Inject constructor( | |||
| val newModel = product.asProductModel().apply { | |||
| id = product.id?.toInt() ?: 0 | |||
| localSiteId = site.id | |||
| metadata = stripProductMetaData(metadata) | |||
There was a problem hiding this comment.
I I was wondering if maybe the stripProductMetaData use case could be called inside the asProductModel() transformation since all data strip happens right after it for every scenario. It could also be easier to maintain and enforce the metadata removal for future implementations.
But I also understand that since the stripping happens through a use case, the dependency injection will get weird and we would have to create the use case object manually, so if you think it's not worth the effort, it's OK too. I'm only concerned with making sure that we won't easily forget to strip that metadata in future changes.
WDYT?
There was a problem hiding this comment.
I'm only concerned with making sure that we won't easily forget to strip that metadata in future changes.
Good point @ThomazFB. ☝️
But I also understand that since the stripping happens through a use case, the dependency injection will get weird and we would have to create the use case object manually
That was also why I avoided adding the use case manually. I decided to go this way because I saw that we also had this line localSiteId = site.id on every call, adding the site id to the product model (so, we always pre-process the product before saving it to the database).
That said, I believe we can do better, so I'll add a task to refactor the asProductModel() and create a map class that can handle the injection of the use case. I will tackle this in a different PR.
Closes 9218
Why
Moving big chunks of data can impact the app's performance. This PR is part of the optimization we are working on as part of the Reliability project to improve the app's performance perception.
Description
This PR adds the
StripProductMetaDataclass to remove all unused fields from the product's metadata. Unused metadata fields can be huge, and removing them will result in better use of the device's resources, improving the app's performance and preventing OOM errors.Testing instructions
Test it with composite builds and, using App inspection or Flipper, check that only StripProductMetaData.SUPPORTED_KEYS are saved in the DB