Skip to content

Commit 6d20781

Browse files
jyeminjyemin
committed
Optimize check for an empty update document
The CRUD specification requires that an update contains at least one field, in order to check for application bugs. Because once the command makes it to the server it can't be distinguished from a replace (where an empty document is legal). The current method of checking for an empty document is to call BsonDocument#isEmpty. But that has the unfortunate affect of hydrating a BsonDocumentWrapper into a full BsonDocument. We really want to avoid that hit. To do so, we need another way of checking for an empty document. We use the technique of wrapping the BsonWriter with a FieldTracking BsonWriter, which tracks whether at least one field is written. BulkWriteBatch then checks that and throws if it's not. This also fixes a bug in which that check was done for both update and replace requests. An empty replace document, while rare, is legal. Thanks to @astral303 for finding this issue and for opening an initial PR that provided inspiration for this fix. JAVA-3328
1 parent b744eff commit 6d20781

File tree

3 files changed

+333
-4
lines changed

3 files changed

+333
-4
lines changed

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

Lines changed: 309 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,309 @@
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.connection;
18+
19+
import org.bson.BsonBinary;
20+
import org.bson.BsonDbPointer;
21+
import org.bson.BsonReader;
22+
import org.bson.BsonRegularExpression;
23+
import org.bson.BsonTimestamp;
24+
import org.bson.BsonWriter;
25+
import org.bson.types.Decimal128;
26+
import org.bson.types.ObjectId;
27+
28+
// Helper class to help determine when an update document contains any fields
29+
// It's an imperfect check because we can't tell if the pipe method ended up writing any fields.
30+
// For the purposes of the check, it's better to assume that pipe does end up writing a field, in order to avoid
31+
// incorrectly reporting an error any time pipe is used
32+
public class FieldTrackingBsonWriter extends BsonWriterDecorator {
33+
34+
private boolean hasWrittenField;
35+
private boolean topLevelDocumentWritten;
36+
37+
public FieldTrackingBsonWriter(final BsonWriter bsonWriter) {
38+
super(bsonWriter);
39+
}
40+
41+
public boolean hasWrittenField() {
42+
return hasWrittenField;
43+
}
44+
45+
@Override
46+
public void writeStartDocument(final String name) {
47+
if (topLevelDocumentWritten) {
48+
hasWrittenField = true;
49+
}
50+
super.writeStartDocument(name);
51+
}
52+
53+
@Override
54+
public void writeStartDocument() {
55+
if (topLevelDocumentWritten) {
56+
hasWrittenField = true;
57+
}
58+
topLevelDocumentWritten = true;
59+
super.writeStartDocument();
60+
}
61+
62+
@Override
63+
public void writeStartArray(final String name) {
64+
hasWrittenField = true;
65+
super.writeStartArray(name);
66+
}
67+
68+
@Override
69+
public void writeStartArray() {
70+
hasWrittenField = true;
71+
super.writeStartArray();
72+
}
73+
74+
@Override
75+
public void writeBinaryData(final String name, final BsonBinary binary) {
76+
hasWrittenField = true;
77+
super.writeBinaryData(name, binary);
78+
}
79+
80+
@Override
81+
public void writeBinaryData(final BsonBinary binary) {
82+
hasWrittenField = true;
83+
super.writeBinaryData(binary);
84+
}
85+
86+
@Override
87+
public void writeBoolean(final String name, final boolean value) {
88+
hasWrittenField = true;
89+
super.writeBoolean(name, value);
90+
}
91+
92+
@Override
93+
public void writeBoolean(final boolean value) {
94+
hasWrittenField = true;
95+
super.writeBoolean(value);
96+
}
97+
98+
@Override
99+
public void writeDateTime(final String name, final long value) {
100+
hasWrittenField = true;
101+
super.writeDateTime(name, value);
102+
}
103+
104+
@Override
105+
public void writeDateTime(final long value) {
106+
hasWrittenField = true;
107+
super.writeDateTime(value);
108+
}
109+
110+
@Override
111+
public void writeDBPointer(final String name, final BsonDbPointer value) {
112+
hasWrittenField = true;
113+
super.writeDBPointer(name, value);
114+
}
115+
116+
@Override
117+
public void writeDBPointer(final BsonDbPointer value) {
118+
hasWrittenField = true;
119+
super.writeDBPointer(value);
120+
}
121+
122+
@Override
123+
public void writeDouble(final String name, final double value) {
124+
hasWrittenField = true;
125+
super.writeDouble(name, value);
126+
}
127+
128+
@Override
129+
public void writeDouble(final double value) {
130+
hasWrittenField = true;
131+
super.writeDouble(value);
132+
}
133+
134+
@Override
135+
public void writeInt32(final String name, final int value) {
136+
hasWrittenField = true;
137+
super.writeInt32(name, value);
138+
}
139+
140+
@Override
141+
public void writeInt32(final int value) {
142+
hasWrittenField = true;
143+
super.writeInt32(value);
144+
}
145+
146+
@Override
147+
public void writeInt64(final String name, final long value) {
148+
super.writeInt64(name, value);
149+
hasWrittenField = true;
150+
}
151+
152+
@Override
153+
public void writeInt64(final long value) {
154+
hasWrittenField = true;
155+
super.writeInt64(value);
156+
}
157+
158+
@Override
159+
public void writeDecimal128(final Decimal128 value) {
160+
hasWrittenField = true;
161+
super.writeDecimal128(value);
162+
}
163+
164+
@Override
165+
public void writeDecimal128(final String name, final Decimal128 value) {
166+
hasWrittenField = true;
167+
super.writeDecimal128(name, value);
168+
}
169+
170+
@Override
171+
public void writeJavaScript(final String name, final String code) {
172+
hasWrittenField = true;
173+
super.writeJavaScript(name, code);
174+
}
175+
176+
@Override
177+
public void writeJavaScript(final String code) {
178+
hasWrittenField = true;
179+
super.writeJavaScript(code);
180+
}
181+
182+
@Override
183+
public void writeJavaScriptWithScope(final String name, final String code) {
184+
super.writeJavaScriptWithScope(name, code);
185+
hasWrittenField = true;
186+
}
187+
188+
@Override
189+
public void writeJavaScriptWithScope(final String code) {
190+
hasWrittenField = true;
191+
super.writeJavaScriptWithScope(code);
192+
}
193+
194+
@Override
195+
public void writeMaxKey(final String name) {
196+
hasWrittenField = true;
197+
super.writeMaxKey(name);
198+
}
199+
200+
@Override
201+
public void writeMaxKey() {
202+
hasWrittenField = true;
203+
super.writeMaxKey();
204+
}
205+
206+
@Override
207+
public void writeMinKey(final String name) {
208+
hasWrittenField = true;
209+
super.writeMinKey(name);
210+
}
211+
212+
@Override
213+
public void writeMinKey() {
214+
hasWrittenField = true;
215+
super.writeMinKey();
216+
}
217+
218+
@Override
219+
public void writeNull(final String name) {
220+
hasWrittenField = true;
221+
super.writeNull(name);
222+
}
223+
224+
@Override
225+
public void writeNull() {
226+
hasWrittenField = true;
227+
super.writeNull();
228+
}
229+
230+
@Override
231+
public void writeObjectId(final String name, final ObjectId objectId) {
232+
hasWrittenField = true;
233+
super.writeObjectId(name, objectId);
234+
}
235+
236+
@Override
237+
public void writeObjectId(final ObjectId objectId) {
238+
hasWrittenField = true;
239+
super.writeObjectId(objectId);
240+
}
241+
242+
@Override
243+
public void writeRegularExpression(final String name, final BsonRegularExpression regularExpression) {
244+
hasWrittenField = true;
245+
super.writeRegularExpression(name, regularExpression);
246+
}
247+
248+
@Override
249+
public void writeRegularExpression(final BsonRegularExpression regularExpression) {
250+
hasWrittenField = true;
251+
super.writeRegularExpression(regularExpression);
252+
}
253+
254+
@Override
255+
public void writeString(final String name, final String value) {
256+
hasWrittenField = true;
257+
super.writeString(name, value);
258+
}
259+
260+
@Override
261+
public void writeString(final String value) {
262+
hasWrittenField = true;
263+
super.writeString(value);
264+
}
265+
266+
@Override
267+
public void writeSymbol(final String name, final String value) {
268+
hasWrittenField = true;
269+
super.writeSymbol(name, value);
270+
}
271+
272+
@Override
273+
public void writeSymbol(final String value) {
274+
hasWrittenField = true;
275+
super.writeSymbol(value);
276+
}
277+
278+
@Override
279+
public void writeTimestamp(final String name, final BsonTimestamp value) {
280+
hasWrittenField = true;
281+
super.writeTimestamp(name, value);
282+
}
283+
284+
@Override
285+
public void writeTimestamp(final BsonTimestamp value) {
286+
hasWrittenField = true;
287+
super.writeTimestamp(value);
288+
}
289+
290+
@Override
291+
public void writeUndefined(final String name) {
292+
hasWrittenField = true;
293+
super.writeUndefined(name);
294+
}
295+
296+
@Override
297+
public void writeUndefined() {
298+
hasWrittenField = true;
299+
super.writeUndefined();
300+
}
301+
302+
@Override
303+
public void pipe(final BsonReader reader) {
304+
// this is a faulty assumption, as we may end up piping an empty document. But if we don't increment here, we may undercount,
305+
// which in this context is worse since we'll thrown an exception when we should not.
306+
hasWrittenField = true;
307+
super.pipe(reader);
308+
}
309+
}

‎driver-core/src/main/com/mongodb/operation/BulkWriteBatch.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import com.mongodb.connection.ConnectionDescription;
3434
import com.mongodb.connection.ServerDescription;
3535
import com.mongodb.connection.SplittablePayload;
36+
import com.mongodb.internal.connection.FieldTrackingBsonWriter;
3637
import com.mongodb.internal.connection.IndexMap;
3738
import com.mongodb.internal.validator.CollectibleDocumentFieldNameValidator;
3839
import com.mongodb.internal.validator.MappedFieldNameValidator;
@@ -392,16 +393,18 @@ public void encode(final BsonWriter writer, final WriteRequest writeRequest, fin
392393
if (!updateValue.isDocument() && !updateValue.isArray()) {
393394
throw new IllegalArgumentException("Invalid BSON value for an update.");
394395
}
395-
if (updateValue.isDocument() && updateValue.asDocument().isEmpty()) {
396-
throw new IllegalArgumentException("Invalid BSON document for an update. The document may not be empty.");
397-
}
398396
if (updateValue.isArray() && updateValue.asArray().isEmpty()) {
399397
throw new IllegalArgumentException("Invalid pipeline for an update. The pipeline may not be empty.");
400398
}
401399

402400
writer.writeName("u");
403401
if (updateValue.isDocument()) {
404-
getCodec(updateValue.asDocument()).encode(writer, updateValue.asDocument(), EncoderContext.builder().build());
402+
FieldTrackingBsonWriter fieldTrackingBsonWriter = new FieldTrackingBsonWriter(writer);
403+
getCodec(updateValue.asDocument()).encode(fieldTrackingBsonWriter, updateValue.asDocument(),
404+
EncoderContext.builder().build());
405+
if (writeRequest.getType() == UPDATE && !fieldTrackingBsonWriter.hasWrittenField()) {
406+
throw new IllegalArgumentException("Invalid BSON document for an update. The document may not be empty.");
407+
}
405408
} else if (update.getType() == WriteRequest.Type.UPDATE && updateValue.isArray()) {
406409
writer.writeStartArray();
407410
for (BsonValue cur : updateValue.asArray()) {

‎driver-core/src/test/functional/com/mongodb/operation/MixedBulkWriteOperationSpecification.groovy

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,23 @@ class MixedBulkWriteOperationSpecification extends OperationFunctionalSpecificat
303303
[async, ordered] << [[true, false], [true, false]].combinations()
304304
}
305305

306+
def 'when replacing with an empty document, update should not throw IllegalArgumentException'() {
307+
given:
308+
def id = new ObjectId()
309+
def operation = new MixedBulkWriteOperation(getNamespace(),
310+
[new UpdateRequest(new BsonDocument('_id', new BsonObjectId(id)), new BsonDocument(), REPLACE)],
311+
true, ACKNOWLEDGED, false)
312+
313+
when:
314+
execute(operation, async)
315+
316+
then:
317+
noExceptionThrown()
318+
319+
where:
320+
[async, ordered] << [[true, false], [true, false]].combinations()
321+
}
322+
306323
def 'when updating with an invalid document, update should throw IllegalArgumentException'() {
307324
given:
308325
def id = new ObjectId()

0 commit comments

Comments
 (0)