Merge pull request #107 from Flowpack/bugfix/cleanup-prefix-setting

BUGFIX: Setting injection doesn't cause errors

Classes/Domain/Model/Index.php

@@ -64,6 +64,12 @@ class Index
         'index.warmer.enabled',
     ];
 
+    static protected $allowedIndexCreateKeys = [
+        'settings',
+        'aliases',
+        'mappings'
+    ];
+
     /**
      * @var DynamicIndexSettingService
      * @Flow\Inject
@@ -88,6 +94,7 @@ class Index
     protected $client;
 
     /**
+     * These are the Flow "Settings" aka Configuration, NOT the index settings
      * @var array
      */
     protected $settings;
@@ -114,21 +121,14 @@ public function __construct(string $name, Client $client = null)
     }
 
     /**
-     * Inject the settings
+     * Inject the framework settings
      *
      * @param array $settings
      * @return void
      */
     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 +141,7 @@ public function findType(string $typeName): AbstractType
     }
 
     /**
-     * @param array <AbstractType> $types
+     * @param array<AbstractType> $types
      * @return TypeGroup
      */
     public function findTypeGroup(array $types): TypeGroup
@@ -173,11 +173,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,22 +191,24 @@ public function request(string $method, string $path = null, array $arguments =
      */
     public function create(): void
     {
-        $this->request('PUT', null, [], json_encode($this->getSettings()));
+        $indexConfiguration = $this->getConfiguration() ?? [];
+        $indexCreateObject = array_filter($indexConfiguration, static fn($key) => in_array($key, self::$allowedIndexCreateKeys, true), ARRAY_FILTER_USE_KEY);
+        $this->request('PUT', null, [], $this->encodeRequestBody($indexCreateObject));
     }
 
     /**
      * @return array|null
      */
-    protected function getSettings(): ?array
+    protected function getConfiguration(): ?array
     {
         if ($this->client instanceof Client) {
             $path = 'indexes.' . $this->client->getBundle() . '.' . $this->settingsKey;
         } else {
             $path = 'indexes.default' . '.' . $this->settingsKey;
         }
 
-        $settings = Arrays::getValueByPath($this->settings, $path);
-        return $settings !== null ? $this->dynamicIndexSettingService->process($settings, $path, $this->getName()) : $settings;
+        $cconfiguration = Arrays::getValueByPath($this->settings, $path);
+        return $cconfiguration !== null ? $this->dynamicIndexSettingService->process($cconfiguration, $path, $this->name) : $cconfiguration;
     }
 
     /**
@@ -215,15 +217,18 @@ 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);
             if ($setting !== null) {
                 $updatableSettings = Arrays::setValueByPath($updatableSettings, $settingPath, $setting);
             }
         }
-        $this->request('PUT', '/_settings', [], json_encode($updatableSettings));
+        if ($updatableSettings !== []) {
+            $this->request('PUT', '/_settings', [], $this->encodeRequestBody($updatableSettings));
+        }
     }
 
     /**
@@ -252,6 +257,11 @@ public function refresh(): Response
      * @return string
      */
     public function getName(): string
+    {
+        return $this->prefixName();
+    }
+
+    public function getOriginalName(): string
     {
         return $this->name;
     }
@@ -273,4 +283,28 @@ public function setSettingsKey(string $settingsKey): void
     {
         $this->settingsKey = $settingsKey;
     }
+
+    /**
+     * Prepends configured preset to the base index name
+     *
+     * @return string
+     */
+    private function prefixName(): string
+    {
+        $indexConfiguration = $this->getConfiguration();
+        if (!isset($indexConfiguration['prefix']) || empty($indexConfiguration['prefix'])) {
+            return $this->name;
+        }
+
+        return $indexConfiguration['prefix'] . '-' . $this->name;
+    }
+
+    private function encodeRequestBody(array $content): string
+    {
+        if ($content === []) {
+            return '';
+        }
+
+        return json_encode($content);
+    }
 }

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")
  */

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

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()) {

Tests/Functional/Domain/IndexTest.php

@@ -0,0 +1,116 @@
+<?php
+namespace Flowpack\ElasticSearch\Tests\Functional\Domain;
+
+/*
+ * This file is part of the Flowpack.ElasticSearch package.
+ *
+ * (c) Contributors of the Flowpack Team - flowpack.org
+ *
+ * This package is Open Source Software. For the full copyright and license
+ * information, please view the LICENSE file which was distributed with this
+ * source code.
+ */
+
+use Flowpack\ElasticSearch\Domain\Model\Client;
+use Flowpack\ElasticSearch\Transfer\Response;
+use Neos\Flow\Tests\FunctionalTestCase;
+use Flowpack\ElasticSearch\Domain\Model\Index;
+
+class IndexTest extends FunctionalTestCase
+{
+    /**
+     * @test
+     */
+    public function indexWithoutPrefix()
+    {
+        $clientMock = $this->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([
+                        '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([
+                        '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());
+    }
+}