Fix Custom Fields Handling in Storage for both Orders and Products #14111

pmusolino · 2024-10-08T16:14:39Z

Description

It has been reported that for some users orders does not load peaMlT-Wz. It seems that this can happen when a store has a lot of custom fields for products, so upserting custom fields creates a deadlock on the writer context, delaying the syncing of orders for around 2 minutes.
This update optimizes the handling of custom fields in both OrdersUpsertUseCase and ProductStore. The changes improve performance by streamlining the logic for managing custom fields, introducing batch processing, and eliminating redundant operations.

Summary of changes

OrdersUpsertUseCase
- Removed checks for the existence of custom fields in storageOrder that are not present in readOnlyOrder, directly removing them to simplify the logic.
- Converted the creation of new custom fields from a loop with appends to a map for efficiency.
- Implemented batch writing for new custom fields to reduce database calls.
ProductStore
- Improved readability and indentation for better code maintenance.
- Simplified the logic for removing custom fields that are not in readOnlyProduct by directly using a set lookup.
- Removed redundant operations for managing custom fields.
- Introduced batch processing for adding new custom fields, thereby optimizing database interactions.

Steps to reproduce

Login to a store with a lot of products and orders, with a lot of custom fields for each one (more than 100).
Notice that Order List is not loading, if not after a lot of time.
Make sure that in the product detail, the list of custom fields will be loaded correctly (cc @hafizrahman).

Testing information

Tested that in a store with a lot of custom fields for products and orders, and everything loads properly.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

…ields` - Update comment to clarify the removal process of custom fields in `handleProductCustomFields`

…ore.swift` - Creates a new array to store new metadata objects before adding to `storageProduct`

…ad-only product - Update logic to only handle new custom fields from read-only product

- Streamline logic for removing custom fields that are not in read-only product

- Optimize custom fields handling in OrdersUpsertUseCase.swift - Replace forEach with a set lookup to improve performance - Introduce batch writing for new custom fields to reduce database calls

dangermattic · 2024-10-08T16:15:11Z

	1 Warning
⚠️	This PR is assigned to the milestone 20.6 ❄️. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

	1 Message
📖	This PR contains changes to `RELEASE-NOTES.txt`. Note that these changes won't affect the final version of the release notes as this version is in code freeze. Please, get in touch with a release manager if you want to update the final release notes.

Generated by 🚫 Danger

Yosemite/Yosemite/Stores/Order/OrdersUpsertUseCase.swift

wpmobilebot · 2024-10-08T16:27:28Z

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

	App Name	WooCommerce iOS
	Build Number	pr14111-161f7cd
	Version	20.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`161f7cd`
	App Center Build	WooCommerce - Prototype Builds #11120

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

selanthiraiyan

Thanks for working on this, Paolo!

I tested this on a store with a large number of custom fields and I validated that orders are loaded as expected. ✅

I will wait for @hafizrahman to test that custom fields are displayed without issues.

Yosemite/Yosemite/Stores/Order/OrdersUpsertUseCase.swift

selanthiraiyan · 2024-10-08T16:59:29Z

Yosemite/Yosemite/Stores/Order/OrdersUpsertUseCase.swift

+            return newStorageMetaData
+        }
+
+        // Batch writing process of multiple custom fields


Yosemite/Yosemite/Stores/ProductStore.swift

selanthiraiyan · 2024-10-08T17:02:35Z

Yosemite/Yosemite/Stores/Order/OrdersUpsertUseCase.swift

+            storage.deleteObject(storageCustomField)
+        }
+
+        // Create `customFields` objects from the `readOnlyOrder`


I think we should unit tests to OrdersUpsertUseCaseTests to validate that it can handle a large number of MetaData items. Right now, I see only tests that add/remove 1 custom field. What do you think?

Implemented here dd9ca23

selanthiraiyan · 2024-10-08T17:03:44Z

Yosemite/Yosemite/Stores/ProductStore.swift

+            storage.deleteObject(storageCustomField)
+        }
+
+        // Create `customFields` objects from the `readOnlyProduct`


Similar to the other comment. I think we should unit tests to validate that it can handle a large number of MetaData items.

The test implemented here dd9ca23 should work also for Products

hafizrahman · 2024-10-08T19:39:50Z

Make sure that in the product detail, the list of custom fields will be loaded correctly (cc @hafizrahman).

At this point in this branch, the Product Details is not able to actually show custom fields yet (only shows entry point), with the logic to show them is in a different branch in #14068

However, I was able to test this by putting a breakpoint inside ProductFormViewController. For each time I'm entering a Product Detail, I can confirm that p viewModel.productModel shows the content of the product and it includes its customFields property properly. Since it is what's used to display the content, I'm fairly confident there's no issue here.

Update: additionally, loading the product list screen itself also works fine. This is tested in both iPad and iPhone in iOS 17.5 Simulator with a site with many orders and products, where the products have many custom fields.

selanthiraiyan · 2024-10-08T23:34:16Z

@pmusolino Please update the PR description to close the issue #14106

…ducts - Modify `OrdersUpsertUseCaseTests.swift` to include a new test case for processing 5000 custom fields without deadlock.

pmusolino · 2024-10-08T23:41:50Z

@selanthiraiyan ready for another check

jaclync · 2024-10-09T04:35:34Z

Yosemite/Yosemite/Stores/Order/OrdersUpsertUseCase.swift

+
+        // Batch writing process of multiple custom fields
+        if !newStorageMetaDataArray.isEmpty {
+            storageOrder.addToCustomFields(NSSet(array: newStorageMetaDataArray))


@pmusolino Just sharing an observation from some debugging - this line caused the upsert to be triggered twice, with newStorageMetaDataArray containing a different custom field object (different Core Data object ID) each time. It's the expected result of the updated approach in this PR, with pre-existing custom fields being deleted and new custom fields re-added.

Thanks, Jaclyn!

From the comment of the failing unit test I wonder whether the updated approach of deleting all the stored custom fields before adding new custom fields will cause any reload or UI issues in screens where we use EntityListener to listen to changes to product or order entities. 🤔

selanthiraiyan

Thanks a lot for working on this @pmusolino and @itsmeichigo! I appreciate the efforts. 🙇

With the new approach of deleting all the MetaData items from Storage we are not doing the "upsert" operation of orders and products. i.e. We are not following the "upsert" behaviour, which is to check the existing items from core data and update them. Instead, we delete all the MetaData items and add them again regardless of whether something changed.

I think we should look for a different solution for the following reasons.

The existing method names and comments expect an upsert operation instead of deleting everything. This is misleading.
Core data will consider that the entity is changed even if we write the same order or product into the storage again. (which triggers callbacks and UI reloads) This is not expected from an "upsert" process.

Unfortunately, I don't have any suggestions currently.

- Update `OrdersUpsertUseCase.swift` to upsert custom fields from `readOnlyOrder`. - Update `ProductStore.swift` to upsert custom fields from `readOnlyProduct`. - Remove redundant custom fields from both `storageOrder` and `storageProduct` based on `readOnlyOrder` and `readOnlyProduct`.

pmusolino · 2024-10-09T17:54:37Z

@selanthiraiyan I have pushed new changes here: 3cc83fd. In this update, MetaData items will not be deleted; instead, there will be just the usual upsert. As a result, the previous unit test in OrderStoreTests will no longer fail. These new changes offer blazing fast performance compared to the previous approach while obtaining the same result. It's just an improvement on how metadata are handled in the main cycle. With these changes, I am no longer able to replicate the issue, and the unit test implemented in this PR still works as expected.
Please take a second look 🙌

selanthiraiyan

This looks great! 👏

I tested it using the store with issues, and the orders load as expected.

selanthiraiyan · 2024-10-09T17:59:50Z

Yosemite/YosemiteTests/Stores/Order/OrdersUpsertUseCaseTests.swift

+        DispatchQueue.global(qos: .background).async {
+            orderUseCase.upsert([order])
+            productStore.upsertStoredProducts(readOnlyProducts: [product], in: backgroundContext)
+        backgroundContext.saveIfNeeded()


nit: Indentation update needed.

pmusolino and others added 6 commits October 8, 2024 18:11

- Add a set of metadata IDs for quick lookup in `handleProductCustomF…

3f7ab1d

…ields` - Update comment to clarify the removal process of custom fields in `handleProductCustomFields`

- Adds batch writing process for multiple custom fields in `ProductSt…

8a2e6f2

…ore.swift` - Creates a new array to store new metadata objects before adding to `storageProduct`

- Remove redundant removal of storage custom fields not present in re…

ea1ea13

…ad-only product - Update logic to only handle new custom fields from read-only product

- Improve indentation and readability in ProductStore.swift

da14c25

- Streamline logic for removing custom fields that are not in read-only product

Refactor Order Custom Fields Handling in OrdersUpsertUseCase

5d765a9

- Optimize custom fields handling in OrdersUpsertUseCase.swift - Replace forEach with a set lookup to improve performance - Introduce batch writing for new custom fields to reduce database calls

Remove all stored custom fields for

0bfa18a

pmusolino added type: bug A confirmed bug. feature: order list Related to the order list. feature: product list Related to the product list. impact: high Impacts a lot of users as reported by our Happiness Engineers. labels Oct 8, 2024

pmusolino added this to the 20.6 ❄️ milestone Oct 8, 2024

pmusolino requested review from hafizrahman, selanthiraiyan, iamgabrielma and itsmeichigo October 8, 2024 16:14

update: release-notes 20.6

1ed6186

itsmeichigo reviewed Oct 8, 2024

View reviewed changes

Yosemite/Yosemite/Stores/Order/OrdersUpsertUseCase.swift Outdated Show resolved Hide resolved

fix: comments

9d34b5c

selanthiraiyan reviewed Oct 8, 2024

View reviewed changes

cleanup unused code

d51ee59

selanthiraiyan self-assigned this Oct 8, 2024

pmusolino added 2 commits October 9, 2024 01:38

Add test for handling large number of custom fields in orders and pro…

dd9ca23

…ducts - Modify `OrdersUpsertUseCaseTests.swift` to include a new test case for processing 5000 custom fields without deadlock.

update: removed extra space

5c03e78

pmusolino requested a review from selanthiraiyan October 8, 2024 23:41

pmusolino enabled auto-merge October 8, 2024 23:47

jaclync reviewed Oct 9, 2024

View reviewed changes

selanthiraiyan reviewed Oct 9, 2024

View reviewed changes

pmusolino disabled auto-merge October 9, 2024 14:35

pmusolino requested a review from selanthiraiyan October 9, 2024 17:54

selanthiraiyan approved these changes Oct 9, 2024

View reviewed changes

fix: indentation

161f7cd

pmusolino enabled auto-merge October 9, 2024 23:10

pmusolino merged commit 3077cf7 into release/20.6 Oct 9, 2024
14 checks passed

pmusolino deleted the fix/orders-do-not-load-because-of-deadlock-in-customfields-final branch October 9, 2024 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Custom Fields Handling in Storage for both Orders and Products #14111

Fix Custom Fields Handling in Storage for both Orders and Products #14111

pmusolino commented Oct 8, 2024 •

edited

Loading

dangermattic commented Oct 8, 2024 •

edited

Loading

wpmobilebot commented Oct 8, 2024 •

edited

Loading

selanthiraiyan left a comment

selanthiraiyan Oct 8, 2024

selanthiraiyan Oct 8, 2024

pmusolino Oct 8, 2024

selanthiraiyan Oct 8, 2024

pmusolino Oct 8, 2024

hafizrahman commented Oct 8, 2024 •

edited

Loading

selanthiraiyan commented Oct 8, 2024

pmusolino commented Oct 8, 2024

jaclync Oct 9, 2024

selanthiraiyan Oct 9, 2024 •

edited

Loading

selanthiraiyan left a comment

pmusolino commented Oct 9, 2024

selanthiraiyan left a comment

selanthiraiyan Oct 9, 2024

pmusolino Oct 9, 2024

Fix Custom Fields Handling in Storage for both Orders and Products #14111

Fix Custom Fields Handling in Storage for both Orders and Products #14111

Conversation

pmusolino commented Oct 8, 2024 • edited Loading

Description

Summary of changes

Steps to reproduce

Testing information

Reviewer (or Author, in the case of optional code reviews):

dangermattic commented Oct 8, 2024 • edited Loading

wpmobilebot commented Oct 8, 2024 • edited Loading

selanthiraiyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hafizrahman commented Oct 8, 2024 • edited Loading

selanthiraiyan commented Oct 8, 2024

pmusolino commented Oct 8, 2024

Choose a reason for hiding this comment

selanthiraiyan Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

selanthiraiyan left a comment

Choose a reason for hiding this comment

pmusolino commented Oct 9, 2024

selanthiraiyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmusolino commented Oct 8, 2024 •

edited

Loading

dangermattic commented Oct 8, 2024 •

edited

Loading

wpmobilebot commented Oct 8, 2024 •

edited

Loading

hafizrahman commented Oct 8, 2024 •

edited

Loading

selanthiraiyan Oct 9, 2024 •

edited

Loading