Prepare field names for search indexes and vector search indexes#2831
Prepare field names for search indexes and vector search indexes#2831GromNaN merged 9 commits intodoctrine:2.13.xfrom
Conversation
GromNaN
force-pushed
the
PHPORM-391
branch
2 times, most recently
from
b844e6e to
bb9f3f2
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces field name mapping support for MongoDB search and vector search indexes, ensuring that field references in index definitions are properly mapped to their database counterparts when they differ from property names in the ODM.
- Adds a
prepareSearchIndexesmethod inSchemaManagerto map field names in search index definitions - Updates search and vector search stages to prepare field paths using the document persister
- Enhances test coverage to verify field name mapping functionality
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/Doctrine/ODM/MongoDB/SchemaManager.php |
Adds field name preparation for search indexes |
lib/Doctrine/ODM/MongoDB/Aggregation/Stage/VectorSearch.php |
Updates to prepare field paths in vector search |
lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Search.php |
Updates to prepare field paths and sort fields in search |
lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Search/AbstractSearchOperator.php |
Adds base field path preparation functionality |
lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php |
Updates to pass document persister to search stages |
tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php |
Adds comprehensive tests for field name mapping |
tests/Documents/CmsArticle.php |
Test document with mapped field names |
tests/Documents/VectorEmbedding.php |
Test document with vector field mapping |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Search/MoreLikeThis.php
Outdated
Show resolved
Hide resolved
| 'createOperator' => static function (Search|CompoundSearchOperatorInterface|SupportsEmbeddableSearchOperators $stage) { | ||
| return $stage->moreLikeThis(['disabledAt' => new DateTime('2020-01-01T00:00:00Z')]); | ||
| }, | ||
| 'className' => User::class, |
There was a problem hiding this comment.
Switching to an other document class to validate transformation of the date value.
lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Search/MoreLikeThis.php
Outdated
Show resolved
Hide resolved
| public function autocomplete(string $path = '', string ...$query): Autocomplete&CompoundSearchOperatorInterface | ||
| { | ||
| return $this->addOperator(new CompoundedAutocomplete($this->getCompoundStage(), $this->getAddOperatorClosure(), $this->getSearchStage(), $path, ...$query)); | ||
| return $this->addOperator(new CompoundedAutocomplete($this->getCompoundStage(), $this->getAddOperatorClosure(), $this->getSearchStage(), $this->getDocumentPersister(), $path, ...$query)); |
There was a problem hiding this comment.
Noted that this class extends Autocomplete, which had its constructor modified to accept a DocumentPersister. I assume the same is true for other classes in this file, which are themselves absent from this PR.
| * } | ||
| * } | ||
| */ | ||
| class Search extends Stage implements SupportsAllSearchOperators |
There was a problem hiding this comment.
I noted that this doesn't extended the abstract class earlier in the PR, which means you had to modify the constructor and introduce a getDocumentPersister() method.
Out of curiosity, why can't it inherit the base class like other stages?
There was a problem hiding this comment.
I think because we don't want that the Search class implement SearchOperator, and the methods implemented by AbstractSearchOperator delegate to the Search class, while the Search class modifies its own state.
| ]) | ||
| ->willReturn(['search_articles']); | ||
| ->with($this->anything()) | ||
| ->willReturnCallback(function (array $indexes) { |
There was a problem hiding this comment.
Just to confirm: you're mocking the response here to avoid requiring an Atlas cluster, correct?
There was a problem hiding this comment.
Yes, that's a unit test that doesn't depend on a MongoDB server.
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
Outstanding question in Compound.php, which may apply to any other classes that had a similar behavior of assigning their own private DocumentPersister instance.
Also, a question in Search.php wasn't addressed. No changes required there but an answer would help my understanding.
I'll LGTM as the "private DocumentPersister" cleanup is straightforward.
| * } | ||
| * } | ||
| */ | ||
| class Search extends Stage implements SupportsAllSearchOperators |
Summary
Map field names in search and vector search index definitions. Since the
$searchand$vectorSearchstages must always be first, we know there is no field name mapped from a previous stage.