Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle search index exceptions if possible #2671

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Jul 31, 2024

Q A
Type bug
BC Break no
Fixed issues #2634 (comment)

Summary

#2630 introduces support for managing search indexes in the schema manager. However, the logic does not correctly handle cases when the server does not support search indexes. This PR adds more graceful handling in cases where no search index changes are necessary. Here's a list of the changes:

  • creating search indexes: no changes; if search indexes are declared on a class and the command to create them fails, we expect an exception to be thrown
  • updating search indexes: if no search indexes are declared on a class, failure of the listSearchIndexes command will no longer cause an exception. If a class has search indexes, the old behaviour still applies
  • deleting search indexes: failure of the listSearchIndexes command no longer throws an exception, since there are no search indexes to be removed.

We specifically check for error code 115 (CommandNotSupported) and check the exception message to make sure that it's actually an issue with the search index command, as there are no good ways to check for search index support.

@nikophil
Copy link

nikophil commented Jul 31, 2024

hi @alcaeus, I would be happy to help, but it seems I cannot test your PR since my app requires doctrine/mongodb-odm-bundle which depends on this lib. So when i add your repo as a new vcs repository in composer.json, composer does not accept to update to dev-fix-search-indexes:

  Problem 1
    - Root composer.json requires doctrine/mongodb-odm dev-fix-search-indexes, found doctrine/mongodb-odm[dev-fix-search-indexes] but these were not loaded, likely because it conflicts with another require.
  Problem 2
    - doctrine/mongodb-odm-bundle is locked to version 5.0.1 and an update of this package was not requested.
    - doctrine/mongodb-odm-bundle 5.0.1 requires doctrine/mongodb-odm ^2.6 -> satisfiable by doctrine/mongodb-odm[2.6.0, ..., 2.9.x-dev] from vcs repo (github https://github.com/alcaeus/mongodb-odm) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.

@alcaeus
Copy link
Member Author

alcaeus commented Jul 31, 2024

So when i add your repo as a new vcs repository in composer.json, composer does not accept to update to dev-fix-search-indexes:

Ah, sorry, it's been a while since I've done this. You can use an inline alias to require the package:
doctrine/mongodb-odm@dev-fix-search-indexes as 2.8.1-dev

That should fix the issue you encountered.

@nikophil
Copy link

nikophil commented Jul 31, 2024

hey thanks, I just learnt something! 😊

Now I have required your PR, I still have the same error:

➜ bin/console doctrine:mongodb:schema:update -n
Updated indexes for all classes
Updated validation for all classes
Unrecognized pipeline stage name: '$listSearchIndexes'

with exit code 255

BTW, we're using mongo 4.4

@alcaeus
Copy link
Member Author

alcaeus commented Jul 31, 2024

BTW, we're using mongo 4.4

Ah, that's probably the reason why you're getting a different message. Thanks for taking a look, I'll update this PR after some more testing.

@alcaeus
Copy link
Member Author

alcaeus commented Aug 1, 2024

@nikophil I've updated the code to also handle server versions where $listSearchIndexes doesn't exist (tested with 4.4.29 on my machine). The error message is consistent, so this should hopefully handle all kinds of server versions.

@@ -44,6 +46,7 @@ final class SchemaManager
private const GRIDFS_CHUNKS_COLLECTION_INDEX = ['filename' => 1, 'uploadDate' => 1];

private const CODE_SHARDING_ALREADY_INITIALIZED = 23;
private const CODE_COMMAND_NOT_SUPPORTED = 115;
Copy link
Member

Choose a reason for hiding this comment

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

I guess there are no consts anywhere in drivers? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope :)

@alcaeus alcaeus merged commit 10ec1da into doctrine:2.8.x Aug 26, 2024
15 of 17 checks passed
@alcaeus alcaeus added this to the 2.8.1 milestone Aug 26, 2024
@alcaeus alcaeus deleted the fix-search-indexes branch August 26, 2024 08:40
@mustanggb
Copy link

mustanggb commented Aug 27, 2024

With 2.8.1 this is still causing us a problem in TbbcMoneyBundle.

I think the circumstances are an index exists (not a search index), then the schema is dropped.

It tries to run a $listSearchIndexes pipeline, which errors (255).

We are dropping the schema with doctrine:mongodb:schema:drop (which I believe comes from mongodb-odm-bundle).

@alcaeus
Copy link
Member Author

alcaeus commented Aug 27, 2024

@mustanggb I just ran the tests for TbbcMoneyBundle locally. With MongoDB 7.0.9, I was able to reproduce the failures using ODM 2.8.0, but using 2.8.1 fixes the tests. Removing the constraint in #180 works on my machine - could you please give it another look and share the exception if one happens? I noticed that the command invocation uses a NullOutput, which makes debugging much more difficult than it should be.

@mustanggb
Copy link

Looks like it's saying $listSearchIndexes stage is only allowed on MongoDB Atlas.

https://github.com/mustanggb/TbbcMoneyBundle/actions/runs/10579960710/job/29313659438#step:8:1123

@Jafarili
Copy link
Contributor

For me also the error is $listSearchIndexes stage is only allowed on MongoDB Atlas

@alcaeus
Copy link
Member Author

alcaeus commented Sep 6, 2024

I've released 2.8.2 with a fix for this. Sorry for the delay here.

@mustanggb
Copy link

Confirming this is working for us now.

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.

5 participants