Jetpack Sync: Checksum performance optimizations for Meta Sync Module (…

…#41390) * Jetpack Sync: Checksum performance optimizations for Meta Sync Module
Automattic · Jan 30, 2025 · a19dccb · a19dccb
1 parent 2cf35b5
commit a19dccb
Show file tree

Hide file tree

Showing 7 changed files with 260 additions and 26 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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",