Skip to content

Conversation

@GromNaN
Copy link
Member

@GromNaN GromNaN commented Nov 5, 2025

Copilot AI review requested due to automatic review settings November 5, 2025 19:13
@GromNaN GromNaN requested a review from a team as a code owner November 5, 2025 19:13
@GromNaN GromNaN requested a review from jmikola November 5, 2025 19:13
Copy link
Contributor

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 introduces a more specific exception (SearchNotSupportedException) to handle Atlas Search-related errors that occur during aggregation operations. The change improves error handling by catching generic server exceptions and re-throwing them as a more specific exception type when they relate to unsupported search functionality.

  • Created a new SearchNotSupportedException class to identify and handle search-related server errors
  • Modified the Aggregate operation to catch and re-throw search-specific exceptions during command execution

Reviewed Changes

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

File Description
src/Exception/SearchNotSupportedException.php New exception class that extends ServerException and provides logic to identify search-related errors based on error codes
src/Operation/Aggregate.php Added try-catch block to intercept ServerException and re-throw as SearchNotSupportedException when appropriate

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@GromNaN GromNaN force-pushed the PHPLIB-1689 branch 2 times, most recently from f9c8674 to 604ef77 Compare November 5, 2025 19:16
@GromNaN GromNaN requested a review from Copilot November 5, 2025 19:16
Copy link
Contributor

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 76.08696% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.63%. Comparing base (82719c4) to head (1348e94).
⚠️ Report is 1 commits behind head on v2.x.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/Exception/SearchNotSupportedException.php 68.96% 9 Missing ⚠️
src/Operation/CreateSearchIndexes.php 80.00% 1 Missing ⚠️
src/Operation/UpdateSearchIndex.php 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               v2.x    #1794      +/-   ##
============================================
- Coverage     87.74%   87.63%   -0.11%     
- Complexity     3185     3196      +11     
============================================
  Files           424      425       +1     
  Lines          6289     6332      +43     
============================================
+ Hits           5518     5549      +31     
- Misses          771      783      +12     
Flag Coverage Δ
6.0-replica_set 86.41% <76.08%> (+0.69%) ⬆️
6.0-server 82.43% <76.08%> (+0.72%) ⬆️
6.0-sharded_cluster 86.21% <76.08%> (+0.69%) ⬆️
8.0-replica_set 87.49% <67.39%> (-0.17%) ⬇️
8.0-server 83.21% <67.39%> (+0.65%) ⬆️
8.0-sharded_cluster 87.33% <67.39%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GromNaN GromNaN force-pushed the PHPLIB-1689 branch 2 times, most recently from 9d13244 to 17b1c47 Compare November 10, 2025 09:51
@GromNaN GromNaN force-pushed the PHPLIB-1689 branch 2 times, most recently from 14c3251 to 50fbc84 Compare November 18, 2025 17:05
@GromNaN
Copy link
Member Author

GromNaN commented Dec 2, 2025

Since mongo 6.0.11-ent, the $listSearchIndexes stage returns nothing, while the previous versions threw a server error. This makes it more complicated to test and detect.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

@GromNaN: Was there anything in this PR you wanted to keep, or should it all be abandoned (given the difficulty in error detection)?

@GromNaN GromNaN force-pushed the PHPLIB-1689 branch 2 times, most recently from 088f9ae to 53e56ee Compare December 17, 2025 17:46
@GromNaN GromNaN requested review from Copilot and jmikola December 17, 2025 17:47
Copy link
Contributor

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

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@GromNaN
Copy link
Member Author

GromNaN commented Dec 17, 2025

I found a more reliable way to detect when Search indexes are supported.
And I made the test pass weither search indexes are supported or not: if there is an exception it must be a SearchIndexNotSupportedException.

Also fixed the exception for update and drop search index.

@GromNaN GromNaN force-pushed the PHPLIB-1689 branch 2 times, most recently from 9c70230 to 80a2c92 Compare December 19, 2025 13:44
@GromNaN GromNaN requested a review from Copilot December 19, 2025 13:45
Copy link
Contributor

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

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// MongoDB 7-ent: Search index commands are only supported with Atlas.
115 => true,
// MongoDB 4 to 6, 7-community
59 => match ($exception->getMessage()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

match inception 🤯

Copy link
Member Author

Choose a reason for hiding this comment

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

A decision tree 🎄

default => false,
},
// MongoDB 4 to 6
40324 => match ($exception->getMessage()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you trawl through all the old versions' source code to find all these error codes? 😭

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried many server versions using mongo-orchestration, m and mongo-launch; Atlas M0 and Atlas Local Docker.
Then our test matrix helped validate all the versions.

try {
$cursor = $this->executeCommand($server, $command);
} catch (ServerException $exception) {
if (SearchNotSupportedException::isSearchNotSupportedError($exception)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice solution


try {
$collection->listSearchIndexes();
} catch (SearchNotSupportedException) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Basically are you doing it like these because it's too complex to skip if it is supported? Can't we just do the inverse of the check you do in skipIfSearchIndexIsNotSupported and expect an exception?

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 is a way to test even if skipIfSearchIndexIsNotSupported is incomplete. Here, I test each operation one by one to ensure that if there is an exception, it is properly wrapped.

I noticed that not all server versions are consistent. As an example in mongo 6.0.11-ent, the $listSearchIndexes stage returns an empty list but createSearchIndexes returns an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants