Preserve error code, code name, and error labels when redacting command monitoring/logging #1225
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| /* | ||
| * Copyright 2008-present MongoDB, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.mongodb.internal; | ||
|
|
||
| import com.mongodb.MongoCommandException; | ||
| import org.bson.BsonArray; | ||
| import org.bson.BsonDocument; | ||
| import org.bson.BsonInt32; | ||
| import org.bson.BsonNumber; | ||
| import org.bson.BsonString; | ||
| import org.bson.BsonValue; | ||
|
|
||
| import java.util.Set; | ||
| import java.util.function.Function; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE; | ||
|
|
||
| /** | ||
| * <p>This class is not part of the public API and may be removed or changed at any time</p> | ||
| */ | ||
| public final class Exceptions { | ||
| public static final class MongoCommandExceptions { | ||
vbabanin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| public static int extractErrorCode(final BsonDocument response) { | ||
| return extractErrorCodeAsBson(response).intValue(); | ||
| } | ||
|
|
||
| public static String extractErrorCodeName(final BsonDocument response) { | ||
| return extractErrorCodeNameAsBson(response).getValue(); | ||
| } | ||
|
|
||
| public static BsonArray extractErrorLabelsAsBson(final BsonDocument response) { | ||
|
There was a problem hiding this comment. Optional: move this down near the other There was a problem hiding this comment. Unlike those methods, this one is |
||
| return response.getArray("errorLabels", new BsonArray()); | ||
| } | ||
|
|
||
| /** | ||
| * Constructs a {@link MongoCommandException} with the data from the {@code original} redacted for security purposes. | ||
| */ | ||
| public static MongoCommandException redacted(final MongoCommandException original) { | ||
| BsonDocument originalResponse = original.getResponse(); | ||
| BsonDocument redactedResponse = new BsonDocument(); | ||
| for (SecurityInsensitiveResponseField field : SecurityInsensitiveResponseField.values()) { | ||
| redactedResponse.append(field.fieldName(), field.fieldValue(originalResponse)); | ||
| } | ||
| MongoCommandException result = new MongoCommandException(redactedResponse, original.getServerAddress()); | ||
| result.setStackTrace(original.getStackTrace()); | ||
| return result; | ||
| } | ||
|
|
||
| private static BsonNumber extractErrorCodeAsBson(final BsonDocument response) { | ||
| return response.getNumber("code", new BsonInt32(-1)); | ||
| } | ||
|
|
||
| private static BsonString extractErrorCodeNameAsBson(final BsonDocument response) { | ||
| return response.getString("codeName", new BsonString("")); | ||
| } | ||
|
|
||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| public enum SecurityInsensitiveResponseField { | ||
| CODE("code", MongoCommandExceptions::extractErrorCodeAsBson), | ||
| CODE_NAME("codeName", MongoCommandExceptions::extractErrorCodeNameAsBson), | ||
| ERROR_LABELS("errorLabels", MongoCommandExceptions::extractErrorLabelsAsBson); | ||
|
|
||
| private final String fieldName; | ||
| private final Function<BsonDocument, BsonValue> fieldValueExtractor; | ||
|
|
||
| SecurityInsensitiveResponseField(final String fieldName, final Function<BsonDocument, BsonValue> fieldValueExtractor) { | ||
| this.fieldName = fieldName; | ||
| this.fieldValueExtractor = fieldValueExtractor; | ||
| } | ||
|
|
||
| String fieldName() { | ||
| return fieldName; | ||
| } | ||
|
|
||
| BsonValue fieldValue(final BsonDocument response) { | ||
| return fieldValueExtractor.apply(response); | ||
| } | ||
|
|
||
| @VisibleForTesting(otherwise = PRIVATE) | ||
| public static Set<String> fieldNames() { | ||
| return Stream.of(SecurityInsensitiveResponseField.values()) | ||
| .map(SecurityInsensitiveResponseField::fieldName) | ||
| .collect(Collectors.toSet()); | ||
| } | ||
| } | ||
|
|
||
| private MongoCommandExceptions() { | ||
| } | ||
| } | ||
|
|
||
| private Exceptions() { | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /* | ||
| * Copyright 2008-present MongoDB, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.mongodb.internal; | ||
|
|
||
| import com.mongodb.MongoCommandException; | ||
| import com.mongodb.ServerAddress; | ||
| import com.mongodb.internal.Exceptions.MongoCommandExceptions; | ||
| import org.bson.BsonArray; | ||
| import org.bson.BsonBoolean; | ||
| import org.bson.BsonDocument; | ||
| import org.bson.BsonInt32; | ||
| import org.bson.BsonString; | ||
| import org.junit.jupiter.api.Nested; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.HashSet; | ||
|
|
||
| import static java.util.Arrays.asList; | ||
| import static org.junit.jupiter.api.Assertions.assertArrayEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| final class ExceptionsTest { | ||
| @Nested | ||
| final class MongoCommandExceptionsTest { | ||
| @Test | ||
| void redacted() { | ||
vbabanin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| MongoCommandException original = new MongoCommandException( | ||
| new BsonDocument("ok", BsonBoolean.FALSE) | ||
| .append("code", new BsonInt32(26)) | ||
| .append("codeName", new BsonString("TimeoutError")) | ||
| .append("errorLabels", new BsonArray(asList(new BsonString("label"), new BsonString("label2")))) | ||
| .append("errmsg", new BsonString("err msg")), | ||
| new ServerAddress()); | ||
| MongoCommandException redacted = MongoCommandExceptions.redacted(original); | ||
| assertArrayEquals(original.getStackTrace(), redacted.getStackTrace()); | ||
| String message = redacted.getMessage(); | ||
| assertTrue(message.contains("26")); | ||
| assertTrue(message.contains("TimeoutError")); | ||
| assertTrue(message.contains("label")); | ||
| assertFalse(message.contains("err msg")); | ||
| assertTrue(redacted.getErrorMessage().isEmpty()); | ||
| assertEquals(26, redacted.getErrorCode()); | ||
| assertEquals("TimeoutError", redacted.getErrorCodeName()); | ||
| assertEquals(new HashSet<>(asList("label", "label2")), redacted.getErrorLabels()); | ||
| assertEquals(MongoCommandExceptions.SecurityInsensitiveResponseField.fieldNames(), redacted.getResponse().keySet()); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen this particular nested class pattern before. Is it in anticipation of adding more nested classes in the future for other exception types? Otherwise why not make this a top level class and remove the outer class?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's something we started doing in the Kafka connector:
change.stream.full.document.before.changeconfig property mongo-kafka#121 (comment)Only at that time I didn't know one must use
@Nested, but now IntelliJ warned me and I learned.Yes. Nesting is one more tool for grouping tests: https://stackoverflow.com/questions/36220889/whats-the-purpose-of-the-junit-5-nested-annotation.
P.S. I'll need to add
@Nestedto the Kafka connector codebase.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: mongodb/mongo-kafka@62fedf0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, the
@Nestedannotation in JUnit, along with its underlying pattern, is designed not only for the grouping of test classes under a common parent but also the sharing of initialization/fixtures from that parent test class.Given that we have a static inner class here, I'm curious if there's an intent to introduce static fields or data in the
Exceptionsclass that must be shared withMongoCommandExceptionor potentially with other added classes in the future, thus warranting the coupling?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test classes are nested because the classes they test are. The reasons the tested classes are nested are:
I see two ways of achieving the above:
com.mongodb.internal.exceptionpackage.I chose 1, but I am not opposed to 2. Let me know if you think we should do 2, and I'll refactor the code and the test.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification. I don't have a strong preference, since nesting or packaging serve the same purpose here. Feel free to decide as you see fit.