From 95d0bbc082f8862981fb998953c611c29c71f486 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Mu=CC=88ller?= Date: Tue, 24 Jan 2023 15:59:38 +0100 Subject: [PATCH 1/4] BUGFIX: Setting injection doesn't cause errors Due to the order of operations with injectSettings and object injection it could cause errors to resolve the index settings early enough to override the name property. This is now resolved later. Additionally via `Index::getOriginalName()` one can now obtain the name without prefix, if necessary. BUGFIX: Index and testing overhauled --- Classes/Domain/Model/Index.php | 115 +++++++++++++--------- Configuration/Testing/Settings.yaml | 29 ++++++ Tests/Functional/Domain/AbstractTest.php | 2 +- Tests/Functional/Domain/IndexTest.php | 120 +++++++++++++++++++++++ 4 files changed, 219 insertions(+), 47 deletions(-) create mode 100644 Configuration/Testing/Settings.yaml create mode 100644 Tests/Functional/Domain/IndexTest.php diff --git a/Classes/Domain/Model/Index.php b/Classes/Domain/Model/Index.php index a60b576..a8b3013 100644 --- a/Classes/Domain/Model/Index.php +++ b/Classes/Domain/Model/Index.php @@ -29,39 +29,45 @@ class Index * @see http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/indices-update-settings.html */ static protected $updatableSettings = [ - 'index.number_of_replicas', - 'index.auto_expand_replicas', - 'index.blocks.read_only', - 'index.blocks.read', - 'index.blocks.write', - 'index.blocks.metadata', - 'index.refresh_interval', - 'index.index_concurrency', - 'index.codec', - 'index.codec.bloom.load', - 'index.fail_on_merge_failure', - 'index.translog.flush_threshold_ops', - 'index.translog.flush_threshold_size', - 'index.translog.flush_threshold_period', - 'index.translog.disable_flush', - 'index.cache.filter.max_size', - 'index.cache.filter.expire', - 'index.gateway.snapshot_interval', - 'index.routing.allocation.include', - 'index.routing.allocation.exclude', - 'index.routing.allocation.require', - 'index.routing.allocation.disable_allocation', - 'index.routing.allocation.disable_new_allocation', - 'index.routing.allocation.disable_replica_allocation', - 'index.routing.allocation.enable', - 'index.routing.allocation.total_shards_per_node', - 'index.recovery.initial_shards', - 'index.gc_deletes', - 'index.ttl.disable_purge', - 'index.translog.fs.type', - 'index.compound_format', - 'index.compound_on_flush', - 'index.warmer.enabled', + 'settings.index.number_of_replicas', + 'settings.index.auto_expand_replicas', + 'settings.index.blocks.read_only', + 'settings.index.blocks.read', + 'settings.index.blocks.write', + 'settings.index.blocks.metadata', + 'settings.index.refresh_interval', + 'settings.index.index_concurrency', + 'settings.index.codec', + 'settings.index.codec.bloom.load', + 'settings.index.fail_on_merge_failure', + 'settings.index.translog.flush_threshold_ops', + 'settings.index.translog.flush_threshold_size', + 'settings.index.translog.flush_threshold_period', + 'settings.index.translog.disable_flush', + 'settings.index.cache.filter.max_size', + 'settings.index.cache.filter.expire', + 'settings.index.gateway.snapshot_interval', + 'settings.index.routing.allocation.include', + 'settings.index.routing.allocation.exclude', + 'settings.index.routing.allocation.require', + 'settings.index.routing.allocation.disable_allocation', + 'settings.index.routing.allocation.disable_new_allocation', + 'settings.index.routing.allocation.disable_replica_allocation', + 'settings.index.routing.allocation.enable', + 'settings.index.routing.allocation.total_shards_per_node', + 'settings.index.recovery.initial_shards', + 'settings.index.gc_deletes', + 'settings.index.ttl.disable_purge', + 'settings.index.translog.fs.type', + 'settings.index.compound_format', + 'settings.index.compound_on_flush', + 'settings.index.warmer.enabled', + ]; + + static protected $allowedIndexCreateKeys = [ + 'settings', + 'aliases', + 'mappings' ]; /** @@ -122,13 +128,6 @@ public function __construct(string $name, Client $client = null) public function injectSettings(array $settings): void { $this->settings = $settings; - $indexSettings = $this->getSettings(); - if (!isset($indexSettings['prefix']) || empty($indexSettings['prefix'])) { - return; - } - // This is obviously a side effect but can only be done after injecting settings - // and it needs to be done as early as possible - $this->name = $settings['prefix'] . '-' . $this->name; } /** @@ -141,7 +140,7 @@ public function findType(string $typeName): AbstractType } /** - * @param array $types + * @param array $types * @return TypeGroup */ public function findTypeGroup(array $types): TypeGroup @@ -173,11 +172,11 @@ public function exists(): bool public function request(string $method, string $path = null, array $arguments = [], $content = null, bool $prefixIndex = true): Response { if ($this->client === null) { - throw new ElasticSearchException('The client of the index "' . $this->name . '" is not set, hence no requests can be done.', 1566313883); + throw new ElasticSearchException('The client of the index "' . $this->prefixName() . '" is not set, hence no requests can be done.', 1566313883); } $path = ltrim($path ? trim($path) : '', '/'); if ($prefixIndex === true) { - $path = '/' . $this->name . '/' . $path; + $path = '/' . $this->prefixName() . '/' . $path; } else { $path = '/' . ltrim($path, '/'); } @@ -191,7 +190,9 @@ public function request(string $method, string $path = null, array $arguments = */ public function create(): void { - $this->request('PUT', null, [], json_encode($this->getSettings())); + $indexSettings = $this->getSettings(); + $creationSettings = array_filter($indexSettings, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY); + $this->request('PUT', null, [], json_encode($creationSettings)); } /** @@ -206,7 +207,7 @@ protected function getSettings(): ?array } $settings = Arrays::getValueByPath($this->settings, $path); - return $settings !== null ? $this->dynamicIndexSettingService->process($settings, $path, $this->getName()) : $settings; + return $settings !== null ? $this->dynamicIndexSettingService->process($settings, $path, $this->name) : $settings; } /** @@ -223,7 +224,9 @@ public function updateSettings(): void $updatableSettings = Arrays::setValueByPath($updatableSettings, $settingPath, $setting); } } - $this->request('PUT', '/_settings', [], json_encode($updatableSettings)); + if ($updatableSettings !== []) { + $this->request('PUT', '/_settings', [], json_encode($updatableSettings)); + } } /** @@ -252,6 +255,11 @@ public function refresh(): Response * @return string */ public function getName(): string + { + return $this->prefixName(); + } + + public function getOriginalName(): string { return $this->name; } @@ -273,4 +281,19 @@ public function setSettingsKey(string $settingsKey): void { $this->settingsKey = $settingsKey; } + + /** + * Prepends configured preset to the base index name + * + * @return string + */ + private function prefixName(): string + { + $indexSettings = $this->getSettings(); + if (!isset($indexSettings['prefix']) || empty($indexSettings['prefix'])) { + return $this->name; + } + + return $indexSettings['prefix'] . '-' . $this->name; + } } diff --git a/Configuration/Testing/Settings.yaml b/Configuration/Testing/Settings.yaml new file mode 100644 index 0000000..a7ef5fd --- /dev/null +++ b/Configuration/Testing/Settings.yaml @@ -0,0 +1,29 @@ +Flowpack: + ElasticSearch: + clients: + FunctionalTests: + - + host: localhost + port: 9200 + scheme: 'http' + username: '' + password: '' + realtimeIndexing: + # we also use this setting for the object indexer client bundle + client: FunctionalTests + + indexes: + FunctionalTests: # Configuration bundle name + index_with_prefix: # The index prefix name, must be the same as in the Neos.ContentRepository.Search.elasticSearch.indexName setting + prefix: 'prefix' + settings: + index: + number_of_replicas: 1 + soft_deletes: + enabled: true + index_without_prefix: + settings: + index: + number_of_replicas: 2 + soft_deletes: + enabled: true diff --git a/Tests/Functional/Domain/AbstractTest.php b/Tests/Functional/Domain/AbstractTest.php index 5f000ac..afa8a40 100644 --- a/Tests/Functional/Domain/AbstractTest.php +++ b/Tests/Functional/Domain/AbstractTest.php @@ -40,7 +40,7 @@ final public function setUp(): void parent::setUp(); $this->clientFactory = $this->objectManager->get(ClientFactory::class); - $client = $this->clientFactory->create(); + $client = $this->clientFactory->create("FunctionalTests"); $this->testingIndex = $client->findIndex('flow_elasticsearch_functionaltests'); if ($this->testingIndex->exists()) { diff --git a/Tests/Functional/Domain/IndexTest.php b/Tests/Functional/Domain/IndexTest.php new file mode 100644 index 0000000..7ab5bf5 --- /dev/null +++ b/Tests/Functional/Domain/IndexTest.php @@ -0,0 +1,120 @@ +createMock(Client::class); + $clientMock->method('getBundle') + ->willReturn('FunctionalTests'); + + $clientMock->expects($this->exactly(2))->method('request') + ->withConsecutive( + [ + 'PUT', + '/index_without_prefix/', + [], + json_encode([ + 'settings' => [ + 'index' => [ + 'number_of_replicas' => 2, + 'soft_deletes' => [ + 'enabled' => true + ] + ] + ] + ], JSON_THROW_ON_ERROR) + ], + // updateSettings should correctly filter soft_deletes as it's not in the allow list + [ + 'PUT', + '/index_without_prefix/_settings', + [], + json_encode([ + 'settings' => [ + 'index' => [ + 'number_of_replicas' => 2 + ] + ] + ], JSON_THROW_ON_ERROR) + ] + ) + ->willReturn($this->createStub(Response::class)); + + $testObject = new Index('index_without_prefix', $clientMock); + $testObject->create(); + $testObject->updateSettings(); + + static::assertSame('index_without_prefix', $testObject->getOriginalName()); + static::assertSame('index_without_prefix', $testObject->getName()); + } + + /** + * @test + */ + public function indexWithPrefix() + { + $clientMock = $this->createMock(Client::class); + $clientMock->method('getBundle') + ->willReturn('FunctionalTests'); + + $clientMock->expects($this->exactly(2))->method('request') + ->withConsecutive( + [ + 'PUT', + '/prefix-index_with_prefix/', + [], + json_encode([ + 'settings' => [ + 'index' => [ + 'number_of_replicas' => 1, + 'soft_deletes' => [ + 'enabled' => true + ] + ] + ] + ], JSON_THROW_ON_ERROR) + ], + // updateSettings should correctly filter soft_deletes as it's not in the allow list + [ + 'PUT', + '/prefix-index_with_prefix/_settings', + [], + json_encode([ + 'settings' => [ + 'index' => [ + 'number_of_replicas' => 1 + ] + ] + ], JSON_THROW_ON_ERROR) + ] + ) + ->willReturn($this->createStub(Response::class)); + + $testObject = new Index('index_with_prefix', $clientMock); + $testObject->create(); + $testObject->updateSettings(); + + static::assertSame('index_with_prefix', $testObject->getOriginalName()); + static::assertSame('prefix-index_with_prefix', $testObject->getName()); + } +} From 02c69e4db3e4c826c37f5bec1426fb358989d2ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Mu=CC=88ller?= Date: Tue, 24 Jan 2023 18:47:54 +0100 Subject: [PATCH 2/4] Clarify naming of settings and index configuration --- Classes/Domain/Model/Index.php | 90 ++++++++++--------- .../Service/DynamicIndexSettingService.php | 1 + Tests/Functional/Domain/IndexTest.php | 12 +-- 3 files changed, 51 insertions(+), 52 deletions(-) diff --git a/Classes/Domain/Model/Index.php b/Classes/Domain/Model/Index.php index a8b3013..3cc30d6 100644 --- a/Classes/Domain/Model/Index.php +++ b/Classes/Domain/Model/Index.php @@ -29,39 +29,39 @@ class Index * @see http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/indices-update-settings.html */ static protected $updatableSettings = [ - 'settings.index.number_of_replicas', - 'settings.index.auto_expand_replicas', - 'settings.index.blocks.read_only', - 'settings.index.blocks.read', - 'settings.index.blocks.write', - 'settings.index.blocks.metadata', - 'settings.index.refresh_interval', - 'settings.index.index_concurrency', - 'settings.index.codec', - 'settings.index.codec.bloom.load', - 'settings.index.fail_on_merge_failure', - 'settings.index.translog.flush_threshold_ops', - 'settings.index.translog.flush_threshold_size', - 'settings.index.translog.flush_threshold_period', - 'settings.index.translog.disable_flush', - 'settings.index.cache.filter.max_size', - 'settings.index.cache.filter.expire', - 'settings.index.gateway.snapshot_interval', - 'settings.index.routing.allocation.include', - 'settings.index.routing.allocation.exclude', - 'settings.index.routing.allocation.require', - 'settings.index.routing.allocation.disable_allocation', - 'settings.index.routing.allocation.disable_new_allocation', - 'settings.index.routing.allocation.disable_replica_allocation', - 'settings.index.routing.allocation.enable', - 'settings.index.routing.allocation.total_shards_per_node', - 'settings.index.recovery.initial_shards', - 'settings.index.gc_deletes', - 'settings.index.ttl.disable_purge', - 'settings.index.translog.fs.type', - 'settings.index.compound_format', - 'settings.index.compound_on_flush', - 'settings.index.warmer.enabled', + 'index.number_of_replicas', + 'index.auto_expand_replicas', + 'index.blocks.read_only', + 'index.blocks.read', + 'index.blocks.write', + 'index.blocks.metadata', + 'index.refresh_interval', + 'index.index_concurrency', + 'index.codec', + 'index.codec.bloom.load', + 'index.fail_on_merge_failure', + 'index.translog.flush_threshold_ops', + 'index.translog.flush_threshold_size', + 'index.translog.flush_threshold_period', + 'index.translog.disable_flush', + 'index.cache.filter.max_size', + 'index.cache.filter.expire', + 'index.gateway.snapshot_interval', + 'index.routing.allocation.include', + 'index.routing.allocation.exclude', + 'index.routing.allocation.require', + 'index.routing.allocation.disable_allocation', + 'index.routing.allocation.disable_new_allocation', + 'index.routing.allocation.disable_replica_allocation', + 'index.routing.allocation.enable', + 'index.routing.allocation.total_shards_per_node', + 'index.recovery.initial_shards', + 'index.gc_deletes', + 'index.ttl.disable_purge', + 'index.translog.fs.type', + 'index.compound_format', + 'index.compound_on_flush', + 'index.warmer.enabled', ]; static protected $allowedIndexCreateKeys = [ @@ -94,6 +94,7 @@ class Index protected $client; /** + * These are the Flow "Settings" aka Configuration, NOT the index settings * @var array */ protected $settings; @@ -120,7 +121,7 @@ public function __construct(string $name, Client $client = null) } /** - * Inject the settings + * Inject the framework settings * * @param array $settings * @return void @@ -190,15 +191,15 @@ public function request(string $method, string $path = null, array $arguments = */ public function create(): void { - $indexSettings = $this->getSettings(); - $creationSettings = array_filter($indexSettings, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY); - $this->request('PUT', null, [], json_encode($creationSettings)); + $indexConfiguration = $this->getConfiguration(); + $indexCreateObject = array_filter($indexConfiguration, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY); + $this->request('PUT', null, [], json_encode($indexCreateObject)); } /** * @return array|null */ - protected function getSettings(): ?array + protected function getConfiguration(): ?array { if ($this->client instanceof Client) { $path = 'indexes.' . $this->client->getBundle() . '.' . $this->settingsKey; @@ -206,8 +207,8 @@ protected function getSettings(): ?array $path = 'indexes.default' . '.' . $this->settingsKey; } - $settings = Arrays::getValueByPath($this->settings, $path); - return $settings !== null ? $this->dynamicIndexSettingService->process($settings, $path, $this->name) : $settings; + $cconfiguration = Arrays::getValueByPath($this->settings, $path); + return $cconfiguration !== null ? $this->dynamicIndexSettingService->process($cconfiguration, $path, $this->name) : $cconfiguration; } /** @@ -216,7 +217,8 @@ protected function getSettings(): ?array */ public function updateSettings(): void { - $settings = $this->getSettings(); + // we only ever need the settings path from all the settings. + $settings = $this->getConfiguration()['settings'] ?? []; $updatableSettings = []; foreach (static::$updatableSettings as $settingPath) { $setting = Arrays::getValueByPath($settings, $settingPath); @@ -289,11 +291,11 @@ public function setSettingsKey(string $settingsKey): void */ private function prefixName(): string { - $indexSettings = $this->getSettings(); - if (!isset($indexSettings['prefix']) || empty($indexSettings['prefix'])) { + $indexConfiguration = $this->getConfiguration(); + if (!isset($indexConfiguration['prefix']) || empty($indexConfiguration['prefix'])) { return $this->name; } - return $indexSettings['prefix'] . '-' . $this->name; + return $indexConfiguration['prefix'] . '-' . $this->name; } } diff --git a/Classes/Service/DynamicIndexSettingService.php b/Classes/Service/DynamicIndexSettingService.php index c9cbd45..0a952a2 100644 --- a/Classes/Service/DynamicIndexSettingService.php +++ b/Classes/Service/DynamicIndexSettingService.php @@ -20,6 +20,7 @@ /** * Transform indices settings dynamically + * FIXME: This should be called DynamicIndexConfigurationService, the "settings" represent more than what elastic index "settings" are. * * @Flow\Scope("singleton") */ diff --git a/Tests/Functional/Domain/IndexTest.php b/Tests/Functional/Domain/IndexTest.php index 7ab5bf5..671f987 100644 --- a/Tests/Functional/Domain/IndexTest.php +++ b/Tests/Functional/Domain/IndexTest.php @@ -50,10 +50,8 @@ public function indexWithoutPrefix() '/index_without_prefix/_settings', [], json_encode([ - 'settings' => [ - 'index' => [ - 'number_of_replicas' => 2 - ] + 'index' => [ + 'number_of_replicas' => 2 ] ], JSON_THROW_ON_ERROR) ] @@ -100,10 +98,8 @@ public function indexWithPrefix() '/prefix-index_with_prefix/_settings', [], json_encode([ - 'settings' => [ - 'index' => [ - 'number_of_replicas' => 1 - ] + 'index' => [ + 'number_of_replicas' => 1 ] ], JSON_THROW_ON_ERROR) ] From 2af447cdc97547dd9f04334365aa0113787f3f4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Mu=CC=88ller?= Date: Wed, 25 Jan 2023 09:56:00 +0100 Subject: [PATCH 3/4] Prevent empty arrays in request body --- Classes/Domain/Model/Index.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/Classes/Domain/Model/Index.php b/Classes/Domain/Model/Index.php index 3cc30d6..7b2f068 100644 --- a/Classes/Domain/Model/Index.php +++ b/Classes/Domain/Model/Index.php @@ -191,9 +191,9 @@ public function request(string $method, string $path = null, array $arguments = */ public function create(): void { - $indexConfiguration = $this->getConfiguration(); - $indexCreateObject = array_filter($indexConfiguration, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY); - $this->request('PUT', null, [], json_encode($indexCreateObject)); + $indexConfiguration = $this->getConfiguration() ?? []; + $indexCreateObject = is_array($indexConfiguration) ? array_filter($indexConfiguration, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY) : []; + $this->request('PUT', null, [], $this->encodeRequestBody($indexCreateObject)); } /** @@ -227,7 +227,7 @@ public function updateSettings(): void } } if ($updatableSettings !== []) { - $this->request('PUT', '/_settings', [], json_encode($updatableSettings)); + $this->request('PUT', '/_settings', [], $this->encodeRequestBody($updatableSettings)); } } @@ -298,4 +298,13 @@ private function prefixName(): string return $indexConfiguration['prefix'] . '-' . $this->name; } + + private function encodeRequestBody(array $content): string + { + if (empty($content)) { + return ''; + } + + return json_encode($content); + } } From 1070e1b4d7c1d4a76654de67973a634306fdf033 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Mu=CC=88ller?= Date: Wed, 25 Jan 2023 13:11:30 +0100 Subject: [PATCH 4/4] Code style cleanup --- Classes/Domain/Model/Index.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Classes/Domain/Model/Index.php b/Classes/Domain/Model/Index.php index 7b2f068..7923afb 100644 --- a/Classes/Domain/Model/Index.php +++ b/Classes/Domain/Model/Index.php @@ -192,7 +192,7 @@ public function request(string $method, string $path = null, array $arguments = public function create(): void { $indexConfiguration = $this->getConfiguration() ?? []; - $indexCreateObject = is_array($indexConfiguration) ? array_filter($indexConfiguration, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY) : []; + $indexCreateObject = array_filter($indexConfiguration, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY); $this->request('PUT', null, [], $this->encodeRequestBody($indexCreateObject)); } @@ -301,7 +301,7 @@ private function prefixName(): string private function encodeRequestBody(array $content): string { - if (empty($content)) { + if ($content === []) { return ''; }