Skip to content

Commit e9447ac

Browse files
authored
Remove assertion for verbose result (#1850)
- Remove an assertion that was failing due to server bug SERVER-113344 where successful operation results are unexpectedly returned in the cursor when verbose results are disabled. - Add unit tests for JAVA-6001 and JAVA-5986. JAVA-6001 JAVA-5986 --------- Co-authored-by: Ross Lawley <ross@mongodb.com> (cherry picked from commit 96d0c6b)
1 parent 5c07c71 commit e9447ac

File tree

3 files changed

+274
-26
lines changed

3 files changed

+274
-26
lines changed

driver-core/src/main/com/mongodb/internal/connection/DualMessageSequences.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.mongodb.internal.connection;
1818

19+
import com.mongodb.internal.VisibleForTesting;
1920
import org.bson.BsonBinaryWriter;
2021
import org.bson.BsonElement;
2122
import org.bson.FieldNameValidator;
@@ -61,7 +62,8 @@ String getSecondSequenceId() {
6162
return secondSequenceId;
6263
}
6364

64-
protected abstract EncodeDocumentsResult encodeDocuments(WritersProviderAndLimitsChecker writersProviderAndLimitsChecker);
65+
@VisibleForTesting(otherwise = VisibleForTesting.AccessModifier.PROTECTED)
66+
public abstract EncodeDocumentsResult encodeDocuments(WritersProviderAndLimitsChecker writersProviderAndLimitsChecker);
6567

6668
/**
6769
* @see #tryWrite(WriteAction)

driver-core/src/main/com/mongodb/internal/operation/ClientBulkWriteOperation.java

Lines changed: 53 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -758,33 +758,25 @@ ClientBulkWriteResult build(@Nullable final MongoException topLevelError, final
758758
deletedCount += response.getNDeleted();
759759
Map<Integer, BsonValue> insertModelDocumentIds = batchResult.getInsertModelDocumentIds();
760760
for (BsonDocument individualOperationResponse : response.getCursorExhaust()) {
761+
boolean individualOperationSuccessful = individualOperationResponse.getNumber("ok").intValue() == 1;
762+
if (individualOperationSuccessful && !verboseResultsSetting) {
763+
//TODO-JAVA-6002 Previously, assertTrue(verboseResultsSetting) was used when ok == 1 because the server
764+
// was not supposed to return successful operation results in the cursor when verboseResultsSetting is false.
765+
// Due to server bug SERVER-113344, these unexpected results must be ignored until we stop supporting server
766+
// versions affected by this bug. When that happens, restore assertTrue(verboseResultsSetting).
767+
continue;
768+
}
761769
int individualOperationIndexInBatch = individualOperationResponse.getInt32("idx").getValue();
762770
int writeModelIndex = batchStartModelIndex + individualOperationIndexInBatch;
763-
if (individualOperationResponse.getNumber("ok").intValue() == 1) {
764-
assertTrue(verboseResultsSetting);
765-
AbstractClientNamespacedWriteModel writeModel = getNamespacedModel(models, writeModelIndex);
766-
if (writeModel instanceof ConcreteClientNamespacedInsertOneModel) {
767-
insertResults.put(
768-
writeModelIndex,
769-
new ConcreteClientInsertOneResult(insertModelDocumentIds.get(individualOperationIndexInBatch)));
770-
} else if (writeModel instanceof ConcreteClientNamespacedUpdateOneModel
771-
|| writeModel instanceof ConcreteClientNamespacedUpdateManyModel
772-
|| writeModel instanceof ConcreteClientNamespacedReplaceOneModel) {
773-
BsonDocument upsertedIdDocument = individualOperationResponse.getDocument("upserted", null);
774-
updateResults.put(
775-
writeModelIndex,
776-
new ConcreteClientUpdateResult(
777-
individualOperationResponse.getInt32("n").getValue(),
778-
individualOperationResponse.getInt32("nModified", new BsonInt32(0)).getValue(),
779-
upsertedIdDocument == null ? null : upsertedIdDocument.get("_id")));
780-
} else if (writeModel instanceof ConcreteClientNamespacedDeleteOneModel
781-
|| writeModel instanceof ConcreteClientNamespacedDeleteManyModel) {
782-
deleteResults.put(
783-
writeModelIndex,
784-
new ConcreteClientDeleteResult(individualOperationResponse.getInt32("n").getValue()));
785-
} else {
786-
fail(writeModel.getClass().toString());
787-
}
771+
if (individualOperationSuccessful) {
772+
collectSuccessfulIndividualOperationResult(
773+
individualOperationResponse,
774+
writeModelIndex,
775+
individualOperationIndexInBatch,
776+
insertModelDocumentIds,
777+
insertResults,
778+
updateResults,
779+
deleteResults);
788780
} else {
789781
batchResultsHaveInfoAboutSuccessfulIndividualOperations = batchResultsHaveInfoAboutSuccessfulIndividualOperations
790782
|| (orderedSetting && individualOperationIndexInBatch > 0);
@@ -824,6 +816,42 @@ ClientBulkWriteResult build(@Nullable final MongoException topLevelError, final
824816
}
825817
}
826818

819+
private void collectSuccessfulIndividualOperationResult(final BsonDocument individualOperationResponse,
820+
final int writeModelIndex,
821+
final int individualOperationIndexInBatch,
822+
final Map<Integer, BsonValue> insertModelDocumentIds,
823+
final Map<Integer, ClientInsertOneResult> insertResults,
824+
final Map<Integer, ClientUpdateResult> updateResults,
825+
final Map<Integer, ClientDeleteResult> deleteResults) {
826+
AbstractClientNamespacedWriteModel writeModel = getNamespacedModel(models, writeModelIndex);
827+
if (writeModel instanceof ConcreteClientNamespacedInsertOneModel) {
828+
insertResults.put(
829+
writeModelIndex,
830+
new ConcreteClientInsertOneResult(insertModelDocumentIds.get(individualOperationIndexInBatch)));
831+
} else if (writeModel instanceof ConcreteClientNamespacedUpdateOneModel
832+
|| writeModel instanceof ConcreteClientNamespacedUpdateManyModel
833+
|| writeModel instanceof ConcreteClientNamespacedReplaceOneModel) {
834+
BsonDocument upsertedIdDocument = individualOperationResponse.getDocument("upserted", null);
835+
updateResults.put(
836+
writeModelIndex,
837+
new ConcreteClientUpdateResult(
838+
individualOperationResponse.getInt32("n").getValue(),
839+
//TODO-JAVA-6005 Previously, we did not provide a default value of 0 because the
840+
// server was supposed to return nModified as 0 when no documents were changed.
841+
// Due to server bug SERVER-113026, we must provide a default of 0 until we stop supporting
842+
// server versions affected by this bug. When that happens, remove the default value for nModified.
843+
individualOperationResponse.getInt32("nModified", new BsonInt32(0)).getValue(),
844+
upsertedIdDocument == null ? null : upsertedIdDocument.get("_id")));
845+
} else if (writeModel instanceof ConcreteClientNamespacedDeleteOneModel
846+
|| writeModel instanceof ConcreteClientNamespacedDeleteManyModel) {
847+
deleteResults.put(
848+
writeModelIndex,
849+
new ConcreteClientDeleteResult(individualOperationResponse.getInt32("n").getValue()));
850+
} else {
851+
fail(writeModel.getClass().toString());
852+
}
853+
}
854+
827855
void onNewServerAddress(final ServerAddress serverAddress) {
828856
this.serverAddress = serverAddress;
829857
}
Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
/*
2+
* Copyright 2008-present MongoDB, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.mongodb.internal.operation;
18+
19+
import com.mongodb.MongoNamespace;
20+
import com.mongodb.ServerAddress;
21+
import com.mongodb.WriteConcern;
22+
import com.mongodb.client.model.Filters;
23+
import com.mongodb.client.model.bulk.ClientBulkWriteOptions;
24+
import com.mongodb.client.model.bulk.ClientBulkWriteResult;
25+
import com.mongodb.client.model.bulk.ClientNamespacedReplaceOneModel;
26+
import com.mongodb.client.model.bulk.ClientNamespacedWriteModel;
27+
import com.mongodb.connection.ClusterId;
28+
import com.mongodb.connection.ConnectionDescription;
29+
import com.mongodb.connection.ServerConnectionState;
30+
import com.mongodb.connection.ServerDescription;
31+
import com.mongodb.connection.ServerId;
32+
import com.mongodb.connection.ServerType;
33+
import com.mongodb.internal.binding.ConnectionSource;
34+
import com.mongodb.internal.binding.ReadWriteBinding;
35+
import com.mongodb.internal.client.model.bulk.AcknowledgedSummaryClientBulkWriteResult;
36+
import com.mongodb.internal.connection.Connection;
37+
import com.mongodb.internal.connection.DualMessageSequences;
38+
import com.mongodb.internal.connection.OperationContext;
39+
import com.mongodb.internal.mockito.MongoMockito;
40+
import org.bson.BsonBinaryWriter;
41+
import org.bson.BsonDocument;
42+
import org.bson.Document;
43+
import org.bson.codecs.Codec;
44+
import org.bson.codecs.DecoderContext;
45+
import org.bson.io.BasicOutputBuffer;
46+
import org.bson.json.JsonReader;
47+
import org.junit.jupiter.api.BeforeEach;
48+
import org.junit.jupiter.api.Test;
49+
50+
import java.util.List;
51+
52+
import static com.mongodb.ClusterFixture.OPERATION_CONTEXT;
53+
import static com.mongodb.MongoClientSettings.getDefaultCodecRegistry;
54+
import static com.mongodb.client.model.bulk.ClientReplaceOneOptions.clientReplaceOneOptions;
55+
import static java.util.Collections.singletonList;
56+
import static org.junit.jupiter.api.Assertions.assertEquals;
57+
import static org.junit.jupiter.api.Assertions.assertTrue;
58+
import static org.mockito.ArgumentMatchers.any;
59+
import static org.mockito.ArgumentMatchers.anyBoolean;
60+
import static org.mockito.ArgumentMatchers.anyString;
61+
import static org.mockito.ArgumentMatchers.isNull;
62+
import static org.mockito.Mockito.doAnswer;
63+
import static org.mockito.Mockito.doReturn;
64+
65+
class ClientBulkWriteOperationTest {
66+
private static final MongoNamespace NAMESPACE = new MongoNamespace("testDb.testCol");
67+
private Connection connection;
68+
private ConnectionSource connectionSource;
69+
private ReadWriteBinding binding;
70+
71+
@BeforeEach
72+
void setUp() {
73+
connection = MongoMockito.mock(Connection.class);
74+
connectionSource = MongoMockito.mock(ConnectionSource.class);
75+
binding = MongoMockito.mock(ReadWriteBinding.class);
76+
77+
doReturn(new ConnectionDescription(new ServerId(new ClusterId("test"), new ServerAddress()))).when(connection).getDescription();
78+
doReturn(connection).when(connectionSource).getConnection();
79+
doReturn(0).when(connectionSource).release();
80+
doReturn(0).when(connection).release();
81+
82+
doReturn(ServerDescription.builder().address(new ServerAddress())
83+
.state(ServerConnectionState.CONNECTED)
84+
.type(ServerType.STANDALONE)
85+
.build()).when(connectionSource).getServerDescription();
86+
doReturn(OPERATION_CONTEXT).when(connectionSource).getOperationContext();
87+
88+
doReturn(connectionSource).when(binding).getWriteConnectionSource();
89+
doReturn(OPERATION_CONTEXT).when(binding).getOperationContext();
90+
}
91+
92+
93+
/**
94+
* This test exists due to SERVER-113344 bug.
95+
*/
96+
//TODO-JAVA-6002
97+
@Test
98+
void shouldIgnoreSuccessfulCursorResultWhenVerboseResultIsFalse() {
99+
//given
100+
mockCommandExecutionResult(
101+
"{'cursor': {"
102+
+ " 'id': NumberLong(0),"
103+
+ " 'firstBatch': [ { 'ok': 1, 'idx': 0, 'n': 1, 'upserted': { '_id': 1 } } ],"
104+
+ " 'ns': 'admin.$cmd.bulkWrite'"
105+
+ "},"
106+
+ " 'nErrors': 0,"
107+
+ " 'nInserted': 0,"
108+
+ " 'nMatched': 0,"
109+
+ " 'nModified': 0,"
110+
+ " 'nUpserted': 1,"
111+
+ " 'nDeleted': 0,"
112+
+ " 'ok': 1"
113+
+ "}"
114+
);
115+
ClientBulkWriteOptions options = ClientBulkWriteOptions.clientBulkWriteOptions()
116+
.ordered(false).verboseResults(false);
117+
List<ClientNamespacedReplaceOneModel> clientNamespacedReplaceOneModels = singletonList(ClientNamespacedWriteModel.replaceOne(
118+
NAMESPACE,
119+
Filters.empty(),
120+
new Document(),
121+
clientReplaceOneOptions().upsert(true)
122+
));
123+
ClientBulkWriteOperation op = new ClientBulkWriteOperation(
124+
clientNamespacedReplaceOneModels,
125+
options,
126+
WriteConcern.ACKNOWLEDGED,
127+
false,
128+
getDefaultCodecRegistry());
129+
//when
130+
ClientBulkWriteResult result = op.execute(binding);
131+
132+
//then
133+
assertEquals(
134+
new AcknowledgedSummaryClientBulkWriteResult(0, 1, 0, 0, 0),
135+
result);
136+
}
137+
138+
/**
139+
* This test exists due to SERVER-113026 bug.
140+
*/
141+
//TODO-JAVA-6005
142+
@Test
143+
void shouldUseDefaultNumberOfModifiedDocumentsWhenMissingInCursor() {
144+
//given
145+
mockCommandExecutionResult("{"
146+
+ " cursor: {"
147+
+ " id: NumberLong(0),"
148+
+ " firstBatch: [ {"
149+
+ " 'ok': 1.0,"
150+
+ " 'idx': 0,"
151+
+ " 'n': 1,"
152+
//nModified field is missing here
153+
+ " 'upserted': {"
154+
+ " '_id': 1"
155+
+ " }"
156+
+ " }],"
157+
+ " ns: 'admin.$cmd.bulkWrite'"
158+
+ " },"
159+
+ " nErrors: 0,"
160+
+ " nInserted: 1,"
161+
+ " nMatched: 0,"
162+
+ " nModified: 0,"
163+
+ " nUpserted: 1,"
164+
+ " nDeleted: 0,"
165+
+ " ok: 1"
166+
+ "}");
167+
ClientBulkWriteOptions options = ClientBulkWriteOptions.clientBulkWriteOptions()
168+
.ordered(false).verboseResults(true);
169+
List<ClientNamespacedReplaceOneModel> clientNamespacedReplaceOneModels = singletonList(ClientNamespacedWriteModel.replaceOne(
170+
NAMESPACE,
171+
Filters.empty(),
172+
new Document(),
173+
clientReplaceOneOptions().upsert(true)
174+
));
175+
ClientBulkWriteOperation op = new ClientBulkWriteOperation(
176+
clientNamespacedReplaceOneModels,
177+
options,
178+
WriteConcern.ACKNOWLEDGED,
179+
false,
180+
getDefaultCodecRegistry());
181+
//when
182+
ClientBulkWriteResult result = op.execute(binding);
183+
184+
//then
185+
assertEquals(1, result.getInsertedCount());
186+
assertEquals(1, result.getUpsertedCount());
187+
assertEquals(0, result.getMatchedCount());
188+
assertEquals(0, result.getModifiedCount());
189+
assertEquals(0, result.getDeletedCount());
190+
assertTrue(result.getVerboseResults().isPresent());
191+
}
192+
193+
private void mockCommandExecutionResult(final String serverResponse) {
194+
doAnswer(invocationOnMock -> {
195+
DualMessageSequences dualMessageSequences = invocationOnMock.getArgument(7);
196+
dualMessageSequences.encodeDocuments(write -> {
197+
write.doAndGetBatchCount(new BsonBinaryWriter(new BasicOutputBuffer()), new BsonBinaryWriter(new BasicOutputBuffer()));
198+
return DualMessageSequences.WritersProviderAndLimitsChecker.WriteResult.OK_LIMIT_NOT_REACHED;
199+
});
200+
return toBsonDocument(serverResponse);
201+
}).when(connection).command(
202+
anyString(),
203+
any(BsonDocument.class),
204+
any(),
205+
isNull(),
206+
any(),
207+
any(OperationContext.class),
208+
anyBoolean(),
209+
any(DualMessageSequences.class)
210+
);
211+
}
212+
213+
private static BsonDocument toBsonDocument(final String serverResponse) {
214+
Codec<BsonDocument> bsonDocumentCodec =
215+
CommandResultDocumentCodec.create(getDefaultCodecRegistry().get(BsonDocument.class), CommandBatchCursorHelper.FIRST_BATCH);
216+
return bsonDocumentCodec.decode(new JsonReader(serverResponse), DecoderContext.builder().build());
217+
}
218+
}

0 commit comments

Comments
 (0)