Adding OrderTableDatastore and Meta caching integration

[prettyboymp]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Closes #45550.

Background:
The current OrderCaching works by attempting to cache the Order objects. However, the current implementation of WC_Data::__sleep()/__wakeup() only allow the ID to be serialized. The reason for this was to avoid serializing the datastore with a data object. The end result means that any unserialized WC_Data object has to be reinitialized completely, requiring DB queries to retrieve the data.

This PR introduces caching of order data into the HPOS DataStores layer so that the DB data is cached reducing the queries to initialize order objects.

Benefits:

1. An entity doesn't need to care where it is stored (e.g. which datastore) or about it's own caching. So we can remove some of the work-arounds added to deal with cache when syncing.
2. Caching a set of Orders continues to only cache the set of IDs instead of the entire object. This ensures that the orders restored from the set are the latest versions of those orders.

Alternative Considered:
I looked at improving the serialization handling of WC_Data and it's child classes so that we could serialize all the relevant data with the entities without connected services, like the data stores. There were a couple issues with this:

First, PHP limitations meant that the serialization/deserialization would need to be pretty complex: PHP doesn't have a simple way to use the default serialization and just exclude certain properties. To properly restore an object, each class in the hierarchy would need to potentially deal with any private properties it defines. With extended classes from other plugins, those child classes would need to be properly modified for this to work or compatibility would break.

Second, there is already a bit of juggling going on around caching orders because the data stores have accessible public API's for making updates. Shifting the caching to them allows them to directly control the cache and update it when needed.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Unit tests have been updated to validate proper cache invalidation.

The following can be used to test the functionality of the OrdersTableDataStore caching. :

1. Create a file named PR46023Tests.php somewhere within the plugins/woocommerce/tests/php/src diretory.

<?php

/**
 * Test for https://github.com/woocommerce/woocommerce/pull/46023 to verify that the caching actually makes
 * performance improvements over the current cache handling by caching the order data not currently preserved
 * with the WC_Data objects.  It works by forcing cached objects/arrays to be serialized like they would be with a 
 * persistent cache solution. 
 */
class PR46023Tests extends WC_Unit_Test_Case {

	use \Automattic\WooCommerce\RestApi\UnitTests\HPOSToggleTrait;

	public function setUp(): void {
		parent::setUp();
		add_filter( 'wc_allow_changing_orders_storage_while_sync_is_pending', '__return_true' );
		$this->setup_cot();
		$GLOBALS['wp_object_cache'] = new Serialing_Cache_Proxy( $GLOBALS['wp_object_cache'] );
	}

	public function tearDown(): void {
		$this->clean_up_cot_setup();
		remove_filter( 'wc_allow_changing_orders_storage_while_sync_is_pending', '__return_true' );
		parent::tearDown();
	}

	public function test_order_cache_perfmance() {
		global $wpdb;
		$order_1 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_2 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_3 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_4 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_5 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_6 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();

		wc_get_orders( [
			$order_1->get_id(),
			$order_2->get_id(),
			$order_3->get_id(),
			$order_4->get_id(),
			$order_5->get_id(),
			$order_6->get_id(),
		] );

		$initial_queries = $wpdb->num_queries;
		wc_get_order( $order_1->get_id() );
		wc_get_order( $order_2->get_id() );
		wc_get_order( $order_3->get_id() );
		wc_get_order( $order_4->get_id() );
		wc_get_order( $order_5->get_id() );
		wc_get_order( $order_6->get_id() );

		$final_queries = $wpdb->num_queries;
		$this->assertEquals( 0, $final_queries - $initial_queries, 'Expected no queries to occur retrieving previously retrieved Orders' );
	}
}

/**
 * Proxy class used to mimic objects
 */
class Serialing_Cache_Proxy {
	public $original_cache_instance;

	public function __construct( $original_cache_instance ) {
		$this->original_cache_instance = $original_cache_instance;
	}

	public function __call( $function, $args ) {
		return $this->original_cache_instance->$function( ...$args );
	}

	public function get( $key, $group = 'default', $force = false, &$found = null ) {
		$data = $this->original_cache_instance->get( $key, $group, $found, $found );
		if ( is_object( $data ) || is_array( $data ) ) {
			return unserialize( serialize( $data ) );
		}

		return $data;
	}
}

2. Run the test unit tests, filtering to the new test with --filter=PR46023Tests

Further Testing

1. With COT tables enabled, create, edit, delete orders via the UI to verify everything still works as expected.
2. With sync mode enabled, both post authoritative and COT authoritative, make changes to an order and verify that new sync issues do not appear.

[github-actions]

Hi @jorgeatorres, @vedanshujain,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[prettyboymp]

Pausing review on this until I can come up with a better testing process.

[vedanshujain]

Great work! Submitting some intial feedback

[vedanshujain]

Just curious, why is this required? Wouldn't the object be correctly cached at this point? (and same for cache invalidation at line 112)

[prettyboymp]

The PostsToOrdersMigrationController runs direct queries against the tables/data the OrdersTableDatastore isn't managed by the OrdersTableDatastore manages without running those queries through it. So there is nothing in that chain that would clear the object in the datastore's cache. We need to explicitly invalidate the cache for those objects since we're modifying data outside of the datastore context.

[vedanshujain]

Note that this change won't be backward compatible, maybe we can instead make this function a no-op with a developer warning.

[prettyboymp]

Good point. We'll need a good default to make sure it still works correctly. I'm thinking that falling back to the value of ::get_table_name() may be a good option.

[vedanshujain]

This function should probably be part of the WPCacheEngine class, it seems very generic, and have a very low dependency on the internals of the datastore class.

[prettyboymp]

The dependency it has on the internals of the datastore class are how objects are cached. Caching is meant to be opaque, so what happens in this method is specific to this implementation, e.g, what cache group/keying to use or if we add a similar implementation on top of a CPT datastore, we would likely call clean_post_cache() within this method.

Ideally, this method wouldn't even need to be public since the fact that there is caching isn't really something that should be known outside of the datastore. Ideally, we also wouldn't have to call the meta datastore to invalidate its cache for an object. But, because there are existing pathways that directly modify data that the OrdersTableDataStore manages, we need a way for those paths to be able to notify the datastore that the data was modified externally.

Would it make more sense to leave this method here if the method were called something like ::objects_modified_externally( array $object-ids )?

[github-actions]

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

[prettyboymp]

I would like to leave this out as all the callers to ::after_meta_change() already flush meta cache and there isn't really a direct reason to flush the cache for the object too. However, having this here is needed for Woo Subscriptions compatibility when $subscription->update_dates() is used to update the created date. This may be a trivial incompatibility, though.

Removing this line takes off another 10 queries from the checkout flow.