Fixed issues after code review

mikadamczyk · mikadamczyk · commit 7cc107cfa12a · 2025-10-15T15:14:14.000+02:00
diff --git a/src/bundle/Core/Resources/config/embeddings.yaml b/src/bundle/Core/Resources/config/embeddings.yaml
diff --git a/src/bundle/Core/Resources/config/services.yml b/src/bundle/Core/Resources/config/services.yml
@@ -1,6 +1,6 @@
 imports:
     - { resource: commands.yml }
-    - { resource: embeddings.yml }
+    - { resource: embeddings.yaml }
 
 parameters:
     ibexa.site_access.default.name: default
diff --git a/src/contracts/Repository/Values/Content/EmbeddingQuery.php b/src/contracts/Repository/Values/Content/EmbeddingQuery.php
@@ -12,10 +12,7 @@
 use Ibexa\Contracts\Core\Repository\Values\Content\Query\Embedding;
 use InvalidArgumentException;
 
-/**
- * This class is used to perform an embedding query.
- */
-class EmbeddingQuery extends Query
+final class EmbeddingQuery extends Query
 {
     private ?Embedding $embedding = null;
 
@@ -29,7 +26,7 @@ public function setEmbedding(?Embedding $embedding): void
         $this->embedding = $embedding;
     }
 
-    public function getFilter(): ?Criterion
+    public function getFilter(): Criterion
     {
         return $this->filter;
     }
@@ -105,7 +102,7 @@ public function isValid(): bool
         if (count($invalid) > 0) {
             throw new InvalidArgumentException(
                 sprintf(
-                    'EmbeddingQuery may not set [%s].',
+                    'EmbeddingQuery did not set [%s].',
                     implode(', ', $invalid)
                 )
             );
diff --git a/src/contracts/Repository/Values/Content/EmbeddingQueryBuilder.php b/src/contracts/Repository/Values/Content/EmbeddingQueryBuilder.php
@@ -13,11 +13,21 @@
 
 final class EmbeddingQueryBuilder
 {
-    private EmbeddingQuery $query;
+    private ?Embedding $embedding = null;
+
+    private ?int $limit = null;
+
+    private ?int $offset = null;
+
+    private ?Criterion $filter = null;
+
+    /** @var array<\Ibexa\Contracts\Core\Repository\Values\Content\Query\Aggregation> */
+    private array $aggregations = [];
+
+    private bool $performCount = false;
 
     private function __construct()
     {
-        $this->query = new EmbeddingQuery();
     }
 
     public static function create(): self
@@ -27,28 +37,28 @@ public static function create(): self
 
     public function withEmbedding(Embedding $embed): self
     {
-        $this->query->setEmbedding($embed);
+        $this->embedding = $embed;
 
         return $this;
     }
 
     public function setLimit(int $limit): self
     {
-        $this->query->setLimit($limit);
+        $this->limit = $limit;
 
         return $this;
     }
 
     public function setOffset(int $offset): self
     {
-        $this->query->setOffset($offset);
+        $this->offset = $offset;
 
         return $this;
     }
 
     public function setFilter(Criterion $filter): self
     {
-        $this->query->setFilter($filter);
+        $this->filter = $filter;
 
         return $this;
     }
@@ -58,20 +68,44 @@ public function setFilter(Criterion $filter): self
      */
     public function setAggregations(array $aggregations): self
     {
-        $this->query->setAggregations($aggregations);
+        $this->aggregations = $aggregations;
 
         return $this;
     }
 
     public function setPerformCount(bool $performCount): self
     {
-        $this->query->setPerformCount($performCount);
+        $this->performCount = $performCount;
 
         return $this;
     }
 
     public function build(): EmbeddingQuery
     {
-        return $this->query;
+        $query = new EmbeddingQuery();
+
+        if ($this->embedding !== null) {
+            $query->setEmbedding($this->embedding);
+        }
+
+        if ($this->limit !== null) {
+            $query->setLimit($this->limit);
+        }
+
+        if ($this->offset !== null) {
+            $query->setOffset($this->offset);
+        }
+
+        if ($this->filter !== null) {
+            $query->setFilter($this->filter);
+        }
+
+        if (!empty($this->aggregations)) {
+            $query->setAggregations($this->aggregations);
+        }
+
+        $query->setPerformCount($this->performCount);
+
+        return $query;
     }
 }
diff --git a/src/contracts/Repository/Values/Content/Query/Embedding.php b/src/contracts/Repository/Values/Content/Query/Embedding.php
@@ -23,7 +23,9 @@ public function __construct(array $value)
         $this->value = $value;
     }
 
-    /** @return float[] */
+    /**
+     * @return float[]
+     */
     public function getValue(): array
     {
         return $this->value;
diff --git a/src/contracts/Repository/Values/Content/QueryValidatorInterface.php b/src/contracts/Repository/Values/Content/QueryValidatorInterface.php
@@ -4,6 +4,8 @@
  * @copyright Copyright (C) Ibexa AS. All rights reserved.
  * @license For full copyright and license information view LICENSE file distributed with this source code.
  */
+declare(strict_types=1);
+
 namespace Ibexa\Contracts\Core\Repository\Values\Content;
 
 interface QueryValidatorInterface
diff --git a/src/contracts/Search/Embedding/EmbeddingResolverNotFoundException.php b/src/contracts/Search/Embedding/EmbeddingResolverNotFoundException.php
@@ -4,6 +4,8 @@
  * @copyright Copyright (C) Ibexa AS. All rights reserved.
  * @license For full copyright and license information view LICENSE file distributed with this source code.
  */
+declare(strict_types=1);
+
 namespace Ibexa\Contracts\Core\Search\Embedding;
 
 use RuntimeException;
diff --git a/src/lib/Search/Common/FieldValueMapper/EmbeddingMapper.php b/src/lib/Search/Common/FieldValueMapper/EmbeddingMapper.php
@@ -13,7 +13,7 @@
 use Ibexa\Core\Search\Common\FieldValueMapper;
 
 /**
- * @internal for internal use by Search engine field value mapper
+ * @internal for internal use by search engine field value mapper
  */
 final class EmbeddingMapper extends FieldValueMapper
 {
diff --git a/src/lib/Search/Embedding/EmbeddingConfiguration.php b/src/lib/Search/Embedding/EmbeddingConfiguration.php
@@ -60,7 +60,7 @@ public function getDefaultEmbeddingModelIdentifier(): string
     }
 
     /**
-     * @return array{name: string, dimensions: int, field_suffix: string, 'embedding_provider': string}
+     * @return array{name: string, dimensions: int, field_suffix: string, embedding_provider: string}
      */
     public function getDefaultEmbeddingModel(): array
     {
diff --git a/tests/integration/Core/Repository/Values/Content/EmbeddingQueryBuilderTest.php b/tests/integration/Core/Repository/Values/Content/EmbeddingQueryBuilderTest.php
@@ -35,19 +35,19 @@ public function testBuilderSetsAllowedProperties(): void
 
         $query = $builder->build();
 
-        $this->assertSame(
+        self::assertSame(
             $embedding,
             $query->getEmbedding(),
             'Embedding should be set by builder'
         );
 
-        $this->assertEquals(10, $query->getLimit(), 'Limit should be set by builder');
-        $this->assertEquals(5, $query->getOffset(), 'Offset should be set by builder');
-        $this->assertTrue($query->getPerformCount(), 'PerformCount flag should be true');
+        self::assertEquals(10, $query->getLimit(), 'Limit should be set by builder');
+        self::assertEquals(5, $query->getOffset(), 'Offset should be set by builder');
+        self::assertTrue($query->getPerformCount(), 'PerformCount flag should be true');
 
         $aggregations = $query->getAggregations();
-        $this->assertIsArray($aggregations, 'Aggregations must be array');
-        $this->assertCount(2, $aggregations, 'Two aggregations added');
+        self::assertIsArray($aggregations, 'Aggregations must be array');
+        self::assertCount(2, $aggregations, 'Two aggregations added');
     }
 
     public function testIsValidReturnsTrueForCleanQuery(): void
@@ -56,7 +56,7 @@ public function testIsValidReturnsTrueForCleanQuery(): void
             ->withEmbedding($this->createMock(Embedding::class))
             ->build();
 
-        $this->assertTrue($query->isValid());
+        self::assertTrue($query->isValid());
     }
 
     public function testSettingSortClausesThenIsValidThrows(): void
@@ -65,15 +65,26 @@ public function testSettingSortClausesThenIsValidThrows(): void
             ->withEmbedding($this->createMock(Embedding::class))
             ->build();
 
-        // bypass setter via array-append magic
         $query->sortClauses[] = new ContentName(BaseQuery::SORT_ASC);
         $query->query = $this->createMock(Criterion::class);
         $query->facetBuilders = [$this->createMock(FacetBuilder::class)];
         $query->spellcheck = new Spellcheck('foo');
 
         $this->expectException(InvalidArgumentException::class);
-        $this->expectExceptionMessage('EmbeddingQuery may not set [query, sortClauses, facetBuilders, spellcheck].');
+        $this->expectExceptionMessage('EmbeddingQuery did not set [query, sortClauses, facetBuilders, spellcheck].');
 
         $query->isValid();
     }
+
+    public function testBuildReturnsNewInstance(): void
+    {
+        $builder = EmbeddingQueryBuilder::create();
+
+        $originalQuery = $builder->build();
+        $builder->setPerformCount(true);
+        $secondQuery = $builder->build();
+
+        self::assertNotSame($originalQuery, $secondQuery);
+        self::assertNotEquals($originalQuery->getPerformCount(), $secondQuery->getPerformCount());
+    }
 }
diff --git a/tests/integration/Core/Search/FieldType/EmbeddingFieldFactoryTest.php b/tests/integration/Core/Search/FieldType/EmbeddingFieldFactoryTest.php
@@ -19,15 +19,15 @@ public function testCreateUsesConfigSuffix(): void
         $suffix = 'model_123';
         $config = $this->createMock(EmbeddingConfigurationInterface::class);
         $config
-            ->expects($this->once())
+            ->expects(self::once())
             ->method('getDefaultEmbeddingModelFieldSuffix')
             ->willReturn($suffix);
 
         $factory = new EmbeddingFieldFactory($config);
 
         $field = $factory->create();
 
-        $this->assertSame(
+        self::assertSame(
             'ibexa_dense_vector_model_123',
             $field->getType(),
             'Factory should prepend "ibexa_dense_vector_" to the suffix from the config'
@@ -46,7 +46,7 @@ public function testCreateWithCustomType(): void
 
         $field = $factory->create($customType);
 
-        $this->assertSame(
+        self::assertSame(
             $customType,
             $field->getType(),
             'Factory should use the explicit type when provided'
diff --git a/tests/lib/Search/Embedding/EmbeddingConfigurationTest.php b/tests/lib/Search/Embedding/EmbeddingConfigurationTest.php
@@ -43,7 +43,7 @@ public function testGetDefaultEmbeddingModel(): void
                 ['embedding_models', null, null, self::MODELS],
             ]);
 
-        $this->assertSame(
+        self::assertSame(
             ['name' => 'text-embedding-ada-002', 'dimensions' => 1536, 'field_suffix' => 'ada002', 'embedding_provider' => 'ibexa_openai'],
             $this->config->getDefaultEmbeddingModel()
         );
@@ -58,7 +58,7 @@ public function testGetEmbeddingModelIdentifiers(): void
                 ['embedding_models', null, null, self::MODELS],
             ]);
 
-        $this->assertSame(
+        self::assertSame(
             ['text-embedding-3-small', 'text-embedding-3-large', 'text-embedding-ada-002'],
             $this->config->getEmbeddingModelIdentifiers()
         );
@@ -71,8 +71,8 @@ public function testGetEmbeddingModels(): void
             ->with('embedding_models')
             ->willReturn(self::MODELS);
 
-        $this->assertSame(self::MODELS, $this->config->getEmbeddingModels());
-        $this->assertSame(
+        self::assertSame(self::MODELS, $this->config->getEmbeddingModels());
+        self::assertSame(
             ['text-embedding-3-small', 'text-embedding-3-large', 'text-embedding-ada-002'],
             $this->config->getEmbeddingModelIdentifiers()
         );
@@ -85,7 +85,7 @@ public function testGetEmbeddingModel(): void
             ->with('embedding_models')
             ->willReturn(self::MODELS);
 
-        $this->assertSame(
+        self::assertSame(
             ['name' => 'text-embedding-ada-002', 'dimensions' => 1536, 'field_suffix' => 'ada002', 'embedding_provider' => 'ibexa_openai'],
             $this->config->getEmbeddingModel('text-embedding-ada-002')
         );
@@ -98,8 +98,8 @@ public function testGetEmbeddingModelWillThrowException(): void
             ->with('embedding_models')
             ->willReturn(self::MODELS);
 
-        self::expectException(InvalidArgumentException::class);
-        self::expectExceptionMessage('Embedding model "non-existing-model" is not configured.');
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('Embedding model "non-existing-model" is not configured.');
 
         $this->config->getEmbeddingModel('non-existing-model');
     }
@@ -111,7 +111,7 @@ public function testGetDefaultEmbeddingModelIdentifier(): void
             ->with('default_embedding_model')
             ->willReturn('text-embedding-ada-002');
 
-        $this->assertSame('text-embedding-ada-002', $this->config->getDefaultEmbeddingModelIdentifier());
+        self::assertSame('text-embedding-ada-002', $this->config->getDefaultEmbeddingModelIdentifier());
     }
 
     public function testGetDefaultEmbeddingProvider(): void
@@ -123,7 +123,7 @@ public function testGetDefaultEmbeddingProvider(): void
                 ['embedding_models', null, null, self::MODELS],
             ]);
 
-        $this->assertSame('ibexa_openai', $this->config->getDefaultEmbeddingProvider());
+        self::assertSame('ibexa_openai', $this->config->getDefaultEmbeddingProvider());
     }
 
     public function getDefaultEmbeddingModelFieldSuffix(): void
@@ -135,6 +135,6 @@ public function getDefaultEmbeddingModelFieldSuffix(): void
                 ['embedding_models', null, null, self::MODELS],
             ]);
 
-        $this->assertSame('ada002', $this->config->getDefaultEmbeddingModelFieldSuffix());
+        self::assertSame('ada002', $this->config->getDefaultEmbeddingModelFieldSuffix());
     }
 }
diff --git a/tests/lib/Search/Embedding/EmbeddingProviderResolverTest.php b/tests/lib/Search/Embedding/EmbeddingProviderResolverTest.php
@@ -56,7 +56,7 @@ public function testResolveReturnsProviderWhenAvailable(): void
 
         $resolved = $this->resolver->resolve();
 
-        $this->assertSame($mockProvider, $resolved);
+        self::assertSame($mockProvider, $resolved);
     }
 
     public function testResolveThrowsWhenProviderMissing(): void

Original file line number	Diff line number	Diff line change
`@@ -12,10 +12,7 @@`
`12`	`12`	`use Ibexa\Contracts\Core\Repository\Values\Content\Query\Embedding;`
`13`	`13`	`use InvalidArgumentException;`
`14`	`14`
`15`		`-/**`
`16`		`- * This class is used to perform an embedding query.`
`17`		`- */`
`18`		`-class EmbeddingQuery extends Query`
	`15`	`+final class EmbeddingQuery extends Query`
`19`	`16`	`{`
`20`	`17`	`private ?Embedding $embedding = null;`
`21`	`18`
`@@ -29,7 +26,7 @@ public function setEmbedding(?Embedding $embedding): void`
`29`	`26`	`$this->embedding = $embedding;`
`30`	`27`	`}`
`31`	`28`
`32`		`- public function getFilter(): ?Criterion`
	`29`	`+ public function getFilter(): Criterion`
`33`	`30`	`{`
`34`	`31`	`return $this->filter;`
`35`	`32`	`}`
`@@ -105,7 +102,7 @@ public function isValid(): bool`
`105`	`102`	`if (count($invalid) > 0) {`
`106`	`103`	`throw new InvalidArgumentException(`
`107`	`104`	`sprintf(`
`108`		`- 'EmbeddingQuery may not set [%s].',`
	`105`	`+ 'EmbeddingQuery did not set [%s].',`
`109`	`106`	`implode(', ', $invalid)`
`110`	`107`	`)`
`111`	`108`	`);`
Original file line number	Diff line number	Diff line change
`@@ -13,11 +13,21 @@`
`13`	`13`
`14`	`14`	`final class EmbeddingQueryBuilder`
`15`	`15`	`{`
`16`		`- private EmbeddingQuery $query;`
	`16`	`+ private ?Embedding $embedding = null;`
	`17`	`+`
	`18`	`+ private ?int $limit = null;`
	`19`	`+`
	`20`	`+ private ?int $offset = null;`
	`21`	`+`
	`22`	`+ private ?Criterion $filter = null;`
	`23`	`+`
	`24`	`+ /** @var array<\Ibexa\Contracts\Core\Repository\Values\Content\Query\Aggregation> */`
	`25`	`+ private array $aggregations = [];`
	`26`	`+`
	`27`	`+ private bool $performCount = false;`
`17`	`28`
`18`	`29`	`private function __construct()`
`19`	`30`	`{`
`20`		`- $this->query = new EmbeddingQuery();`
`21`	`31`	`}`
`22`	`32`
`23`	`33`	`public static function create(): self`
`@@ -27,28 +37,28 @@ public static function create(): self`
`27`	`37`
`28`	`38`	`public function withEmbedding(Embedding $embed): self`
`29`	`39`	`{`
`30`		`- $this->query->setEmbedding($embed);`
	`40`	`+ $this->embedding = $embed;`
`31`	`41`
`32`	`42`	`return $this;`
`33`	`43`	`}`
`34`	`44`
`35`	`45`	`public function setLimit(int $limit): self`
`36`	`46`	`{`
`37`		`- $this->query->setLimit($limit);`
	`47`	`+ $this->limit = $limit;`
`38`	`48`
`39`	`49`	`return $this;`
`40`	`50`	`}`
`41`	`51`
`42`	`52`	`public function setOffset(int $offset): self`
`43`	`53`	`{`
`44`		`- $this->query->setOffset($offset);`
	`54`	`+ $this->offset = $offset;`
`45`	`55`
`46`	`56`	`return $this;`
`47`	`57`	`}`
`48`	`58`
`49`	`59`	`public function setFilter(Criterion $filter): self`
`50`	`60`	`{`
`51`		`- $this->query->setFilter($filter);`
	`61`	`+ $this->filter = $filter;`
`52`	`62`
`53`	`63`	`return $this;`
`54`	`64`	`}`
`@@ -58,20 +68,44 @@ public function setFilter(Criterion $filter): self`
`58`	`68`	`*/`
`59`	`69`	`public function setAggregations(array $aggregations): self`
`60`	`70`	`{`
`61`		`- $this->query->setAggregations($aggregations);`
	`71`	`+ $this->aggregations = $aggregations;`
`62`	`72`
`63`	`73`	`return $this;`
`64`	`74`	`}`
`65`	`75`
`66`	`76`	`public function setPerformCount(bool $performCount): self`
`67`	`77`	`{`
`68`		`- $this->query->setPerformCount($performCount);`
	`78`	`+ $this->performCount = $performCount;`
`69`	`79`
`70`	`80`	`return $this;`
`71`	`81`	`}`
`72`	`82`
`73`	`83`	`public function build(): EmbeddingQuery`
`74`	`84`	`{`
`75`		`- return $this->query;`
	`85`	`+ $query = new EmbeddingQuery();`
	`86`	`+`
	`87`	`+ if ($this->embedding !== null) {`
	`88`	`+ $query->setEmbedding($this->embedding);`
	`89`	`+ }`
	`90`	`+`
	`91`	`+ if ($this->limit !== null) {`
	`92`	`+ $query->setLimit($this->limit);`
	`93`	`+ }`
	`94`	`+`
	`95`	`+ if ($this->offset !== null) {`
	`96`	`+ $query->setOffset($this->offset);`
	`97`	`+ }`
	`98`	`+`
	`99`	`+ if ($this->filter !== null) {`
	`100`	`+ $query->setFilter($this->filter);`
	`101`	`+ }`
	`102`	`+`
	`103`	`+ if (!empty($this->aggregations)) {`
	`104`	`+ $query->setAggregations($this->aggregations);`
	`105`	`+ }`
	`106`	`+`
	`107`	`+ $query->setPerformCount($this->performCount);`
	`108`	`+`
	`109`	`+ return $query;`
`76`	`110`	`}`
`77`	`111`	`}`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,9 @@ public function __construct(array $value)`
`23`	`23`	`$this->value = $value;`
`24`	`24`	`}`
`25`	`25`
`26`		`- /** @return float[] */`
	`26`	`+ /**`
	`27`	`+ * @return float[]`
	`28`	`+ */`
`27`	`29`	`public function getValue(): array`
`28`	`30`	`{`
`29`	`31`	`return $this->value;`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@`
`13`	`13`	`use Ibexa\Core\Search\Common\FieldValueMapper;`
`14`	`14`
`15`	`15`	`/**`
`16`		`- * @internal for internal use by Search engine field value mapper`
	`16`	`+ * @internal for internal use by search engine field value mapper`
`17`	`17`	`*/`
`18`	`18`	`final class EmbeddingMapper extends FieldValueMapper`
`19`	`19`	`{`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ public function getDefaultEmbeddingModelIdentifier(): string`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`/**`
`63`		`- * @return array{name: string, dimensions: int, field_suffix: string, 'embedding_provider': string}`
	`63`	`+ * @return array{name: string, dimensions: int, field_suffix: string, embedding_provider: string}`
`64`	`64`	`*/`
`65`	`65`	`public function getDefaultEmbeddingModel(): array`
`66`	`66`	`{`