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

DRIVERS-2630: add e2e testing for search index commands and clarifications to search index spec #1442

Merged
merged 16 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ specifications:
- `Enumerating Collections <../enumerate-collections.rst>`__
- `Enumerating Databases <../enumerate-databases.rst>`__
- `GridFS <../gridfs/gridfs-spec.rst>`__
- `Index Management <../index-management.rst>`__
- `Index Management <../index-management/index-management.rst>`__
- `Transactions <../transactions/transactions.rst>`__
- `Convenient API for Transactions <../transactions-convenient-api/transactions-convenient-api.rst>`__

Expand Down
50 changes: 43 additions & 7 deletions source/index-management/index-management.rst
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,10 @@ Index View API
* For drivers that cannot make IndexView iterable, they MUST implement this method to
* return a list of indexes. In the case of async drivers, this MAY return a Future<Cursor>
* or language/implementation equivalent.
*
* If drivers are unable to make the IndexView iterable, they MAY opt to provide the options for
* listing search indexes via the `list` method instead of the `Collection.listSearchIndexes` method.
baileympearson marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion with @dariakp on slack, we decided that a more consistent API is to include the name in an options object. There were two reasons for this change:

  • It's odd to have a set of search index management methods that have a name as the first parameter and an optional second parameter in others. It makes more sense to include an optional name in an "options" object.
  • This is more closely aligned with the regular index management API.

Copy link
Member

Choose a reason for hiding this comment

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

I understand this was already allowed, this is how it is already implemented in PHP.

*/
list(): Cursor;

Expand Down Expand Up @@ -873,8 +877,8 @@ search index management helpers.
options as outline in the `CRUD specification <https://github.com/mongodb/specifications/blob/master/source/crud/crud.rst#read>`_. Drivers MAY combine the aggregation options with
any future ``listSearchIndexes`` stage options, if that is idiomatic for a driver's language.

Notes
-----
Asynchronicity
--------------

The search index commands are asynchronous and return from the server before the index is successfully updated, created or dropped.
In order to determine when an index has been created / updated, users are expected to run the ``listSearchIndexes`` repeatedly
Expand All @@ -888,7 +892,35 @@ An example, from Javascript:
while (!(await collection.listSearchIndexes({ name }).hasNext())) {
await setTimeout(1000);
}


Where are read concern and write concern?
-----------------------------------------

These commands internally proxy the search index management commands to a separate process that runs alongside an Atlas cluster. As such, read concern and
write concern are not relevant for the search index management commands.

Consistency with Existing APIs
------------------------------

Drivers SHOULD strive for a search index management API that is as consistent with their existing index management API as much as possible.
dariakp marked this conversation as resolved.
Show resolved Hide resolved


NamespaceNotFound Errors
------------------------

Some drivers suppress NamespaceNotFound errors for CRUD helpers. Drivers MAY suppress NamespaceNotFound errors from
the search index management helpers.

Drivers MUST suppress NamespaceNotFound errors for the ``dropSearchIndex`` helper. Drop operations should be idempotent:

.. code:: typescript

await collection.dropSearchIndex('my-test-index');
// subsequent calls should behave the same for the user as the first call
await collection.dropSearchIndex('my-test-index');
await collection.dropSearchIndex('my-test-index');


Common Interfaces
-----------------

Expand Down Expand Up @@ -926,7 +958,7 @@ Standard API for Search Indexes
* takes an SearchIndexModel as a parameter, or for those languages with method
* overloading MAY decide to implement both.
*/
createSearchIndex(name: String, definition: Document, options: Optional<CreateSearchIndexOptions>): String;
createSearchIndex(definition: Document, name: Optional<string>, options: Optional<CreateSearchIndexOptions>): String;

/**
* Convenience method for creating a single index.
Expand Down Expand Up @@ -974,7 +1006,7 @@ Index View API for Search Indexes
/**
* Returns the search index view for this collection.
*/
searchIndexes(name: Optional<String>, aggregateOptions: Optional<AggregationOptions>, options: Optional<ListSearchIndexesOptions>): SearchIndexView;
searchIndexes(name: Optional<String>, aggregateOptions: Optional<AggregationOptions>, options: Optional<ListSearchIndexOptions>): SearchIndexView;
}

interface SearchIndexView extends Iterable<Document> {
Expand All @@ -990,6 +1022,9 @@ Index View API for Search Indexes
* For drivers that cannot make SearchIndexView iterable, they MUST implement this method to
* return a list of indexes. In the case of async drivers, this MAY return a Future<Cursor>
* or language/implementation equivalent.
*
* If drivers are unable to make the SearchIndexView iterable, they MAY opt to provide the options for
* listing search indexes via the `list` method instead of the `Collection.listSearchIndexes` method.
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
*/
list(): Cursor<Document>;

Expand All @@ -1003,7 +1038,7 @@ Index View API for Search Indexes
* takes an SearchIndexModel as a parameter, or for those languages with method
* overloading MAY decide to implement both.
*/
createOne(name: String, definition: Document, options: Optional<CreateSearchIndexOptions>): String;
createOne(definition: Document, name: Optional<string>, options: Optional<CreateSearchIndexOptions>): String;

/**
* This is a convenience method for creating a single index.
Expand Down Expand Up @@ -1031,7 +1066,7 @@ Index View API for Search Indexes
/**
* Updates a single search index from the collection by the index name.
*/
updateOne(name: String, options: Optional<UpdateSearchIndexOptions>): Result;
updateOne(name: String, definition: Document, options: Optional<UpdateSearchIndexOptions>): Result;
dariakp marked this conversation as resolved.
Show resolved Hide resolved
}

---------
Expand Down Expand Up @@ -1093,3 +1128,4 @@ Changelog
:2023-05-10: Merge index enumeration and index management specs and get rid of references
to legacy server versions.
:2023-05-18: Add the search index management API.
:2023-07-17: Add search index management clarifications.
dariakp marked this conversation as resolved.
Show resolved Hide resolved
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
153 changes: 151 additions & 2 deletions source/index-management/tests/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ For each of the configurations:
indicated indexes act on

Tests
-----

- Run the driver's method that returns a list of index names, and:

Expand All @@ -45,4 +44,154 @@ Tests
- verify the "unique" flags show up for the unique index
- verify there are no duplicates in the returned list
- if the result consists of statically defined index models that include an ``ns`` field, verify
that its value is accurate
that its value is accurate

Search Index Management Helpers
-------------------------------

These tests are intended to smoke test the search management helpers end-to-end against a live Atlas cluster.

The search index management commands are asynchronous and mongod/mongos returns before the changes to a clusters' search indexes have completed. When
these prose tests specify "waiting for the changes", drivers should repeatedly poll the cluster with ``listSearchIndexes``
until the changes are visible. Each test specifies the condition that is considered "ready". For example, when creating a
new search index, waiting until the inserted index has a status ``queryable: true`` indicates that the index was successfully
created.

The commands tested in these prose tests take a while to successfully complete. Drivers should raise the timeout for each test to avoid timeout errors if
the test timeout is too low. 5 minutes is a sufficiently large timeout that any timeout that occurs indicates a real failure, but this value is not required and can be tweaked per-driver.

There is a server-side limitation that prevents multiple search indexes from being created with the same name, definition and
collection name. This limitation does not take into account collection uuid. Because these commands are asynchronous, any cleanup
code that may run after a test (cleaning a database or dropping search indexes) may not have completed by the next iteration of the
test (or the next test run, if running locally). To address this issue, each test uses a randomly generated collection name. Drivers
may generate this collection name however they like, but a suggested implementation is a hex representation of an
ObjectId (``new ObjectId().toHexString()`` in Node).

Setup
~~~~~

baileympearson marked this conversation as resolved.
Show resolved Hide resolved
These tests must run against an Atlas cluster with a 7.0+ server. Tools are available drivers-evergreen-tools which can setup and teardown
Atlas clusters. To ensure that the Atlas cluster is cleaned up after each CI run, drivers should configure evergreen to run these tests
as a part of a task group. Be sure that the cluster gets torn down!

When working locally on these tests, the same Atlas setup and teardown scripts can be used locally to provision a cluster for development.

Case 1: Driver can successfully create and list search indexes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

kevinAlbs marked this conversation as resolved.
Show resolved Hide resolved
#. Create a collection with a randomly generated name (referred to as ``coll0``).
#. Create a new search index on ``coll0`` with the ``createSearchIndex`` helper. Use the following definition:

.. code:: typescript

{
GromNaN marked this conversation as resolved.
Show resolved Hide resolved
name: 'test-search-index',
definition: {
mappings: { dynamic: false }
}
}

#. Assert that the command returns the name of the index: ``"test-search-index"``.
#. Run ``coll0.listSearchIndexes()`` repeatedly every 5 seconds until the following condition is satisfied:
1. An index with the ``name`` of ``test-search-index`` is present and the index has a field ``queryable`` with a value of ``true``.

joshbsiegel marked this conversation as resolved.
Show resolved Hide resolved
kevinAlbs marked this conversation as resolved.
Show resolved Hide resolved
#. Assert that ``index`` has a property ``mappings`` whose value is ``{ dynamic: false }``

Case 2: Driver can successfully create multiple indexes in batch
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

#. Create a collection with a randomly generated name (referred to as ``coll0``).
#. Create two new search indexes on ``coll0`` with the ``createSearchIndexes`` helper. Use the following
definitions when creating the indexes. These definitions are referred to as ``indexDefinitions``.

.. code:: typescript

{
name: 'test-search-index-1',
definition: {
mappings: { dynamic: false }
}
}
{
name: 'test-search-index-2',
definition: {
mappings: { dynamic: false }
}
}

#. Assert that the command returns an array containing the new indexes' names: ``["test-search-index-1", "test-search-index-2"]``.
GromNaN marked this conversation as resolved.
Show resolved Hide resolved
#. Run ``coll0.listSearchIndexes()`` repeatedly every 5 seconds until the following condition is satisfied. Store
the result in ``indexes``.
1. An index with the ``name`` of ``test-search-index-1`` is present and index has a field ``queryable`` with the value of ``true``.
2. An index with the ``name`` of ``test-search-index-2`` is present and index has a field ``queryable`` with the value of ``true``.
#. For each ``index`` in ``indexDefinitions``
1. Find the matching index definition in ``indexes`` by matching on ``index.name``. If no index exists, raise an error.
kevinAlbs marked this conversation as resolved.
Show resolved Hide resolved
2. Assert that the matching index ``mappings``, whose value is ``{ dynamic: false }``

Case 3: Driver can successfully drop search indexes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

#. Create a collection with a randomly generated name (referred to as ``coll0``).
#. Create a new search index on ``coll0`` with the following definition:

.. code:: typescript

{
name: 'test-search-index',
definition: {
mappings: { dynamic: false }
}
}

#. Assert that the command returns the name of the index: ``"test-search-index"``.
#. Run ``coll0.listSearchIndexes()`` repeatedly every 5 seconds until the following condition is satisfied:
1. An index with the ``name`` of ``test-search-index`` is present and index has a field ``queryable`` with the value of ``true``.

kevinAlbs marked this conversation as resolved.
Show resolved Hide resolved
#. Run a ``dropSearchIndexes`` on ``coll0``, using ``test-search-index`` for the name.
#. Run ``coll0.listSearchIndexes()`` repeatedly every 5 seconds until ``listSearchIndexes`` returns an empty array.

This test fails if it times out waiting for the deletion to succeed.

Case 4: Driver can update a search index
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

#. Create a collection with a randomly generated name (referred to as ``coll0``).
#. Create a new search index on ``coll0`` with the following definition:

.. code:: typescript

{
name: 'test-search-index',
definition: {
mappings: { dynamic: false }
}
}

#. Assert that the command returns the name of the index: ``"test-search-index"``.
#. Run ``coll0.listSearchIndexes()`` repeatedly every 5 seconds until the following condition is satisfied:
1. An index with the ``name`` of ``test-search-index`` is present and index has a field ``queryable`` with the value of ``true``.

#. Run a ``updateSearchIndex`` on ``coll0``, using the following definition.

.. code:: typescript

{
name: 'test-search-index',
definition: {
mappings: { dynamic: true }
}
}

#. Assert that the command does not error and the server responds with a success.
#. Run ``coll0.listSearchIndexes()`` repeatedly every 5 seconds until the following condition is satisfied:
1. An index with the ``name`` of ``test-search-index`` is present. This index is referred to as ``index``.
2. The index has a field ``queryable`` with a value of ``true`` and has a field ``status`` with the value of ``READY``.

#. Assert that an index is present with the name ``test-search-index`` and the definition has a
kevinAlbs marked this conversation as resolved.
Show resolved Hide resolved
property ``mappings``, whose value is ``{ dynamic: true }``

GromNaN marked this conversation as resolved.
Show resolved Hide resolved
Case 5: ``dropSearchIndex`` suppresses namespace not found errors
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

#. Create a driver-side collection object for a randomly generated collection name. Do not create this collection on the server.
#. Run a ``dropSearchIndex`` command and assert that no error is thrown.
6 changes: 4 additions & 2 deletions source/index-management/tests/createSearchIndex.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@
}
},
"expectError": {
"isError": true
"isError": true,
"errorContains": "Search index commands are only supported with Atlas"
}
}
],
Expand Down Expand Up @@ -100,7 +101,8 @@
}
},
"expectError": {
"isError": true
"isError": true,
"errorContains": "Search index commands are only supported with Atlas"
}
}
],
Expand Down
10 changes: 6 additions & 4 deletions source/index-management/tests/createSearchIndex.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ tests:
arguments:
model: { definition: &definition { mappings: { dynamic: true } } }
expectError:
# Search indexes are only available on 7.0+ atlas clusters. DRIVERS-2630 will add e2e testing
# against an Atlas cluster and the expectError will be removed.
# This test always errors in a non-Atlas environment. The test functions as a unit test by asserting
# that the driver constructs and sends the correct command.
isError: true
errorContains: Search index commands are only supported with Atlas
expectEvents:
- client: *client0
events:
Expand All @@ -47,9 +48,10 @@ tests:
arguments:
model: { definition: &definition { mappings: { dynamic: true } } , name: 'test index' }
expectError:
# Search indexes are only available on 7.0+ atlas clusters. DRIVERS-2630 will add e2e testing
# against an Atlas cluster and the expectError will be removed.
# This test always errors in a non-Atlas environment. The test functions as a unit test by asserting
# that the driver constructs and sends the correct command.
isError: true
errorContains: Search index commands are only supported with Atlas
expectEvents:
- client: *client0
events:
Expand Down
9 changes: 6 additions & 3 deletions source/index-management/tests/createSearchIndexes.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@
"models": []
},
"expectError": {
"isError": true
"isError": true,
"errorContains": "Search index commands are only supported with Atlas"
}
}
],
Expand Down Expand Up @@ -87,7 +88,8 @@
]
},
"expectError": {
"isError": true
"isError": true,
"errorContains": "Search index commands are only supported with Atlas"
}
}
],
Expand Down Expand Up @@ -135,7 +137,8 @@
]
},
"expectError": {
"isError": true
"isError": true,
"errorContains": "Search index commands are only supported with Atlas"
}
}
],
Expand Down
Loading
Loading