diff --git a/projects/packages/sync/.phan/baseline.php b/projects/packages/sync/.phan/baseline.php index 49e36b2f4fc0c..a3955bc005b47 100644 --- a/projects/packages/sync/.phan/baseline.php +++ b/projects/packages/sync/.phan/baseline.php @@ -23,7 +23,7 @@ // PhanRedundantCondition : 4 occurrences // PhanTypeExpectedObjectPropAccess : 4 occurrences // PhanTypeMismatchArgumentInternal : 4 occurrences - // PhanTypeArraySuspicious : 3 occurrences + // PhanTypeArraySuspicious : 2 occurrences // PhanTypeArraySuspiciousNullable : 3 occurrences // PhanAccessMethodInternal : 2 occurrences // PhanImpossibleCondition : 2 occurrences @@ -66,7 +66,7 @@ 'src/modules/class-full-sync-immediately.php' => ['PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeMismatchReturn'], 'src/modules/class-full-sync.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeComparisonFromArray'], 'src/modules/class-import.php' => ['PhanTypeMismatchArgumentInternal'], - 'src/modules/class-meta.php' => ['PhanParamSignatureMismatch', 'PhanTypeArraySuspicious', 'PhanTypeMismatchArgumentProbablyReal'], + 'src/modules/class-meta.php' => ['PhanParamSignatureMismatch'], 'src/modules/class-module.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchReturnProbablyReal'], 'src/modules/class-network-options.php' => ['PhanParamSignatureMismatch', 'PhanTypeMismatchReturnProbablyReal'], 'src/modules/class-options.php' => ['PhanParamSignatureMismatch', 'PhanTypeMismatchReturnProbablyReal'], diff --git a/projects/packages/sync/changelog/update-sync-meta-module b/projects/packages/sync/changelog/update-sync-meta-module new file mode 100644 index 0000000000000..4087b570d4ba1 --- /dev/null +++ b/projects/packages/sync/changelog/update-sync-meta-module @@ -0,0 +1,4 @@ +Significance: patch +Type: changed + +Jetpack Sync: Checksum performance optimizations for Meta Sync Module diff --git a/projects/packages/sync/src/modules/class-meta.php b/projects/packages/sync/src/modules/class-meta.php index de293a9bca4f8..d92ab518d024c 100644 --- a/projects/packages/sync/src/modules/class-meta.php +++ b/projects/packages/sync/src/modules/class-meta.php @@ -38,6 +38,8 @@ public function name() { * @return array */ public function get_objects_by_id( $object_type, $config ) { + global $wpdb; + $table = _get_meta_table( $object_type ); if ( ! $table ) { @@ -48,13 +50,42 @@ public function get_objects_by_id( $object_type, $config ) { return array(); } - $meta_objects = array(); + $object_id_column = $object_type . '_id'; + $object_key_pairs = array(); + foreach ( $config as $item ) { - $meta = null; if ( isset( $item['id'] ) && isset( $item['meta_key'] ) ) { - $meta = $this->get_object_by_id( $object_type, (int) $item['id'], (string) $item['meta_key'] ); + $object_key_pairs[ (int) $item['id'] ][] = (string) $item['meta_key']; } - $meta_objects[ $item['id'] . '-' . $item['meta_key'] ] = $meta; + } + + $meta_objects = array(); + $where_sql = ''; + $current_query_length = 0; + + foreach ( $object_key_pairs as $object_id => $keys ) { + $keys_placeholders = implode( ',', array_fill( 0, count( $keys ), '%s' ) ); + $where_condition = trim( + $wpdb->prepare( + // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared + "( `$object_id_column` = %d AND meta_key IN ( $keys_placeholders ) )", + array_merge( array( $object_id ), $keys ) + ) + ); + + $where_sql = empty( $where_sql ) ? $where_condition : $where_sql . ' OR ' . $where_condition; + + $current_query_length += strlen( $where_sql ); + + if ( $current_query_length > self::MAX_DB_QUERY_LENGTH ) { + $meta_objects = $this->fetch_prepared_meta_from_db( $object_type, $where_sql, $meta_objects ); + $where_sql = ''; + $current_query_length = 0; + } + } + + if ( ! empty( $where_sql ) ) { + $meta_objects = $this->fetch_prepared_meta_from_db( $object_type, $where_sql, $meta_objects ); } return $meta_objects; @@ -76,37 +107,86 @@ public function get_object_by_id( $object_type, $id = null, $meta_key = null ) { return null; } - $table = _get_meta_table( $object_type ); + $table = _get_meta_table( $object_type ); + + if ( ! $table ) { + return null; + } + $object_id_column = $object_type . '_id'; // Sanitize so that the array only has integer values. - $meta = $wpdb->get_results( - $wpdb->prepare( + $where_condition = $wpdb->prepare( // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared - "SELECT * FROM {$table} WHERE {$object_id_column} = %d AND meta_key = %s", - $id, - $meta_key - ), - ARRAY_A + "{$object_id_column} = %d AND meta_key = %s", + $id, + $meta_key ); - $meta_objects = null; + $meta_objects = $this->fetch_prepared_meta_from_db( $object_type, $where_condition ); + + $key = $id . '-' . $meta_key; + + return $meta_objects[ $key ] ?? null; + } + + /** + * Fetch meta from DB and return them in a standard format. + * + * @param string $object_type The meta object type, eg 'post', 'user' etc. + * @param string $where Prepared SQL 'where' statement. + * @param array $meta_objects An existing array of meta to populate. Defaults to an empty array. + * @return array + */ + private function fetch_prepared_meta_from_db( $object_type, $where, $meta_objects = array() ) { + global $wpdb; + + $table = _get_meta_table( $object_type ); + $object_id_column = $object_type . '_id'; + + $meta = $wpdb->get_results( // phpcs:ignore WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery + // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared + "SELECT * FROM {$table} WHERE {$where}", + ARRAY_A + ); if ( ! is_wp_error( $meta ) && ! empty( $meta ) ) { foreach ( $meta as $meta_entry ) { - if ( 'post' === $object_type && strlen( $meta_entry['meta_value'] ) >= Posts::MAX_POST_META_LENGTH ) { - $meta_entry['meta_value'] = ''; + $object_id = $meta_entry[ $object_id_column ]; + $meta_key = $meta_entry['meta_key']; + $key = $object_id . '-' . $meta_key; + + if ( ! isset( $meta_objects[ $key ] ) ) { + $meta_objects[ $key ] = array(); } - $meta_objects[] = array( - 'meta_type' => $object_type, - 'meta_id' => $meta_entry['meta_id'], - 'meta_key' => $meta_key, - 'meta_value' => $meta_entry['meta_value'], - 'object_id' => $meta_entry[ $object_id_column ], - ); + + $meta_objects[ $key ][] = $this->get_prepared_meta_object( $object_type, $meta_entry ); } } return $meta_objects; } + + /** + * Accepts a DB meta entry and returns it in a standard format. + * + * @param string $object_type The meta object type, eg 'post', 'user' etc. + * @param array $meta_entry A meta array. + * @return array + */ + private function get_prepared_meta_object( $object_type, $meta_entry ) { + $object_id_column = $object_type . '_id'; + + if ( 'post' === $object_type && strlen( $meta_entry['meta_value'] ) >= Posts::MAX_POST_META_LENGTH ) { + $meta_entry['meta_value'] = ''; + } + + return array( + 'meta_type' => $object_type, + 'meta_id' => $meta_entry['meta_id'], + 'meta_key' => $meta_entry['meta_key'], + 'meta_value' => $meta_entry['meta_value'], + 'object_id' => $meta_entry[ $object_id_column ], + ); + } } diff --git a/projects/packages/sync/src/modules/class-module.php b/projects/packages/sync/src/modules/class-module.php index 7f0efd2a80f8e..a6e058e6ec474 100644 --- a/projects/packages/sync/src/modules/class-module.php +++ b/projects/packages/sync/src/modules/class-module.php @@ -29,6 +29,15 @@ abstract class Module { */ const ARRAY_CHUNK_SIZE = 10; + /** + * Max query length for DB queries. + * + * @access public + * + * @var int + */ + const MAX_DB_QUERY_LENGTH = 15 * 1024; + /** * Sync module name. * diff --git a/projects/plugins/jetpack/changelog/update-sync-meta-module b/projects/plugins/jetpack/changelog/update-sync-meta-module new file mode 100644 index 0000000000000..916cb15ef262a --- /dev/null +++ b/projects/plugins/jetpack/changelog/update-sync-meta-module @@ -0,0 +1,5 @@ +Significance: patch +Type: other +Comment: Updated Sync related unit tests + + diff --git a/projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-meta.php b/projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-meta.php index 2f049b669c49e..4fa39a0b76221 100644 --- a/projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-meta.php +++ b/projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-meta.php @@ -11,6 +11,7 @@ class WP_Test_Jetpack_Sync_Meta extends WP_Test_Jetpack_Sync_Base { protected $post_id; + protected $meta_id; protected $meta_module; protected $whitelisted_post_meta = 'foobar'; @@ -25,7 +26,7 @@ public function set_up() { $this->meta_module = Modules::get_module( 'meta' ); Settings::update_settings( array( 'post_meta_whitelist' => array( 'foobar' ) ) ); $this->post_id = self::factory()->post->create(); - add_post_meta( $this->post_id, $this->whitelisted_post_meta, 'foo' ); + $this->meta_id = add_post_meta( $this->post_id, $this->whitelisted_post_meta, 'foo' ); $this->sender->do_sync(); } @@ -298,6 +299,42 @@ public function assertOptionIsSynced( $meta_key, $value, $type, $object_id ) { $this->assertEqualsObject( $value, $this->server_replica_storage->get_metadata( $type, $object_id, $meta_key, true ), 'Synced option doesn\'t match local option.' ); } + /** + * Verify that get_object_by_id will return null for non existing meta. + */ + public function test_get_object_by_id_will_return_null_for_non_existing_meta() { + $module = Modules::get_module( 'meta' ); + $this->assertNull( $module->get_object_by_id( 'post', $this->post_id, 'does_not_exist' ) ); + } + + /** + * Test get_object_by_id with multiple meta for the same object_id and key. + */ + public function test_get_object_by_id_multiple_meta_same_object_id_and_key() { + $meta_id = add_post_meta( $this->post_id, $this->whitelisted_post_meta, 'bar' ); + $module = Modules::get_module( 'meta' ); + + $metas = $module->get_object_by_id( 'post', $this->post_id, $this->whitelisted_post_meta ); + $expected = array( + array( + 'meta_type' => 'post', + 'meta_id' => (string) $this->meta_id, + 'meta_key' => $this->whitelisted_post_meta, + 'meta_value' => 'foo', + 'object_id' => (string) $this->post_id, + ), + array( + 'meta_type' => 'post', + 'meta_id' => (string) $meta_id, + 'meta_key' => $this->whitelisted_post_meta, + 'meta_value' => 'bar', + 'object_id' => (string) $this->post_id, + ), + ); + + $this->assertSame( $expected, $metas ); + } + /** * Verify that meta_values above size limit are truncated in get_object_by_id */ @@ -321,4 +358,104 @@ public function test_get_object_by_id_size_limit_max() { $metas = $module->get_object_by_id( 'post', $this->post_id, $this->whitelisted_post_meta ); $this->assertEquals( $meta_test_value, $metas[0]['meta_value'] ); } + + /** + * Tests get_objects_by_id + */ + public function test_get_objects_by_id() { + $module = Modules::get_module( 'meta' ); + $meta_id = add_post_meta( $this->post_id, $this->whitelisted_post_meta, 'bar' ); + $config = array( + array( + 'id' => $this->post_id, + 'meta_key' => $this->whitelisted_post_meta, + ), + ); + $metas = $module->get_objects_by_id( 'post', $config ); + + $expected = array( + $this->post_id . '-' . $this->whitelisted_post_meta => array( + array( + 'meta_type' => 'post', + 'meta_id' => (string) $this->meta_id, + 'meta_key' => $this->whitelisted_post_meta, + 'meta_value' => 'foo', + 'object_id' => (string) $this->post_id, + ), + array( + 'meta_type' => 'post', + 'meta_id' => (string) $meta_id, + 'meta_key' => $this->whitelisted_post_meta, + 'meta_value' => 'bar', + 'object_id' => (string) $this->post_id, + ), + ), + ); + $this->assertSame( $expected, $metas ); + } + + /** + * Verify that meta_values above size limit are truncated in get_objects_by_id. + */ + public function test_get_objects_by_id_size_limit_exceeded() { + $meta_test_value = str_repeat( 'X', Posts::MAX_POST_META_LENGTH ); + update_post_meta( $this->post_id, $this->whitelisted_post_meta, $meta_test_value ); + + $module = Modules::get_module( 'meta' ); + $config = array( + array( + 'id' => $this->post_id, + 'meta_key' => $this->whitelisted_post_meta, + ), + ); + $metas = $module->get_objects_by_id( 'post', $config ); + + $expected = array( + $this->post_id . '-' . $this->whitelisted_post_meta => array( + array( + 'meta_type' => 'post', + 'meta_id' => (string) $this->meta_id, + 'meta_key' => $this->whitelisted_post_meta, + 'meta_value' => '', + 'object_id' => (string) $this->post_id, + ), + ), + ); + $this->assertSame( $expected, $metas ); + } + + /** + * Tests get_objects_by_id when the max DB query length is exceeded. + */ + public function test_get_objects_by_id_max_query_length_exceeded() { + $module = Modules::get_module( 'meta' ); + $long_meta_key = str_repeat( 'X', $module::MAX_DB_QUERY_LENGTH ); + // We are not actually adding the meta, only in $config so that it produces a long DB query. + $config = array( + array( + 'id' => 9999, + 'meta_key' => $long_meta_key, + ), + array( + 'id' => $this->post_id, + 'meta_key' => $this->whitelisted_post_meta, + ), + ); + + $metas = $module->get_objects_by_id( 'post', $config ); + + $expected = array( + $this->post_id . '-' . $this->whitelisted_post_meta => array( + array( + 'meta_type' => 'post', + 'meta_id' => (string) $this->meta_id, + 'meta_key' => $this->whitelisted_post_meta, + 'meta_value' => 'foo', + 'object_id' => (string) $this->post_id, + ), + ), + ); + + $this->assertSame( $expected, $metas ); + } } diff --git a/tools/phpcs-excludelist.json b/tools/phpcs-excludelist.json index b4414dcbda76f..a93b4b1b5cd67 100644 --- a/tools/phpcs-excludelist.json +++ b/tools/phpcs-excludelist.json @@ -7,7 +7,6 @@ "projects/packages/sync/src/class-replicastore.php", "projects/packages/sync/src/modules/class-full-sync-immediately.php", "projects/packages/sync/src/modules/class-full-sync.php", - "projects/packages/sync/src/modules/class-meta.php", "projects/packages/sync/src/modules/class-module.php", "projects/packages/sync/src/modules/class-posts.php", "projects/packages/sync/src/modules/class-term-relationships.php",