Fix `MixedBulkWriteOperation` such that it does not leak `MongoWriteConcernWithResponseException` to users #1051

stIncMale · 2022-11-18T21:08:39Z

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java

stIncMale · 2022-11-18T21:11:54Z

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java

+            assertThrows(MongoWriteConcernException.class, () -> {
+                try {
+                    collection.insertOne(new Document());
+                } catch (MongoWriteConcernException e) {
+                    assertEquals(91, e.getCode());
+                    throw e;
+                }
+            });


Now that the leak is fixed, we can expect MongoWriteConcernException here.

FYI this can also be written as:

MongoWriteConcernException e = assertThrows(MongoWriteConcernException.class, () -> collection.insertOne(new Document())); assertEquals(91, e.getCode());

Thank you, I have not realized assertThrows returns the thrown exception :) Done.

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

stIncMale · 2022-11-18T21:25:52Z

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

@@ -292,9 +271,10 @@ private BulkWriteResult executeBulkWriteBatch(final RetryState retryState, final
                currentBulkWriteTracker = BulkWriteTracker.attachNext(retryState, currentBatch);
                currentBatch = currentBulkWriteTracker.batch().orElseThrow(Assertions::fail);
            } catch (MongoException exception) {
-                if (!(retryState.isFirstAttempt() || (exception instanceof MongoWriteConcernWithResponseException))) {
+                if (!retryState.isFirstAttempt() && !(exception instanceof MongoWriteConcernWithResponseException)) {


Changed this because I found the new form easier to understand (I was trying to understand what's going on, not very successfully, though).

Should there be a separate catch for MongoWriteConcernWithResponseException to make this easier to understand?

Currently we have

} catch (MongoException exception) { if (!retryState.isFirstAttempt() && !(exception instanceof MongoWriteConcernWithResponseException)) { addRetryableWriteErrorLabel(exception, maxWireVersion); } handleMongoWriteConcernWithResponseException(retryState, false); throw exception; }

with the change you propose we will have

} catch (MongoWriteConcernWithResponseException exception) { handleMongoWriteConcernWithResponseException(retryState, false); throw exception; } catch (MongoException exception) { if (!retryState.isFirstAttempt()) { addRetryableWriteErrorLabel(exception, maxWireVersion); } handleMongoWriteConcernWithResponseException(retryState, false); throw exception; }

The current code is shorter and does not have code duplication, the proposed code does not have the instanceof check. To me the current approach seems the lesser of two evils.

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

…ernWithResponseException to users JAVA-4775

rozza

Looks good - added a couple of comments for consideration only

rozza · 2022-11-21T10:24:20Z

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java

+            assertThrows(MongoWriteConcernException.class, () -> {
+                try {
+                    collection.insertOne(new Document());
+                } catch (MongoWriteConcernException e) {
+                    assertEquals(91, e.getCode());
+                    throw e;
+                }
+            });


FYI this can also be written as:

MongoWriteConcernException e = assertThrows(MongoWriteConcernException.class, () -> collection.insertOne(new Document())); assertEquals(91, e.getCode());

rozza · 2022-11-21T10:36:23Z

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

@@ -292,9 +271,10 @@ private BulkWriteResult executeBulkWriteBatch(final RetryState retryState, final
                currentBulkWriteTracker = BulkWriteTracker.attachNext(retryState, currentBatch);
                currentBatch = currentBulkWriteTracker.batch().orElseThrow(Assertions::fail);
            } catch (MongoException exception) {
-                if (!(retryState.isFirstAttempt() || (exception instanceof MongoWriteConcernWithResponseException))) {
+                if (!retryState.isFirstAttempt() && !(exception instanceof MongoWriteConcernWithResponseException)) {


Should there be a separate catch for MongoWriteConcernWithResponseException to make this easier to understand?

...-sync/src/test/functional/com/mongodb/client/MongoWriteConcernWithResponseExceptionTest.java

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

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java

...-sync/src/test/functional/com/mongodb/client/MongoWriteConcernWithResponseExceptionTest.java

jyemin · 2022-11-19T21:25:46Z

...-sync/src/test/functional/com/mongodb/client/MongoWriteConcernWithResponseExceptionTest.java

+            MongoCollection<Document> collection = client.getDatabase(getDefaultDatabaseName())
+                    .getCollection("originalErrorMustBePropagatedIfNoWritesPerformed");
+            collection.drop();
+            assertThrows(MongoWriteConcernException.class, () -> {


Suggested change

assertThrows(MongoWriteConcernException.class, () -> {

assertThrows(MongoWriteConcernException.class, () -> collection.insertOne(new Document()));

If it's asserting that it throws MongoWriteConcernException it can't also throw MongoWriteConcernWithResponseException

assertThrows(MongoWriteConcernException.class, ...) verifies that MongoWriteConcernException is thrown. What if a different exception is thrown? - it simply reports that. I want to have the The internal exception leaked message when the different exception is MongoWriteConcernWithResponseException, and I don't see another way of achieving this. Of course, if this happens, both assertions will fail with the inner assertion being the cause of the outer one.

It seems overly complicated structure just to get a more helpful failure message, but I can live with it.

Co-authored-by: Jeff Yemin <[email protected]>

stIncMale self-assigned this Nov 18, 2022

stIncMale commented Nov 18, 2022

View reviewed changes

driver-sync/src/test/functional/com/mongodb/client/RetryableWritesProseTest.java Show resolved Hide resolved

stIncMale commented Nov 18, 2022

View reviewed changes

driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java Show resolved Hide resolved

stIncMale commented Nov 18, 2022

View reviewed changes

driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java Show resolved Hide resolved

Fix MixedBulkWriteOperation such that it does not leak MongoWriteConc…

a46882b

…ernWithResponseException to users JAVA-4775

stIncMale force-pushed the JAVA-4775 branch from ab49ee5 to a46882b Compare November 18, 2022 23:10

stIncMale requested a review from jyemin November 18, 2022 23:14

rozza approved these changes Nov 21, 2022

View reviewed changes

jyemin requested changes Nov 21, 2022

View reviewed changes

stIncMale and others added 2 commits November 21, 2022 19:30

Apply suggestions from code review

5da59c2

Co-authored-by: Jeff Yemin <[email protected]>

Fix the broken code: the proposed change was problematic

478caf5

stIncMale requested a review from jyemin November 22, 2022 03:03

jyemin approved these changes Nov 22, 2022

View reviewed changes

stIncMale merged commit 3387038 into mongodb:master Nov 22, 2022

stIncMale deleted the JAVA-4775 branch November 22, 2022 15:05

	assertThrows(MongoWriteConcernException.class, () -> {
	assertThrows(MongoWriteConcernException.class, () -> collection.insertOne(new Document()));

Fix MixedBulkWriteOperation such that it does not leak MongoWriteConcernWithResponseException to users #1051

Fix MixedBulkWriteOperation such that it does not leak MongoWriteConcernWithResponseException to users #1051

Uh oh!

Conversation

stIncMale commented Nov 18, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rozza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix `MixedBulkWriteOperation` such that it does not leak `MongoWriteConcernWithResponseException` to users #1051

Fix `MixedBulkWriteOperation` such that it does not leak `MongoWriteConcernWithResponseException` to users #1051