PHPLIB-1143: Add search index operations to Collection class #1097
Conversation
GromNaN
force-pushed
the
PHPLIB-1143
branch
4 times, most recently
from
233c594
to
964e7fc
Compare
There was a problem hiding this comment.
I think I've figured out how spec works: all the Atlas Search index command get a server error because they are not available on local servers. The spec checks the command that is sent to the server with expectedEvents. Therefore, I don't validate some inputs client-side to let the server return an error. This is more future-proof as some options that are invalid in 7.0 could be allowed in a future release.
GromNaN
force-pushed
the
PHPLIB-1143
branch
2 times, most recently
from
87a034d
to
356176e
Compare
src/Collection.php
Outdated
| * @throws DriverRuntimeException for other driver errors (e.g. connection errors) | ||
| * @see ListSearchIndexes::__construct() for supported aggregation and list options | ||
| */ | ||
| public function listSearchIndexes(?string $name = null, array $options = []): Iterator |
There was a problem hiding this comment.
How do you feel about folding name into $options to simplify this API?
Surveying existing API methods, there are only a few cases where we explicitly list optional parameters in the signature:
- Query filter for read operations (e.g.
countDocuments(),find()) - Pipeline for change steams (
watch()) - Database name for
rename()
Filters and the pipeline are commonly provided to their operations, and the database name was an interesting case that we discussed at length in #840.
The name option seems more comparable to the filter option on Database::listCollections.
Long term, I think we'd probably want to have a conversation about migrating from options arrays to named parameters. That's something to consider in 2.0 when we'll surely require PHP 8+. For now, I'd want to be conservative about what optional parameters showing up outside of our existing options arrays.
/cc @alcaeus in case you have an opinion here
There was a problem hiding this comment.
Sure, I've harmonized Collection::listSearchIndexes($options) with Database::listCollections($options) and Collection::listIndexes($options).
Do you want to report this signature change to the spec owner?
There was a problem hiding this comment.
Do you want to report this signature change to the spec owner?
I think it makes sense to follow up with the spec authors about this. The current signature in the spec is:
listSearchIndexes(name: Optional<String>, aggregationOptions: Optional<AggregationOptions>, listIndexOptions: Optional<ListSearchIndexOptions>): Cursor<Document>;
ListSearchIndexOptions isn't defined in the spec, but I see no reason for name to be broken out as a positional argument. If the database enumeration spec is relevant here (see: Filters), I should mention that there's no signature defined in that spec. It just loosely talks about various options for the listDatabases command.
Having said that, moving $name to $options in PHPLIB should not require any changes in the spec. The specs are seldom prescriptive about how to solicit options, so the only important bit here is that name is still an option.
There was a problem hiding this comment.
Discussed with the spec author: mongodb/specifications#1442 (comment)
GromNaN
force-pushed
the
PHPLIB-1143
branch
3 times, most recently
from
6500e8f
to
4ccc852
Compare
|
GromNaN
force-pushed
the
PHPLIB-1143
branch
2 times, most recently
from
8e5e678
to
a14eaf1
Compare
GromNaN
force-pushed
the
PHPLIB-1143
branch
3 times, most recently
from
0d3d54a
to
9b6d5a1
Compare
|
mongodb/specifications#1442 have been merged and the spec tests files are up to date. Last thing is to run tests in CI against Atlas PHPLIB-1168. |
GromNaN
force-pushed
the
PHPLIB-1143
branch
4 times, most recently
from
b779ebf
to
5b64391
Compare
|
|
||
| public function provideInvalidIndexSpecificationTypes(): array | ||
| { | ||
| return $this->wrapValuesForDataProvider($this->getInvalidDocumentValues()); |
There was a problem hiding this comment.
| return $this->wrapValuesForDataProvider($this->getInvalidDocumentValues()); | |
| return $this->wrapValuesForDataProvider($this->getInvalidArrayValues()); |
I think this can be updated now that you're using is_array() to validate the list elements. The above code is consistent with provideInvalidIndexSpecificationTypes() in CreateIndexesTest.php.
|
|
||
| $this->assertCount(1, $indexes); | ||
| $this->assertSame($name, $indexes[0]->name); | ||
| $this->assertDocumentsMatch($mapping, $indexes[0]->latestDefinition); |
There was a problem hiding this comment.
Apologies for not specifying, but I was suggesting assertMatchesDocument() or assertSameDocument() from the base TestCase class instead of comparing JSON strings here.
assertDocumentsMatch() is unique to the legacy spec test runner (note that its DocumentsMatchConstraint class differs from the unified test runner's Matches constraint), and I'd be hesitant to move the legacy test runner logic into the main test suite, as the ultimate goal is to delete all of that code once each spec can be ported over to the unified format.
Is it feasible to use one of the two existing TestCase methods here? assertMatchesDocument() allows extra fields in the actual document, while assertSameDocument() does not -- so you can decide which is most appropriate. If so, you can then revert the introduction of DocumentsMatchConstraint to FunctionalTestCase.
tests/FunctionalTestCase.php
Outdated
| * @param array|object $expectedDocument | ||
| * @param array|object $actualDocument | ||
| */ | ||
| public static function assertDocumentsMatch($expectedDocument, $actualDocument, string $message = ''): void |
There was a problem hiding this comment.
I explained in another thread why I'd rather not introduce legacy spec test classes into the main test suite, but even if this was to stay this would be a method to add on the base TestCase class since it doesn't depend on a server connection.
The base TestCase is also where the existing BSON assertions are.
Fix PHPLIB-1143
Spec files sync from mongodb/specifications@267a54d
Previously introduced by mongodb/specifications@ae35908
createSearchIndex,createSearchIndexes,dropSearchIndex,updateSearchIndex,listSearchIndexesMONGODB_URIvariable on Atlas. Currently only M10+ are supported.