From a77682af6bd406bb426cb81f5b402e0970214788 Mon Sep 17 00:00:00 2001 From: Rebecca Hum <16962021+rebeccahum@users.noreply.github.com> Date: Wed, 3 May 2023 16:46:08 -0600 Subject: [PATCH] Search: Default state is no active index versions --- search/includes/classes/class-versioning.php | 30 ++--- .../classes/commands/class-corecommand.php | 104 ++++++++++----- .../includes/classes/test-class-health.php | 15 +++ .../includes/classes/test-class-queue.php | 26 ++-- .../includes/classes/test-class-search.php | 24 ++++ .../classes/test-class-settingshealthjob.php | 13 ++ .../classes/test-class-versioning.php | 126 ++++++++---------- 7 files changed, 207 insertions(+), 131 deletions(-) diff --git a/search/includes/classes/class-versioning.php b/search/includes/classes/class-versioning.php index f78051dfee6..3e56293fe25 100644 --- a/search/includes/classes/class-versioning.php +++ b/search/includes/classes/class-versioning.php @@ -227,36 +227,22 @@ public function get_versions( Indexable $indexable, bool $provide_default = true $versions = []; if ( $indexable->global ) { - $versions = get_site_option( self::INDEX_VERSIONS_OPTION_GLOBAL, array() ); + $versions = get_site_option( self::INDEX_VERSIONS_OPTION_GLOBAL ); } else { - $versions = get_option( self::INDEX_VERSIONS_OPTION, array() ); + $versions = get_option( self::INDEX_VERSIONS_OPTION ); } $slug = $indexable->slug; - if ( ! $this->versions_array_has_slug( $versions, $slug ) ) { - if ( $provide_default ) { - return array( - 1 => array( - 'number' => 1, - 'active' => true, - 'created_time' => null, // We don't know when it was actually created - 'activated_time' => null, - ), - ); - } else { - return []; - } + if ( ! is_array( $versions ) || ! isset( $versions[ $indexable->slug ] ) || + ( isset( $versions[ $indexable->slug ] ) && ! is_array( $versions[ $indexable->slug ] ) ) ) { + return []; } // Normalize the versions to ensure consistency (have all fields, etc) return array_map( array( $this, 'normalize_version' ), $versions[ $slug ] ); } - private function versions_array_has_slug( $versions, $slug ) { - return is_array( $versions ) && isset( $versions[ $slug ] ) && is_array( $versions[ $slug ] ) && ! empty( $versions[ $slug ] ); - } - /** * Normalize the fields of a version, to handle old or incomplete data * @@ -426,7 +412,11 @@ public function get_version( Indexable $indexable, $version_number ) { public function add_version( Indexable $indexable ) { $versions = $this->get_versions( $indexable ); - $new_version_number = $this->get_next_version_number( $versions ); + if ( empty( $versions ) ) { + $new_version_number = 1; + } else { + $new_version_number = $this->get_next_version_number( $versions ); + } if ( ! $new_version_number ) { return new WP_Error( 'unable-to-get-next-version', 'Unable to determine next index version' ); diff --git a/search/includes/classes/commands/class-corecommand.php b/search/includes/classes/commands/class-corecommand.php index 283ee26577b..db4db082b2f 100644 --- a/search/includes/classes/commands/class-corecommand.php +++ b/search/includes/classes/commands/class-corecommand.php @@ -125,44 +125,88 @@ private function list_indexes() { } protected function maybe_setup_index_version( $assoc_args ) { - if ( array_key_exists( 'version', $assoc_args ) || array_key_exists( 'using-versions', $assoc_args ) ) { - $version_number = ''; - $using_versions = $assoc_args['using-versions'] ?? false; - if ( $assoc_args['version'] ?? false ) { + $setup_flag = isset( $assoc_args['setup'] ) && $assoc_args['setup']; + $using_versions_flag = isset( $assoc_args['using-versions'] ) && $assoc_args['using-versions']; + $version_flag = isset( $assoc_args['version'] ) && $assoc_args['version']; + + // For each indexable specified, override the version + if ( ! isset( $assoc_args['indexables'] ) ) { + $indexables = \ElasticPress\Indexables::factory()->get_all(); + if ( empty( $indexables ) ) { + WP_CLI::Error( 'No indexables found!' ); + } + } else { + $indexables = $this->parse_indexables( $assoc_args ); + } + + $search = \Automattic\VIP\Search\Search::instance(); + foreach ( $indexables as $indexable ) { + if ( ! $version_flag && ! $using_versions_flag && ! $setup_flag ) { + // No flags set, so we need to get the active version to index. + $version_number = $search->versioning->get_active_version_number( $indexable ); + if ( is_wp_error( $version_number ) ) { + WP_CLI::Error( sprintf( 'No active version found for %s', $indexable->slug ) ); + } + $this->set_version( $indexable, $version_number ); + continue; + } + + if ( $version_flag ) { + // If version flag passed in, explicitly use that. This will also cover a --version && --setup case. $version_number = $assoc_args['version']; - } elseif ( $using_versions ) { - $version_number = 'next'; + $version = $search->versioning->get_version( $indexable, $version_number ); + if ( ! $version ) { + WP_CLI::Error( sprintf( 'No version found for %d for %s', $version_number, $indexable->slug ) ); + } + + $this->set_version( $indexable, $version_number ); + continue; } - if ( $version_number ) { - $search = \Automattic\VIP\Search\Search::instance(); - - // For each indexable specified, override the version - $indexables = $this->parse_indexables( $assoc_args ); - - if ( $using_versions ) { - foreach ( $indexables as $indexable ) { - $current_versions = $search->versioning->get_versions( $indexable ); - if ( count( $current_versions ) > 1 ) { - WP_CLI::error( sprintf( - 'There needs to be only one version per indexable in order to automatically use versions to reindex. Please remove inactive versions for indexable "%s".', - $indexable->slug - ) ); - } - } + $current_versions = $search->versioning->get_versions( $indexable ); + if ( $using_versions_flag ) { + if ( count( $current_versions ) > 1 ) { + WP_CLI::error( sprintf( + 'There needs to be only one version per indexable in order to automatically use versions to reindex. Please remove inactive versions for indexable "%s".', + $indexable->slug + ) ); + } + if ( empty( $current_versions ) ) { + WP_CLI::error( sprintf( + 'There needs to be at least one version in order to use the --using-versions parameter. Please add a version for indexable "%s".', + $indexable->slug + ) ); + } - foreach ( $indexables as $indexable ) { - $result = $search->versioning->add_version( $indexable ); - if ( is_wp_error( $result ) ) { - WP_CLI::error( sprintf( 'Error adding new version: %s', $result->get_error_message() ) ); - } + $version_number = $search->versioning->get_active_version_number( $indexable ); + if ( is_wp_error( $version_number ) ) { + WP_CLI::Error( sprintf( 'No active version found for %s. Must have an active version to use the --using-versions parameter.', $indexable->slug ) ); + } + $result = $search->versioning->add_version( $indexable ); + if ( is_wp_error( $result ) ) { + WP_CLI::error( sprintf( 'Error adding new version: %s', $result->get_error_message() ) ); + } + $version_number = 'next'; + } else { + if ( empty( $current_versions ) ) { + // If no versions exist, create new version and activate it. + $result = $search->versioning->add_version( $indexable ); + if ( is_wp_error( $result ) ) { + WP_CLI::error( sprintf( 'Error adding new version: %s', $result->get_error_message() ) ); + } + $result = $search->versioning->activate_version( $indexable, 1 ); // Activate first version + if ( is_wp_error( $result ) ) { + WP_CLI::error( sprintf( 'Error activating version 1: %s', $result->get_error_message() ) ); } } - - foreach ( $indexables as $indexable ) { - $this->set_version( $indexable, $version_number ); + // Check that there is an active version to use before assigning $version_number. + $active_version = $search->versioning->get_active_version_number( $indexable ); + if ( is_wp_error( $active_version ) ) { + $result = $search->versioning->activate_version( $indexable, array_key_first( $current_versions ) ); // Activate first version } + $version_number = 'active'; } + $this->set_version( $indexable, $version_number ); } } diff --git a/tests/search/includes/classes/test-class-health.php b/tests/search/includes/classes/test-class-health.php index abd64dc8e45..035e295f345 100644 --- a/tests/search/includes/classes/test-class-health.php +++ b/tests/search/includes/classes/test-class-health.php @@ -37,9 +37,24 @@ class Health_Test extends WP_UnitTestCase { public function tearDown(): void { Constant_Mocker::clear(); + delete_option( Versioning::INDEX_VERSIONS_OPTION ); + parent::tearDown(); } + public function setUp(): void { + do_action( 'plugins_loaded' ); + + parent::setUp(); + + Constant_Mocker::define( 'VIP_ORIGIN_DATACENTER', 'foo' ); + + // Set-up initial version + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + self::$search_instance->versioning->add_version( $indexable ); + self::$search_instance->versioning->activate_version( $indexable, 1 ); + } + public static function setUpBeforeClass(): void { self::$search_instance = new \Automattic\VIP\Search\Search(); self::$search_instance->init(); diff --git a/tests/search/includes/classes/test-class-queue.php b/tests/search/includes/classes/test-class-queue.php index 07c344757e0..e42e5a45a4c 100644 --- a/tests/search/includes/classes/test-class-queue.php +++ b/tests/search/includes/classes/test-class-queue.php @@ -27,14 +27,6 @@ public static function setUpBeforeClass(): void { } require_once __DIR__ . '/../../../../search/search.php'; - - \Automattic\VIP\Search\Search::instance()->init(); - - // Required so that EP registers the Indexables - do_action( 'plugins_loaded' ); - - // Users indexable doesn't get registered by default, but we have tests that queue user objects - \ElasticPress\Indexables::factory()->register( new \ElasticPress\Indexable\User\User() ); } public function setUp(): void { @@ -49,6 +41,22 @@ public function setUp(): void { $this->es = \Automattic\VIP\Search\Search::instance(); $this->es->init(); + // Required so that EP registers the Indexables + do_action( 'plugins_loaded' ); + + // Users indexable doesn't get registered by default, but we have tests that queue user objects + \ElasticPress\Indexables::factory()->register( new \ElasticPress\Indexable\User\User() ); + + // Create user indexable and activate it + $user_indexable = \ElasticPress\Indexables::factory()->get( 'user' ); + $this->es->versioning->add_version( $user_indexable ); + $this->es->versioning->activate_version( $user_indexable, 1 ); + + // Create post indexable and activate it + $post_indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + $this->es->versioning->add_version( $post_indexable ); + $this->es->versioning->activate_version( $post_indexable, 1 ); + $this->queue = $this->es->queue; $this->queue->schema->prepare_table(); @@ -61,6 +69,8 @@ public function setUp(): void { public function tearDown(): void { remove_filter( 'ep_do_intercept_request', [ $this, 'filter_index_exists_request_ok' ], PHP_INT_MAX ); + delete_option( $this->es->versioning::INDEX_VERSIONS_OPTION ); + parent::tearDown(); } diff --git a/tests/search/includes/classes/test-class-search.php b/tests/search/includes/classes/test-class-search.php index 6db598e35f6..7ed7da31571 100644 --- a/tests/search/includes/classes/test-class-search.php +++ b/tests/search/includes/classes/test-class-search.php @@ -44,6 +44,18 @@ public function setUp(): void { }, E_USER_WARNING ); } + public function test_initial_state_no_versions() { + $this->init_es(); + + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + $versions = $this->search_instance->versioning->get_versions( $indexable ); + + $this->assertEquals( [], $versions, 'There should be no versions in the initial state' ); + + $skip = apply_filters( 'ep_skip_query_integration', false ); + $this->assertTrue( $skip ); + } + public function tearDown(): void { restore_error_handler(); @@ -64,13 +76,18 @@ public function test_query_es_with_invalid_type() { * Test `ep_index_name` filter for ElasticPress + VIP Search */ public function test__vip_search_filter_ep_index_name() { + Constant_Mocker::define( 'VIP_ORIGIN_DATACENTER', 'bar' ); $this->init_es(); $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + $this->search_instance->versioning->add_version( $indexable ); + $this->search_instance->versioning->activate_version( $indexable, 1 ); $index_name = apply_filters( 'ep_index_name', 'index-name', 1, $indexable ); $this->assertEquals( 'vip-123-post-1', $index_name ); + + delete_option( $this->search_instance->versioning::INDEX_VERSIONS_OPTION ); } /** @@ -79,13 +96,18 @@ public function test__vip_search_filter_ep_index_name() { * On "global" indexes, such as users, no blog id will be present */ public function test__vip_search_filter_ep_index_name_global_index() { + Constant_Mocker::define( 'VIP_ORIGIN_DATACENTER', 'bar' ); + $this->init_es(); $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + $this->search_instance->versioning->add_version( $indexable ); + $this->search_instance->versioning->activate_version( $indexable, 1 ); $index_name = apply_filters( 'ep_index_name', 'index-name', null, $indexable ); $this->assertEquals( 'vip-123-post', $index_name ); + delete_option( $this->search_instance->versioning::INDEX_VERSIONS_OPTION ); } /** @@ -294,6 +316,8 @@ public function test__vip_search_filter_ep_index_name_with_overridden_version() Constant_Mocker::define( 'FILES_CLIENT_SITE_ID', 123 ); $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + $this->search_instance->versioning->add_version( $indexable ); + $this->search_instance->versioning->activate_version( $indexable, 1 ); add_filter( 'ep_do_intercept_request', [ $this, 'filter_ok_es_requests' ], PHP_INT_MAX, 5 ); diff --git a/tests/search/includes/classes/test-class-settingshealthjob.php b/tests/search/includes/classes/test-class-settingshealthjob.php index caf4edc4d32..26dfa487495 100644 --- a/tests/search/includes/classes/test-class-settingshealthjob.php +++ b/tests/search/includes/classes/test-class-settingshealthjob.php @@ -41,9 +41,22 @@ public static function tearDownAfterClass(): void { public function setUp(): void { parent::setUp(); + Constant_Mocker::define( 'VIP_ORIGIN_DATACENTER', 'foo' ); + \Automattic\VIP\Prometheus\Plugin::get_instance()->init_registry(); self::$search->load_collector(); \Automattic\VIP\Prometheus\Plugin::get_instance()->load_collectors(); + + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + self::$search->versioning->add_version( $indexable ); + self::$search->versioning->activate_version( $indexable, 1 ); + } + + public function tearDown(): void { + parent::tearDown(); + + Constant_Mocker::clear(); + delete_option( Versioning::INDEX_VERSIONS_OPTION ); } public function test__process_indexables_settings_health_results__reports_error() { diff --git a/tests/search/includes/classes/test-class-versioning.php b/tests/search/includes/classes/test-class-versioning.php index 04915823f41..ee13952d069 100644 --- a/tests/search/includes/classes/test-class-versioning.php +++ b/tests/search/includes/classes/test-class-versioning.php @@ -32,6 +32,24 @@ class Versioning_Test extends WP_UnitTestCase { 'update_index_settings', ]; + public function setUp(): void { + parent::setUp(); + + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + self::$version_instance->add_version( $indexable ); + self::$version_instance->activate_version( $indexable, 1 ); + + add_filter( 'ep_do_intercept_request', [ $this, 'filter_index_exists_request_ok' ], PHP_INT_MAX, 5 ); + } + + public function tearDown(): void { + remove_filter( 'ep_do_intercept_request', [ $this, 'filter_index_exists_request_ok' ], PHP_INT_MAX ); + + delete_option( Versioning::INDEX_VERSIONS_OPTION ); + + parent::tearDown(); + } + public function mock_http_response( $mocked_response ) { add_filter( 'pre_http_request', function( $response, $args, $url ) use ( $mocked_response ) { return $mocked_response; @@ -73,18 +91,6 @@ public static function setUpBeforeClass(): void { Constant_Mocker::define( 'FILES_CLIENT_SITE_ID', 200508 ); } - public function setUp(): void { - parent::setUp(); - - add_filter( 'ep_do_intercept_request', [ $this, 'filter_index_exists_request_ok' ], PHP_INT_MAX, 5 ); - } - - public function tearDown(): void { - remove_filter( 'ep_do_intercept_request', [ $this, 'filter_index_exists_request_ok' ], PHP_INT_MAX ); - - parent::tearDown(); - } - public static function tearDownAfterClass(): void { Constant_Mocker::clear(); @@ -210,7 +216,7 @@ public function get_active_version_data() { // Indexable slug 'post', // Expected active version - 1, + new \WP_Error( 'no-active-version', 'No active version found' ), ), array( @@ -650,11 +656,7 @@ public function add_version_data() { // Should have added the default version 1 data 1 => array( 'number' => 1, - 'active' => true, // Defaults to active when no other index version is known - ), - 2 => array( - 'number' => 2, - 'active' => false, + 'active' => false, // Defaults to active when no other index version is known ), ), ), @@ -782,47 +784,13 @@ public function activate_version_data() { ), ), ), - - // Target index is already marked active, and is index 1 - array( - // Input array of versions - array(), - // Indexable slug - 'post', - // Version to activate - 1, - // Expected new versions - array( - 1 => array( - 'number' => 1, - 'active' => true, - ), - ), - ), - - // Switching back to 1, which may not exist in the option - array( - // Input array of versions - array(), - // Indexable slug - 'post', - // Version to activate - 1, - // Expected new versions - array( - 1 => array( - 'number' => 1, - 'active' => true, - ), - ), - ), ); } /** * @dataProvider activate_version_data */ - public function test_activate_version( $versions, $indexable_slug, $version_to_activate, $expected_new_versions ) { + public function test_activate_version__successful( $versions, $indexable_slug, $version_to_activate, $expected_new_versions ) { $indexable = \ElasticPress\Indexables::factory()->get( $indexable_slug ); self::$version_instance->update_versions( $indexable, $versions ); @@ -847,6 +815,18 @@ public function test_activate_version( $versions, $indexable_slug, $version_to_a $this->assertEquals( $now, $active_version['activated_time'], '"activated_time" property of currently active version does not match expected value' ); } + public function test_activate_version__no_versions() { + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + self::$version_instance->update_versions( $indexable, [] ); + + $now = time(); + + // Add the new version + $error = self::$version_instance->activate_version( $indexable, 1 ); + + $this->assertTrue( is_wp_error( $error ), 'Activating version should return WP_Error, but it succeeded' ); + } + public function activate_version_invalid_data() { return array( // No index marked active @@ -1116,8 +1096,6 @@ public function test_delete_version_while_active() { } public function test_current_version_number_overrides() { - delete_option( Versioning::INDEX_VERSIONS_OPTION ); - $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); $this->setup_ok_es_requests(); @@ -1183,9 +1161,12 @@ public function test_queue_job_replication() { self::$search->queue->empty_queue(); - // For these tests, we're just using the post type and index versions 1, 2, and 3, for simplicity - self::$version_instance->update_versions( \ElasticPress\Indexables::factory()->get( 'post' ), array() ); // Reset them - self::$version_instance->add_version( \ElasticPress\Indexables::factory()->get( 'post' ) ); + // For these tests, we're just using the post type and index versions 1 and 2 for simplicity + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + self::$version_instance->update_versions( $indexable, array() ); // Reset them + self::$version_instance->add_version( $indexable ); + self::$version_instance->activate_version( $indexable, 1 ); + self::$version_instance->add_version( $indexable ); do_action( 'vip_search_indexing_object_queued', 1, 'post', array( 'foo' => 'bar' ), 1 ); do_action( 'vip_search_indexing_object_queued', 2, 'post', array( 'foo' => 'bar' ), 1 ); @@ -1318,9 +1299,12 @@ public function test_replicate_queued_objects_to_other_versions( $input, $expect self::$search->queue->empty_queue(); - // For these tests, we're just using the post type and index versions 1, 2, and 3, for simplicity - self::$version_instance->update_versions( \ElasticPress\Indexables::factory()->get( 'post' ), array() ); // Reset them - self::$version_instance->add_version( \ElasticPress\Indexables::factory()->get( 'post' ) ); + // For these tests, we're just using the post type and index versions 1 and 2, for simplicity + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + self::$version_instance->update_versions( $indexable, array() ); // Reset them + self::$version_instance->add_version( $indexable ); + self::$version_instance->activate_version( $indexable, 1 ); + self::$version_instance->add_version( $indexable ); $queue_table_name = self::$search->queue->schema->get_table_name(); @@ -1346,9 +1330,12 @@ public function test_replicate_indexed_objects_to_other_versions() { self::$search->queue->empty_queue(); - // For these tests, we're just using the post type and index versions 1, 2, and 3, for simplicity - self::$version_instance->update_versions( \ElasticPress\Indexables::factory()->get( 'post' ), array() ); // Reset them - self::$version_instance->add_version( \ElasticPress\Indexables::factory()->get( 'post' ) ); + // For these tests, we're just using the post type and index versions 1 and 2 for simplicity + $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); + self::$version_instance->update_versions( $indexable, array() ); // Reset them + self::$version_instance->add_version( $indexable ); + self::$version_instance->activate_version( $indexable, 1 ); + self::$version_instance->add_version( $indexable ); $indexable = \ElasticPress\Indexables::factory()->get( 'post' ); @@ -1492,19 +1479,12 @@ public function test_get_version() { $this->assertEquals( $new['number'], $new_retrieved['number'], 'Wrong version number for returned version on newly created version' ); } - private $default_versions = [ - 1 => [ - 'number' => 1, - 'active' => true, - 'created_time' => null, - 'activated_time' => null, - ], - ]; + private $default_versions = []; public function get_versions_default_data() { return [ [ - null, + false, $this->default_versions, ], [