Follow-up to PR 136 #151
Follow-up to PR 136 #151
Conversation
| private static final BulkWriteResult BULK_WRITE_RESULT = BulkWriteResult.acknowledged( | ||
| 1, 0, 2, 3, emptyList(), List.of(new BulkWriteInsert(0, new BsonObjectId(new ObjectId(1, 2))))); | ||
| private static final BulkWriteResult BULK_WRITE_RESULT = | ||
| BulkWriteResult.acknowledged(0, 0, 0, 0, emptyList(), emptyList()); |
There was a problem hiding this comment.
There seems to be no reason to make BULK_WRITE_RESULT any more complex than this.
| @ParameterizedTest(name = "test executeBatch MongoException {0}") | ||
| @MethodSource({"genericMongoExceptions", "timeoutExceptions"}) | ||
| void testExecuteBatchMongoException(MongoException mongoException) throws SQLException { | ||
| doThrow(mongoException).when(mongoCollection).bulkWrite(eq(clientSession), anyList()); | ||
|
|
||
| assertExecuteBatchThrowsSqlException(sqlException -> { | ||
| assertThatObject(sqlException) | ||
| .returns(mongoException.getCode(), SQLException::getErrorCode) | ||
| .returns(null, SQLException::getSQLState) | ||
| .returns(mongoException, SQLException::getCause); | ||
| }); | ||
| assertExecuteBatchThrowsSqlException( | ||
| sqlException -> assertGenericMongoException(sqlException, mongoException)); |
There was a problem hiding this comment.
I removed testExecuteBatchTimeoutException, and made testExecuteBatchMongoException similar to testExecuteUpdateMongoException and testExecuteQueryMongoException. I suspect, there was a point before PR 136 was merged at which the split made sense.
| createMongoBulkWriteException(1), // failed model index | ||
| createMongoBulkWriteException(0), // failed model index | ||
| 0), // expected update count length | ||
| Arguments.of(mqlCommand, createMongoBulkWriteException(1), 0), |
There was a problem hiding this comment.
The case with the first model (index 0) failing wasn't present. Probably because it was causing an assertion, as it was using the invalid error code 0 due to the method createMongoBulkWriteException being a bit incorrect. I fixed that and introduced the missing case.
| } | ||
| } | ||
|
|
||
| private static MongoBulkWriteException createMongoBulkWriteException(int errorCode, int failedModelIndex) { |
There was a problem hiding this comment.
This method wasn't used.
| BULK_WRITE_RESULT, | ||
| List.of(new BulkWriteError( | ||
| failedModelIndex, DUMMY_EXCEPTION_MESSAGE, DUMMY_ERROR_DETAILS, failedModelIndex)), | ||
| List.of(new BulkWriteError(1, DUMMY_EXCEPTION_MESSAGE, DUMMY_ERROR_DETAILS, failedModelIndex)), |
There was a problem hiding this comment.
We should not use failedModelIndex as the error code, because failedModelIndex may be 0, but the error code must not.
| if (pipeline.isEmpty()) { | ||
| throw createSyntaxErrorException("%s. $project stage is missing [%s]", command, null); | ||
| } | ||
| var projectStageIndex = pipeline.size() - 1; |
There was a problem hiding this comment.
Checking pipeline, then using it seems more natural.
There was a problem hiding this comment.
The changes to this file are unrelated to PR 136. There are a few other simple unrelated changes.
There was a problem hiding this comment.
The changes to this file are unrelated to PR 136.
|
|
||
| private static String getFieldName(String unsupportedField) { | ||
| return BsonDocument.parse("{" + unsupportedField + "}").getFirstKey(); | ||
| private static String getFieldName(String field) { |
There was a problem hiding this comment.
This method does not care whether the argument passed is a supported field or not.
|
|
||
| @Test | ||
| void testAbsentRequiredAggregateCommandField() { | ||
| void testMissingRequiredAggregateCommandField() { |
There was a problem hiding this comment.
Replaced "absent" with "missing", as that's what the rest of the codebase uses:
$project stage is missingCommand name is missingCollection name is missingStructAggregateEmbeddableIntegrationTests/EmbeddableIntegrationTests.testReadNestedValuesMissingFields
1 participant
This is a follow-up to #136 (review).
Reviewing the changes commit by commit will likely simplify the process.