Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding OrderTableDatastore and Meta caching integration #46023

Open
wants to merge 29 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
5b732f8
Adding initial handling for OrderTableDatastore caching integration
prettyboymp Mar 27, 2024
99ed80b
fix ::cache_objects() mapping usage
prettyboymp Mar 28, 2024
0b79a82
Extend the WPCacheEngineTest tests to cover new multi-set/get methods
prettyboymp Mar 28, 2024
93cda51
Clear order data cache after ::persist_order_to_db()
prettyboymp Mar 29, 2024
1ed6f97
clear child order cache after a direct parent_order_id update
prettyboymp Mar 29, 2024
5b31713
make sure we clear cache after any direct db writes
prettyboymp Mar 29, 2024
52550a0
simplify interface to clear object cache
prettyboymp Apr 1, 2024
0584c40
add changelog
prettyboymp Apr 2, 2024
40875d4
fixing lint errors
prettyboymp Apr 2, 2024
8492c59
more lint fixes
prettyboymp Apr 2, 2024
c407d7b
continuing lint fixes
prettyboymp Apr 2, 2024
cbd5e38
additional lint fixes
prettyboymp Apr 2, 2024
1c7ac9f
Merge branch 'trunk' into fix/45550-cache-orders-at-datastore-layer
prettyboymp Apr 2, 2024
2aaa3d3
fix typo in test
prettyboymp Apr 2, 2024
36ef5f0
Merge branch 'trunk' into fix/45550-cache-orders-at-datastore-layer
prettyboymp Apr 3, 2024
a4d1163
Merge branch 'trunk' into fix/45550-cache-orders-at-datastore-layer
prettyboymp Apr 3, 2024
e6a4445
Merge branch 'trunk' into fix/45550-cache-orders-at-datastore-layer
prettyboymp Apr 5, 2024
7373c72
Adding cache invalidation after OrderTable deletion
prettyboymp Apr 5, 2024
7bdb27b
Add test to verify cache invalidation after order migration
prettyboymp Apr 5, 2024
9d553c7
lint fixes
prettyboymp Apr 5, 2024
2629f31
lint fixes
prettyboymp Apr 5, 2024
3187faa
Merge branch 'trunk' into fix/45550-cache-orders-at-datastore-layer
prettyboymp Apr 15, 2024
144ff44
update test_get_order_doesnt_return_invalid_cached_order after #46393…
prettyboymp Apr 15, 2024
3111137
lint fixes
prettyboymp Apr 15, 2024
22f2898
fix bug where partially cached set didn't correctly load uncached orders
prettyboymp Apr 18, 2024
c4e2746
Merge branch 'trunk' into fix/45550-cache-orders-at-datastore-layer
prettyboymp Apr 23, 2024
e4b252b
Merge branch 'trunk' into fix/45550-cache-orders-at-datastore-layer
prettyboymp May 6, 2024
b5780b5
changing CustomMetaDataStore::get_cache_group() to non-abstract metho…
prettyboymp May 7, 2024
86d6af2
Attempt to retrieve order types from cache prior to querying
prettyboymp May 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: performance

Added caching to HPOS Data Stores
62 changes: 61 additions & 1 deletion plugins/woocommerce/src/Caching/WPCacheEngine.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class WPCacheEngine implements CacheEngine {
/**
* Retrieves an object cached under a given key.
*
* @param string $key They key under which the object to retrieve is cached.
* @param string $key The key under which the object to retrieve is cached.
* @param string $group The group under which the object is cached.
*
* @return array|object|null The cached object, or null if there's no object cached under the passed key.
Expand All @@ -22,6 +22,40 @@ public function get_cached_object( string $key, string $group = '' ) {
return false === $value ? null : $value;
}

/**
* Retrieves a set of objects cached under the given keys.
*
* @param string[] $keys The keys under which the object to retrieve is cached.
* @param string $group The group under which the object is cached.
*
* @return array The cached array of objects keyed by the given keys, values will be null for any non-cached keys.
*/
public function get_cached_objects( array $keys, string $group = '' ) {
$prefix = self::get_cache_prefix( $group );
$key_map = array_combine(
$keys,
array_map(
function ( $key ) use ( $prefix ) {
return $prefix . $key;
},
$keys
)
);

$cached_values = wp_cache_get_multiple( array_values( $key_map ), $group );
$return_values = array();
foreach ( $key_map as $key => $prefixed_key ) {
if ( isset( $cached_values[ $prefixed_key ] ) && false !== $cached_values[ $prefixed_key ] ) {
$return_values[ $key ] = $cached_values[ $prefixed_key ];
} else {
$return_values[ $key ] = null;

}
}

return $return_values;
}

/**
* Caches an object under a given key, and with a given expiration.
*
Expand All @@ -37,6 +71,32 @@ public function cache_object( string $key, $object, int $expiration, string $gro
return false !== wp_cache_set( $prefixed_key, $object, $group, $expiration );
}

/**
* Caches an object under a given key, and with a given expiration.
*
* @param array $objects The objects to cache keyed by the key to cache under.
* @param int $expiration Expiration for the cached object, in seconds.
* @param string $group The group under which the object will be cached.
*
* @return array Array of return values, grouped by key. Each value is either
* true on success, or false on failure
*/
public function cache_objects( array $objects, int $expiration, string $group = '' ): array {
$prefix = self::get_cache_prefix( $group );

$objects = array_combine(
array_map(
function ( $key ) use ( $prefix ) {
return $prefix . $key;
},
array_keys( $objects )
),
$objects,
);

return wp_cache_set_multiple( $objects, $group, $expiration );
}

/**
* Removes a cached object from the cache.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

namespace Automattic\WooCommerce\Database\Migrations\CustomOrderTable;

use Automattic\WooCommerce\Caching\WPCacheEngine;
use Automattic\WooCommerce\Internal\DataStores\Orders\CustomOrdersTableController;
use Automattic\WooCommerce\Internal\DataStores\Orders\OrdersTableDataStore;
use Automattic\WooCommerce\Utilities\ArrayUtil;

/**
Expand All @@ -26,7 +28,7 @@ class PostsToOrdersMigrationController {
/**
* Array of objects used to perform the migration.
*
* @var TableMigrator[]
* @var \Automattic\WooCommerce\Database\Migrations\TableMigrator[]
*/
private $all_migrators;

Expand Down Expand Up @@ -98,13 +100,16 @@ public function migrate_orders( array $order_post_ids ): void {
}

$this->handle_migration_error( $order_post_ids, $errors, $exception, $using_transactions, $name );
if ( ! $using_transactions ) {
wc_get_container()->get( OrdersTableDataStore::class )->invalidate_cache_for_objects( $order_post_ids );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return;
}

if ( $using_transactions ) {
$this->commit_transaction();
}

wc_get_container()->get( OrdersTableDataStore::class )->invalidate_cache_for_objects( $order_post_ids );
}

/**
Expand Down
150 changes: 125 additions & 25 deletions plugins/woocommerce/src/Internal/DataStores/CustomMetaDataStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

namespace Automattic\WooCommerce\Internal\DataStores;

use Automattic\WooCommerce\Caching\WPCacheEngine;

/**
* Implements functions similar to WP's add_metadata(), get_metadata(), and friends using a custom table.
*
Expand Down Expand Up @@ -50,34 +52,32 @@ protected function get_db_info() {
);
}

/**
* Returns the cache group to store cached data in. This should be unique for the underlying data storage, e.g. table.
*
* @return string
*/
protected function get_cache_group() {
return $this->get_table_name();
}

/**
* Returns an array of meta for an object.
*
* @param WC_Data $object WC_Data object.
* @param \WC_Data $object WC_Data object.
* @return array
*/
public function read_meta( &$object ) {
global $wpdb;

$db_info = $this->get_db_info();

// phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared
$raw_meta_data = $wpdb->get_results(
$wpdb->prepare(
"SELECT {$db_info['meta_id_field']} AS meta_id, meta_key, meta_value FROM {$db_info['table']} WHERE {$db_info['object_id_field']} = %d ORDER BY meta_id",
$object->get_id()
)
);
// phpcs:enable WordPress.DB.PreparedSQL.InterpolatedNotPrepared
$raw_meta_data = $this->get_meta_data_for_object_ids( array( $object->get_id() ) );

return $raw_meta_data;
return isset( $raw_meta_data[ $object->get_id() ] ) ? (array) $raw_meta_data[ $object->get_id() ] : array();
}

/**
* Deletes meta based on meta ID.
*
* @param WC_Data $object WC_Data object.
* @param stdClass $meta (containing at least ->id).
* @param \WC_Data $object WC_Data object.
* @param \stdClass $meta (containing at least ->id).
*
* @return bool
*/
Expand All @@ -91,7 +91,18 @@ public function delete_meta( &$object, $meta ) : bool {
$db_info = $this->get_db_info();
$meta_id = absint( $meta->id );

return (bool) $wpdb->delete( $db_info['table'], array( $db_info['meta_id_field'] => $meta_id ) );
$successful = (bool) $wpdb->delete(
$db_info['table'],
array(
$db_info['meta_id_field'] => $meta_id,
$db_info['object_id_field'] => $object->get_id(),
)
);
if ( $successful ) {
$this->invalidate_cache_for_objects( array( $object->get_id() ) );
}

return $successful;
}

/**
Expand Down Expand Up @@ -122,14 +133,18 @@ public function add_meta( &$object, $meta ) {
);
// phpcs:enable WordPress.DB.SlowDBQuery.slow_db_query_meta_value,WordPress.DB.SlowDBQuery.slow_db_query_meta_key

return $result ? (int) $wpdb->insert_id : false;
$insert_id = $result ? (int) $wpdb->insert_id : false;
if ( false !== $insert_id ) {
$this->invalidate_cache_for_objects( array( $object->get_id() ) );
}
return $insert_id;
}

/**
* Update meta.
*
* @param WC_Data $object WC_Data object.
* @param stdClass $meta (containing ->id, ->key and ->value).
* @param \WC_Data $object WC_Data object.
* @param \stdClass $meta (containing ->id, ->key and ->value).
*
* @return bool
*/
Expand All @@ -147,17 +162,22 @@ public function update_meta( &$object, $meta ) : bool {
);
// phpcs:enable WordPress.DB.SlowDBQuery.slow_db_query_meta_value,WordPress.DB.SlowDBQuery.slow_db_query_meta_key

$db_info = $this->get_db_info();

$result = $wpdb->update(
$db_info['table'],
$this->get_table_name(),
$data,
array( $db_info['meta_id_field'] => $meta->id ),
array(
$this->get_meta_id_field() => $meta->id,
$this->get_object_id_field() => $object->get_id(),
),
'%s',
'%d'
);

return 1 === $result;
$is_successful = 1 === $result;
if ( $is_successful ) {
$this->invalidate_cache_for_objects( array( $object->get_id() ) );
}
return $is_successful;
}

/**
Expand Down Expand Up @@ -265,4 +285,84 @@ public function get_meta_keys( $limit = 100, $order = 'ASC', $include_private =
return $wpdb->get_col( $query ); // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared -- $query is prepared.
}

/**
* Return order meta data for multiple IDs. Results are cached.
*
* @param array $ids List of order IDs.
*
* @return \stdClass[] DB Order objects or error.
*/
public function get_meta_data_for_object_ids( array $ids ): array {
global $wpdb;

$cache_engine = wc_get_container()->get( WPCacheEngine::class );
$meta_data = $cache_engine->get_cached_objects( $ids, $this->get_cache_group() );
$meta_data = array_filter( $meta_data );
$uncached_order_ids = array_diff( $ids, array_keys( $meta_data ) );

if ( empty( $uncached_order_ids ) ) {
return $meta_data;
}

$id_placeholder = implode( ', ', array_fill( 0, count( $uncached_order_ids ), '%d' ) );
$meta_table = $this->get_table_name();
$object_id_column = $this->get_object_id_field();

// phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare -- $object_id_column and $meta_table is hardcoded. IDs are prepared above.
$meta_rows = $wpdb->get_results(
$wpdb->prepare(
"SELECT id, $object_id_column as object_id, meta_key, meta_value FROM $meta_table WHERE $object_id_column in ( $id_placeholder )",
$ids
)
);
// phpcs:enable

foreach ( $meta_rows as $meta_row ) {
if ( ! isset( $meta_data[ $meta_row->object_id ] ) ) {
$meta_data[ $meta_row->object_id ] = array();
}
$meta_data[ $meta_row->object_id ][] = (object) array(
'meta_id' => $meta_row->id,
'meta_key' => $meta_row->meta_key, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key
'meta_value' => $meta_row->meta_value, // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_value
);
}
$cache_engine->cache_objects( $meta_data, 0, $this->get_cache_group() );

return $meta_data;
}

/**
* Delete cached meta data for the given object_ids.
*
* @internal This method should only be used by internally and in cases where the CRUD operations of this datastore
* are bypassed for performance purposes. This interface is not guaranteed.
*
* @param array $object_ids The object_ids to delete cache for.
*
* @return bool[] Array of return values, grouped by the object_id. Each value is either true on success, or false
* if the contents were not deleted.
*/
public function invalidate_cache_for_objects( array $object_ids ): array {
$cache_engine = wc_get_container()->get( WPCacheEngine::class );
$return_values = array();
foreach ( $object_ids as $object_id ) {
$return_values[ $object_id ] = $cache_engine->delete_cached_object( $object_id, $this->get_cache_group() );
}
return $return_values;
}

/**
* Invalidate all the cache used by this data store.
*
* @internal This method should only be used by internally and in cases where the CRUD operations of this datastore
* are bypassed for performance purposes. This interface is not guaranteed.
*
* @return bool Whether the cache as fully invalidated.
*/
public function invalidate_all_cache(): bool {
$cache_engine = wc_get_container()->get( WPCacheEngine::class );

return $cache_engine->delete_cache_group( $this->get_cache_group() );
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ public function delete_database_tables() {
foreach ( $table_names as $table_name ) {
$this->database_util->drop_database_table( $table_name );
}
if ( is_callable( array( $this->data_store, 'invalidate_all_cache' ) ) ) {
$this->data_store->invalidate_all_cache();
}
delete_option( self::ORDERS_TABLE_CREATED );
}

Expand Down