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

Remove write and read concern from Atlas Search Index commands. #1241

Merged
merged 9 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import com.mongodb.MongoCommandException;
import com.mongodb.MongoNamespace;
import com.mongodb.WriteConcern;
import com.mongodb.internal.async.SingleResultCallback;
import com.mongodb.internal.binding.AsyncWriteBinding;
import com.mongodb.internal.binding.WriteBinding;
Expand All @@ -40,12 +39,9 @@
*/
abstract class AbstractWriteSearchIndexOperation implements AsyncWriteOperation<Void>, WriteOperation<Void> {
private final MongoNamespace namespace;
private final WriteConcern writeConcern;

AbstractWriteSearchIndexOperation(final MongoNamespace mongoNamespace,
final WriteConcern writeConcern) {
AbstractWriteSearchIndexOperation(final MongoNamespace mongoNamespace) {
this.namespace = mongoNamespace;
this.writeConcern = writeConcern;
}

@Override
Expand Down Expand Up @@ -101,8 +97,4 @@ <E extends Throwable> void swallowOrThrow(@Nullable final E mongoExecutionExcept
MongoNamespace getNamespace() {
return namespace;
}

WriteConcern getWriteConcern() {
return writeConcern;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.mongodb.internal.operation;

import com.mongodb.MongoNamespace;
import com.mongodb.WriteConcern;
import org.bson.BsonArray;
import org.bson.BsonDocument;
import org.bson.BsonString;
Expand All @@ -26,7 +25,6 @@
import java.util.stream.Collectors;

import static com.mongodb.assertions.Assertions.assertNotNull;
import static com.mongodb.internal.operation.WriteConcernHelper.appendWriteConcernToCommand;

/**
* An operation that creates one or more Atlas Search indexes.
Expand All @@ -37,9 +35,8 @@ final class CreateSearchIndexesOperation extends AbstractWriteSearchIndexOperati
private static final String COMMAND_NAME = "createSearchIndexes";
private final List<SearchIndexRequest> indexRequests;

CreateSearchIndexesOperation(final MongoNamespace namespace, final List<SearchIndexRequest> indexRequests,
final WriteConcern writeConcern) {
super(namespace, writeConcern);
CreateSearchIndexesOperation(final MongoNamespace namespace, final List<SearchIndexRequest> indexRequests) {
super(namespace);
this.indexRequests = assertNotNull(indexRequests);
}

Expand All @@ -61,9 +58,7 @@ private static BsonDocument convert(final SearchIndexRequest request) {

@Override
BsonDocument buildCommand() {
BsonDocument command = new BsonDocument(COMMAND_NAME, new BsonString(getNamespace().getCollectionName()))
return new BsonDocument(COMMAND_NAME, new BsonString(getNamespace().getCollectionName()))
.append("indexes", convert(indexRequests));
appendWriteConcernToCommand(getWriteConcern(), command);
return command;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,11 @@
package com.mongodb.internal.operation;

import com.mongodb.MongoNamespace;
import com.mongodb.WriteConcern;
import com.mongodb.lang.Nullable;
import org.bson.BsonDocument;
import org.bson.BsonString;

import static com.mongodb.internal.operation.CommandOperationHelper.isNamespaceError;
import static com.mongodb.internal.operation.WriteConcernHelper.appendWriteConcernToCommand;

/**
* An operation that drops an Alas Search index.
Expand All @@ -34,9 +32,8 @@ final class DropSearchIndexOperation extends AbstractWriteSearchIndexOperation {
private static final String COMMAND_NAME = "dropSearchIndex";
private final String indexName;

DropSearchIndexOperation(final MongoNamespace namespace, final String indexName,
final WriteConcern writeConcern) {
super(namespace, writeConcern);
DropSearchIndexOperation(final MongoNamespace namespace, final String indexName) {
super(namespace);
this.indexName = indexName;
}

Expand All @@ -49,9 +46,7 @@ <E extends Throwable> void swallowOrThrow(@Nullable final E mongoExecutionExcept

@Override
BsonDocument buildCommand() {
BsonDocument command = new BsonDocument(COMMAND_NAME, new BsonString(getNamespace().getCollectionName()))
return new BsonDocument(COMMAND_NAME, new BsonString(getNamespace().getCollectionName()))
.append("name", new BsonString(indexName));
appendWriteConcernToCommand(getWriteConcern(), command);
return command;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -651,20 +651,19 @@ CreateSearchIndexesOperation createSearchIndexes(final List<SearchIndexModel> in
.map(this::createSearchIndexRequest)
.collect(Collectors.toList());

return new CreateSearchIndexesOperation(assertNotNull(namespace), indexRequests, writeConcern);
return new CreateSearchIndexesOperation(assertNotNull(namespace), indexRequests);
}

UpdateSearchIndexesOperation updateSearchIndex(final String indexName, final Bson definition) {
BsonDocument definitionDocument = assertNotNull(toBsonDocument(definition));
SearchIndexRequest searchIndexRequest = new SearchIndexRequest(definitionDocument, indexName);

return new UpdateSearchIndexesOperation(assertNotNull(namespace), searchIndexRequest,
writeConcern);
return new UpdateSearchIndexesOperation(assertNotNull(namespace), searchIndexRequest);
}


DropSearchIndexOperation dropSearchIndex(final String indexName) {
return new DropSearchIndexOperation(assertNotNull(namespace), indexName, writeConcern);
return new DropSearchIndexOperation(assertNotNull(namespace), indexName);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,9 @@
package com.mongodb.internal.operation;

import com.mongodb.MongoNamespace;
import com.mongodb.WriteConcern;
import org.bson.BsonDocument;
import org.bson.BsonString;

import static com.mongodb.internal.operation.WriteConcernHelper.appendWriteConcernToCommand;

/**
* An operation that updates an Atlas Search index.
*
Expand All @@ -32,19 +29,16 @@ final class UpdateSearchIndexesOperation extends AbstractWriteSearchIndexOperati
private static final String COMMAND_NAME = "updateSearchIndex";
private final SearchIndexRequest request;

UpdateSearchIndexesOperation(final MongoNamespace namespace, final SearchIndexRequest request,
final WriteConcern writeConcern) {
super(namespace, writeConcern);
UpdateSearchIndexesOperation(final MongoNamespace namespace, final SearchIndexRequest request) {
super(namespace);
this.request = request;
}

@Override
BsonDocument buildCommand() {
BsonDocument command = new BsonDocument(COMMAND_NAME, new BsonString(getNamespace().getCollectionName()))
return new BsonDocument(COMMAND_NAME, new BsonString(getNamespace().getCollectionName()))
.append("name", new BsonString(request.getIndexName()))
.append("definition", request.getDefinition());
appendWriteConcernToCommand(getWriteConcern(), command);
return command;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,10 @@ public ListSearchIndexesPublisher<Document> listSearchIndexes() {
@Override
public <TResult> ListSearchIndexesPublisher<TResult> listSearchIndexes(final Class<TResult> resultClass) {
notNull("resultClass", resultClass);
return new ListSearchIndexesPublisherImpl<>(mongoOperationPublisher.withDocumentClass(resultClass));

return new ListSearchIndexesPublisherImpl<>(mongoOperationPublisher
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear on why, in driver-sync's MongoCollectionImpl, read concern seems to have been removed, while here it seems like we're adding it. Can you clarify the intent?

.withReadConcern(ReadConcern.DEFAULT)
.withDocumentClass(resultClass));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,9 @@ final class ListSearchIndexesIterableImpl<TResult> extends MongoIterableImpl<TRe
private final CodecRegistry codecRegistry;

ListSearchIndexesIterableImpl(final MongoNamespace namespace, final OperationExecutor executor,
final ReadConcern readConcern, final Class<TResult> resultClass,
final CodecRegistry codecRegistry, final ReadPreference readPreference,
final boolean retryReads) {
super(null, executor, readConcern, readPreference, retryReads);
final Class<TResult> resultClass, final CodecRegistry codecRegistry,
final ReadPreference readPreference, final boolean retryReads) {
super(null, executor, ReadConcern.DEFAULT, readPreference, retryReads);

this.resultClass = resultClass;
this.operations = new SyncOperations<>(namespace, BsonDocument.class, readPreference, codecRegistry, retryReads);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,7 @@ private <TResult> ListIndexesIterable<TResult> createListIndexesIterable(@Nullab
}

private <TResult> ListSearchIndexesIterable<TResult> createListSearchIndexesIterable(final Class<TResult> resultClass) {
return new ListSearchIndexesIterableImpl<>(getNamespace(), executor, readConcern,
return new ListSearchIndexesIterableImpl<>(getNamespace(), executor,
resultClass, codecRegistry, readPreference, retryReads);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@
package com.mongodb.client;

import com.mongodb.MongoClientSettings;
import com.mongodb.ReadConcern;
import com.mongodb.WriteConcern;
import com.mongodb.client.model.SearchIndexModel;
import com.mongodb.event.CommandListener;
import com.mongodb.event.CommandStartedEvent;
import org.bson.BsonDocument;
import org.bson.Document;
import org.bson.conversions.Bson;
import org.junit.jupiter.api.AfterEach;
Expand All @@ -38,7 +43,9 @@
import java.util.stream.StreamSupport;

import static com.mongodb.ClusterFixture.serverVersionAtLeast;
import static com.mongodb.assertions.Assertions.assertFalse;
import static com.mongodb.client.Fixture.getMongoClientSettings;
import static com.mongodb.client.Fixture.getMongoClientSettingsBuilder;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;

Expand Down Expand Up @@ -74,8 +81,8 @@ public abstract class AbstractAtlasSearchIndexManagementProseTest {
protected abstract MongoClient createMongoClient(MongoClientSettings settings);

protected AbstractAtlasSearchIndexManagementProseTest() {
Assumptions.assumeTrue(serverVersionAtLeast(6, 0));
Assumptions.assumeTrue(hasAtlasSearchIndexHelperEnabled(), "Atlas Search Index tests are disabled"); //TODO enable by flag
Assumptions.assumeTrue(serverVersionAtLeast(6, 0));
Assumptions.assumeTrue(hasAtlasSearchIndexHelperEnabled(), "Atlas Search Index tests are disabled");
}

private static boolean hasAtlasSearchIndexHelperEnabled() {
Expand All @@ -84,7 +91,29 @@ private static boolean hasAtlasSearchIndexHelperEnabled() {

@BeforeEach
public void setUp() {
client = createMongoClient(getMongoClientSettings());
MongoClientSettings mongoClientSettings = getMongoClientSettingsBuilder()
.writeConcern(WriteConcern.MAJORITY)
.readConcern(ReadConcern.MAJORITY)
.addCommandListener(new CommandListener() {
@Override
public void commandStarted(final CommandStartedEvent event) {
/* This test case examines scenarios where the write or read concern is not forwarded to the server
for any Atlas Index Search commands. If a write or read concern is included in the command,
the server will return an error. */
if (isSearchIndexCommand(event)) {
BsonDocument command = event.getCommand();
assertFalse(command.containsKey("writeConcern"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I realized that I had noticed this earlier and then neglected to comment on it: asserting like this in any event listener is likely a no-op since the driver (I think) swallows exceptions thrown by listeners and in any case it may throw on a different thread for reactive tests.

You need to instead set some state in the listener and then assert on that state in the main test code. There are examples of this in many places in the driver.

Since this PR is already merged, you'll have to address it in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

(This is a good example of where writing a failing test first, TDD style, can catch errors in the test itself)

assertFalse(command.containsKey("readConcern"));
}
}

private boolean isSearchIndexCommand(final CommandStartedEvent event) {
return event.getCommand().toJson().contains("SearchIndex");
}
})
.build();

client = createMongoClient(mongoClientSettings);
db = client.getDatabase("test");

String collectionName = UUID.randomUUID().toString();
Expand Down