Skip to content

Commit 96d0c6b

Browse files
vbabaninrozza
andauthored
Remove assertion for verbose result (#1835)
- 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 <[email protected]>
1 parent 486b41d commit 96d0c6b

File tree

3 files changed

+271
-26
lines changed

3 files changed

+271
-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
@@ -772,33 +772,25 @@ ClientBulkWriteResult build(@Nullable final MongoException topLevelError, final
772772
deletedCount += response.getNDeleted();
773773
Map<Integer, BsonValue> insertModelDocumentIds = batchResult.getInsertModelDocumentIds();
774774
for (BsonDocument individualOperationResponse : response.getCursorExhaust()) {
775+
boolean individualOperationSuccessful = individualOperationResponse.getNumber("ok").intValue() == 1;
776+
if (individualOperationSuccessful && !verboseResultsSetting) {
777+
//TODO-JAVA-6002 Previously, assertTrue(verboseResultsSetting) was used when ok == 1 because the server
778+
// was not supposed to return successful operation results in the cursor when verboseResultsSetting is false.
779+
// Due to server bug SERVER-113344, these unexpected results must be ignored until we stop supporting server
780+
// versions affected by this bug. When that happens, restore assertTrue(verboseResultsSetting).
781+
continue;
782+
}
775783
int individualOperationIndexInBatch = individualOperationResponse.getInt32("idx").getValue();
776784
int writeModelIndex = batchStartModelIndex + individualOperationIndexInBatch;
777-
if (individualOperationResponse.getNumber("ok").intValue() == 1) {
778-
assertTrue(verboseResultsSetting);
779-
AbstractClientNamespacedWriteModel writeModel = getNamespacedModel(models, writeModelIndex);
780-
if (writeModel instanceof ConcreteClientNamespacedInsertOneModel) {
781-
insertResults.put(
782-
writeModelIndex,
783-
new ConcreteClientInsertOneResult(insertModelDocumentIds.get(individualOperationIndexInBatch)));
784-
} else if (writeModel instanceof ConcreteClientNamespacedUpdateOneModel
785-
|| writeModel instanceof ConcreteClientNamespacedUpdateManyModel
786-
|| writeModel instanceof ConcreteClientNamespacedReplaceOneModel) {
787-
BsonDocument upsertedIdDocument = individualOperationResponse.getDocument("upserted", null);
788-
updateResults.put(
789-
writeModelIndex,
790-
new ConcreteClientUpdateResult(
791-
individualOperationResponse.getInt32("n").getValue(),
792-
individualOperationResponse.getInt32("nModified", new BsonInt32(0)).getValue(),
793-
upsertedIdDocument == null ? null : upsertedIdDocument.get("_id")));
794-
} else if (writeModel instanceof ConcreteClientNamespacedDeleteOneModel
795-
|| writeModel instanceof ConcreteClientNamespacedDeleteManyModel) {
796-
deleteResults.put(
797-
writeModelIndex,
798-
new ConcreteClientDeleteResult(individualOperationResponse.getInt32("n").getValue()));
799-
} else {
800-
fail(writeModel.getClass().toString());
801-
}
785+
if (individualOperationSuccessful) {
786+
collectSuccessfulIndividualOperationResult(
787+
individualOperationResponse,
788+
writeModelIndex,
789+
individualOperationIndexInBatch,
790+
insertModelDocumentIds,
791+
insertResults,
792+
updateResults,
793+
deleteResults);
802794
} else {
803795
batchResultsHaveInfoAboutSuccessfulIndividualOperations = batchResultsHaveInfoAboutSuccessfulIndividualOperations
804796
|| (orderedSetting && individualOperationIndexInBatch > 0);
@@ -838,6 +830,42 @@ ClientBulkWriteResult build(@Nullable final MongoException topLevelError, final
838830
}
839831
}
840832

833+
private void collectSuccessfulIndividualOperationResult(final BsonDocument individualOperationResponse,
834+
final int writeModelIndex,
835+
final int individualOperationIndexInBatch,
836+
final Map<Integer, BsonValue> insertModelDocumentIds,
837+
final Map<Integer, ClientInsertOneResult> insertResults,
838+
final Map<Integer, ClientUpdateResult> updateResults,
839+
final Map<Integer, ClientDeleteResult> deleteResults) {
840+
AbstractClientNamespacedWriteModel writeModel = getNamespacedModel(models, writeModelIndex);
841+
if (writeModel instanceof ConcreteClientNamespacedInsertOneModel) {
842+
insertResults.put(
843+
writeModelIndex,
844+
new ConcreteClientInsertOneResult(insertModelDocumentIds.get(individualOperationIndexInBatch)));
845+
} else if (writeModel instanceof ConcreteClientNamespacedUpdateOneModel
846+
|| writeModel instanceof ConcreteClientNamespacedUpdateManyModel
847+
|| writeModel instanceof ConcreteClientNamespacedReplaceOneModel) {
848+
BsonDocument upsertedIdDocument = individualOperationResponse.getDocument("upserted", null);
849+
updateResults.put(
850+
writeModelIndex,
851+
new ConcreteClientUpdateResult(
852+
individualOperationResponse.getInt32("n").getValue(),
853+
//TODO-JAVA-6005 Previously, we did not provide a default value of 0 because the
854+
// server was supposed to return nModified as 0 when no documents were changed.
855+
// Due to server bug SERVER-113026, we must provide a default of 0 until we stop supporting
856+
// server versions affected by this bug. When that happens, remove the default value for nModified.
857+
individualOperationResponse.getInt32("nModified", new BsonInt32(0)).getValue(),
858+
upsertedIdDocument == null ? null : upsertedIdDocument.get("_id")));
859+
} else if (writeModel instanceof ConcreteClientNamespacedDeleteOneModel
860+
|| writeModel instanceof ConcreteClientNamespacedDeleteManyModel) {
861+
deleteResults.put(
862+
writeModelIndex,
863+
new ConcreteClientDeleteResult(individualOperationResponse.getInt32("n").getValue()));
864+
} else {
865+
fail(writeModel.getClass().toString());
866+
}
867+
}
868+
841869
void onNewServerAddress(final ServerAddress serverAddress) {
842870
this.serverAddress = serverAddress;
843871
}
Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
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.ClusterFixture;
20+
import com.mongodb.MongoNamespace;
21+
import com.mongodb.ServerAddress;
22+
import com.mongodb.WriteConcern;
23+
import com.mongodb.client.model.Filters;
24+
import com.mongodb.client.model.bulk.ClientBulkWriteOptions;
25+
import com.mongodb.client.model.bulk.ClientBulkWriteResult;
26+
import com.mongodb.client.model.bulk.ClientNamespacedReplaceOneModel;
27+
import com.mongodb.client.model.bulk.ClientNamespacedWriteModel;
28+
import com.mongodb.connection.ClusterId;
29+
import com.mongodb.connection.ConnectionDescription;
30+
import com.mongodb.connection.ServerConnectionState;
31+
import com.mongodb.connection.ServerDescription;
32+
import com.mongodb.connection.ServerId;
33+
import com.mongodb.connection.ServerType;
34+
import com.mongodb.internal.binding.ConnectionSource;
35+
import com.mongodb.internal.binding.ReadWriteBinding;
36+
import com.mongodb.internal.client.model.bulk.AcknowledgedSummaryClientBulkWriteResult;
37+
import com.mongodb.internal.connection.Connection;
38+
import com.mongodb.internal.connection.DualMessageSequences;
39+
import com.mongodb.internal.connection.OperationContext;
40+
import com.mongodb.internal.mockito.MongoMockito;
41+
import org.bson.BsonBinaryWriter;
42+
import org.bson.BsonDocument;
43+
import org.bson.Document;
44+
import org.bson.codecs.Codec;
45+
import org.bson.codecs.DecoderContext;
46+
import org.bson.io.BasicOutputBuffer;
47+
import org.bson.json.JsonReader;
48+
import org.junit.jupiter.api.BeforeEach;
49+
import org.junit.jupiter.api.Test;
50+
51+
import java.util.List;
52+
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(any(OperationContext.class));
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(connectionSource).when(binding).getWriteConnectionSource(any(OperationContext.class));
87+
}
88+
89+
90+
/**
91+
* This test exists due to SERVER-113344 bug.
92+
*/
93+
//TODO-JAVA-6002
94+
@Test
95+
void shouldIgnoreSuccessfulCursorResultWhenVerboseResultIsFalse() {
96+
//given
97+
mockCommandExecutionResult(
98+
"{'cursor': {"
99+
+ " 'id': NumberLong(0),"
100+
+ " 'firstBatch': [ { 'ok': 1, 'idx': 0, 'n': 1, 'upserted': { '_id': 1 } } ],"
101+
+ " 'ns': 'admin.$cmd.bulkWrite'"
102+
+ "},"
103+
+ " 'nErrors': 0,"
104+
+ " 'nInserted': 0,"
105+
+ " 'nMatched': 0,"
106+
+ " 'nModified': 0,"
107+
+ " 'nUpserted': 1,"
108+
+ " 'nDeleted': 0,"
109+
+ " 'ok': 1"
110+
+ "}"
111+
);
112+
ClientBulkWriteOptions options = ClientBulkWriteOptions.clientBulkWriteOptions()
113+
.ordered(false).verboseResults(false);
114+
List<ClientNamespacedReplaceOneModel> clientNamespacedReplaceOneModels = singletonList(ClientNamespacedWriteModel.replaceOne(
115+
NAMESPACE,
116+
Filters.empty(),
117+
new Document(),
118+
clientReplaceOneOptions().upsert(true)
119+
));
120+
ClientBulkWriteOperation op = new ClientBulkWriteOperation(
121+
clientNamespacedReplaceOneModels,
122+
options,
123+
WriteConcern.ACKNOWLEDGED,
124+
false,
125+
getDefaultCodecRegistry());
126+
//when
127+
ClientBulkWriteResult result = op.execute(binding, ClusterFixture.OPERATION_CONTEXT);
128+
129+
//then
130+
assertEquals(
131+
new AcknowledgedSummaryClientBulkWriteResult(0, 1, 0, 0, 0),
132+
result);
133+
}
134+
135+
/**
136+
* This test exists due to SERVER-113026 bug.
137+
*/
138+
//TODO-JAVA-6005
139+
@Test
140+
void shouldUseDefaultNumberOfModifiedDocumentsWhenMissingInCursor() {
141+
//given
142+
mockCommandExecutionResult("{"
143+
+ " cursor: {"
144+
+ " id: NumberLong(0),"
145+
+ " firstBatch: [ {"
146+
+ " 'ok': 1.0,"
147+
+ " 'idx': 0,"
148+
+ " 'n': 1,"
149+
//nModified field is missing here
150+
+ " 'upserted': {"
151+
+ " '_id': 1"
152+
+ " }"
153+
+ " }],"
154+
+ " ns: 'admin.$cmd.bulkWrite'"
155+
+ " },"
156+
+ " nErrors: 0,"
157+
+ " nInserted: 1,"
158+
+ " nMatched: 0,"
159+
+ " nModified: 0,"
160+
+ " nUpserted: 1,"
161+
+ " nDeleted: 0,"
162+
+ " ok: 1"
163+
+ "}");
164+
ClientBulkWriteOptions options = ClientBulkWriteOptions.clientBulkWriteOptions()
165+
.ordered(false).verboseResults(true);
166+
List<ClientNamespacedReplaceOneModel> clientNamespacedReplaceOneModels = singletonList(ClientNamespacedWriteModel.replaceOne(
167+
NAMESPACE,
168+
Filters.empty(),
169+
new Document(),
170+
clientReplaceOneOptions().upsert(true)
171+
));
172+
ClientBulkWriteOperation op = new ClientBulkWriteOperation(
173+
clientNamespacedReplaceOneModels,
174+
options,
175+
WriteConcern.ACKNOWLEDGED,
176+
false,
177+
getDefaultCodecRegistry());
178+
//when
179+
ClientBulkWriteResult result = op.execute(binding, ClusterFixture.OPERATION_CONTEXT);
180+
181+
//then
182+
assertEquals(1, result.getInsertedCount());
183+
assertEquals(1, result.getUpsertedCount());
184+
assertEquals(0, result.getMatchedCount());
185+
assertEquals(0, result.getModifiedCount());
186+
assertEquals(0, result.getDeletedCount());
187+
assertTrue(result.getVerboseResults().isPresent());
188+
}
189+
190+
private void mockCommandExecutionResult(final String serverResponse) {
191+
doAnswer(invocationOnMock -> {
192+
DualMessageSequences dualMessageSequences = invocationOnMock.getArgument(7);
193+
dualMessageSequences.encodeDocuments(write -> {
194+
write.doAndGetBatchCount(new BsonBinaryWriter(new BasicOutputBuffer()), new BsonBinaryWriter(new BasicOutputBuffer()));
195+
return DualMessageSequences.WritersProviderAndLimitsChecker.WriteResult.OK_LIMIT_NOT_REACHED;
196+
});
197+
return toBsonDocument(serverResponse);
198+
}).when(connection).command(
199+
anyString(),
200+
any(BsonDocument.class),
201+
any(),
202+
isNull(),
203+
any(),
204+
any(OperationContext.class),
205+
anyBoolean(),
206+
any(DualMessageSequences.class)
207+
);
208+
}
209+
210+
private static BsonDocument toBsonDocument(final String serverResponse) {
211+
Codec<BsonDocument> bsonDocumentCodec =
212+
CommandResultDocumentCodec.create(getDefaultCodecRegistry().get(BsonDocument.class), CommandBatchCursorHelper.FIRST_BATCH);
213+
return bsonDocumentCodec.decode(new JsonReader(serverResponse), DecoderContext.builder().build());
214+
}
215+
}

0 commit comments

Comments
 (0)