Skip to content

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Sep 30, 2025

Q A
Type feature
BC Break maybe
Fixed issues PHPORM-388

Summary

When performing a search query on a search index that doesn't exist, the aggregation returns no results. This is frustrating for users that will try to optimize the query while the issue is the missing search index.

This PR adds a configuration setting (enabled by default) to check if the search index exists only when performing a search aggregation with no returned results.

@GromNaN GromNaN requested review from Copilot and jmikola September 30, 2025 12:31
@GromNaN GromNaN added this to the 2.13.0 milestone Sep 30, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds functionality to detect and report missing search indexes when Atlas Search aggregations return empty results. The feature helps developers identify when queries fail due to non-existent search indexes rather than actual empty result sets.

  • Introduces a new SchemaException class for search index validation errors
  • Adds configuration option to enable/disable search index existence checks (enabled by default)
  • Implements automatic validation of search indexes when aggregations return empty results

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
lib/Doctrine/ODM/MongoDB/SchemaException.php New exception class for search index schema errors
lib/Doctrine/ODM/MongoDB/Configuration.php Configuration methods for controlling search index validation
lib/Doctrine/ODM/MongoDB/Aggregation/Aggregation.php Core logic to validate search index existence on empty results
tests/Doctrine/ODM/MongoDB/Tests/Functional/AtlasSearchTest.php Test coverage for the new search index validation feature

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

*/
private function assertSearchIndexExistsWhenAggregationResultsIsEmpty(Iterator $iterator): void
{
if ($iterator->current() !== false) {
Copy link
Preview

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using $iterator->current() !== false to check if results are empty is incorrect. Iterator::current() returns the current element or null if empty, not a boolean. This should be $iterator->current() !== null or use $iterator->valid() to properly check if the iterator has elements.

Suggested change
if ($iterator->current() !== false) {
if ($iterator->current() !== null) {

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Copilot is correct here. Assuming we end up propagating the return value from MongoDB\Driver\Cursor::current(), it will either be an array/object or null.

Another random thought: if the pipeline happened to yield a tailable cursor (e.g. $changeStream), the result might also be empty until the first getMore; however, that also wouldn't have anything to do with a search index so we should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function uses current() which returns false when there is noting (confirmed by tests)

Replaced by ! $iterator->current(), so that we support both null|false. When there is a value, it is array|object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where a false return value comes from, but that seems incorrect. If this isn't coming from the driver, I wonder if there's a non-compliant Iterator implementation in ODM responsible for returning false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an inconsistency between the current() function and the Iterator::current() function. I'll change to use the Iterator::key() method which is more reliable to detect if there is an object, as said in the documentation note.

}

return $this->rewindable ? new CachingIterator($cursor) : new UnrewindableIterator($cursor);
$iterator = $this->rewindable ? new CachingIterator($cursor) : new UnrewindableIterator($cursor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted that both CachingIterator and UnrewindableIterator rewind the inner iterator upon construction, so it's kosher to immediately call current() in the assertion function; however, you should probably make a note of this.

I think the best way to capture this would be to use union types on the assertion method's signature and add a docblock there that notes the point above.

Otherwise, we could not be certain that an arbitrary Iterator instance would be rewound already and capable of accessing current().

*/
private function assertSearchIndexExistsWhenAggregationResultsIsEmpty(Iterator $iterator): void
{
if ($iterator->current() !== false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Copilot is correct here. Assuming we end up propagating the return value from MongoDB\Driver\Cursor::current(), it will either be an array/object or null.

Another random thought: if the pipeline happened to yield a tailable cursor (e.g. $changeStream), the result might also be empty until the first getMore; however, that also wouldn't have anything to do with a search index so we should be fine.

@GromNaN GromNaN force-pushed the PHPORM-388 branch 2 times, most recently from 3e029e9 to bd7f87b Compare October 1, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants