Order relationship meta data isn't filtered from the Subscription objects when HPOS is enabled.

[prettyboymp]

Describe the bug

The wcs_subscription_data_store_props_to_ignore filter is only run for the WCS_Subscription_Data_Store_CPT, This filter is used by the WCS_Related_Order_Store_Cached_CPT class to add meta_keys that the data store shouldn't apply to the data object. When HPOS is enabled, this meta data isn't filtered out and is included in the meta applied to the object when loaded.

Is there still a strong reason to hide this meta value from the object or avoid adding it to the $internal_meta_keys?

To Reproduce

This was found while converting tests to run with HPOS enabled. It broke the following tests because the loaded copy of the subscriptions had the _subscription_renewal_order_ids_cache meta_data loaded, while the created version of the subscription object did not.

• WCS_Functions_Test::test_wcs_get_subscription
• WCS_Functions_Test::test_5_wcs_get_subscriptions

Run either of these with HPOS enabled, e.g.:

HPOS=1 ./vendor/bin/phpunit --filter= WCS_Functions_Test::test_wcs_get_subscription

[thenbrent]

Is there still a strong reason to hide this meta value from the object or avoid adding it to the $internal_meta_keys?

The answer is yes.

The reason WCS_Related_Order_Store_Cached_CPT didn't add its meta data to a subscription originally was because of the expectation that one day we'd have HPOS and that cache wouldn't be needed.

WCS_Related_Order_Store_Cached_CPT was really just a workaround for the CPT data store to provide a performant way to query related order data for a subscription until something like HPOS was available which allowed that query to be done at the database level efficiently.

Without being able to recall exactly how WCS_Related_Order_Store_Cached_CPT is used across all of WC Subscriptions, it is possible that the right approach here is not to ensure WCS_Related_Order_Store_Cached_CPT can filter its meta_keys when a HPOS data store is enabled, but instead to go so far as to have WCS_Related_Order_Store_Cached_CPT not loaded at all when HPOS is enabled.

[prettyboymp]

WCS_Related_Order_Store_Cached_CPT was really just a workaround for the CPT data store to provide a performant way to query related order data for a subscription until something like HPOS was available which allowed that query to be done at the database level efficiently.

The way the WCS_Related_Order_Store_Cached_CPT is used to make sure that both sides of the relationship end up in meta so that we don't end up needing to run a really slow meta query against meta to find all the related orders.

https://github.com/Automattic/woocommerce-subscriptions-core/blob/f05ef2f8fa0d5fe24069311b3838b31ec0ae95cf/includes/data-stores/class-wcs-related-order-store-cpt.php#L42-L60

I don't think the way HPOS tables are implemented help the performance here. We don't have a separate storage outside of order meta to store the relationships, so we would still need the cache implementation. The only way to get around it would be to add a separate table specifically for order relationships that allowed more efficient indexing.

[thenbrent]

The only way to get around it would be to add a separate table specifically for order relationships that allowed more efficient indexing.

Ah yes, thanks for reminding me (going back to ancient history in my memory here).

The ideal end state of subscriptions moving to a order specific scheme was to have a new table for storing order relationships and move away from meta. I can understand why that didn't happen with the Subscriptions HPOS work given the urgency on it and all the various other higher priority items around WC Subscriptions and payments though and for the same reasons can't see it happening in future so it seems like we're left with meta until it's a problem (I don't think WC Subscriptions has had perf issues or other problems that would lead us to proiritize moving away from meta and WCS_Related_Order_Store_Cached_CPT).

Coming back to the original question, I don't know of a strong reason to hide this meta value from the subscription object other than the points @james-allan made via Slack:

I haven’t done a deep dive to find the original reason but I suspect it was because when the WC core datastores were introduced, letting the datastore handle this meta led to issues because of the way internal meta keys have to have publicly defined setters & getters and the core data stores only save when the subscription is saved whereas we wanted our own datastore to handle it immediately. I can only vaguely recall the details, but we ran into some issues while working on HPOS support because the core WC datastore was overriding previously set subscription order cache data with old values because of race conditions and multiple instances of the same subscription existing throughout the request etc. Our related order caches sit separate to the order/subscription datastore and are saved immediately to the database and to achieve that we had to make it separate to the core object datastore.

The WC CRUD classes do create a lot of opportunities for race conditions that can be really hard to trace (especially on merchant sites with limited access), so keeping a cache up-to-date and accurate is much easier and better handled outside the objects' memory.