-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve error code, code name, and error labels when redacting command monitoring/logging #1225
Conversation
…nd monitoring/logging JAVA-4843
* <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 { |
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?
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.
It's something we started doing in the Kafka connector:
- Add the new
change.stream.full.document.before.change
config property mongo-kafka#121 (comment) - https://github.com/mongodb/mongo-kafka/blob/master/src/test/java/com/mongodb/kafka/connect/source/MongoSourceConfigTest.java#L549,
Only at that time I didn't know one must use @Nested
, but now IntelliJ warned me and I learned.
Is it in anticipation of adding more nested classes in the future for other exception types?
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 @Nested
to the Kafka connector codebase.
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.
P.S. I'll need to add
@Nested
to the Kafka connector codebase.
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 @Nested
annotation 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 Exceptions
class that must be shared with MongoCommandException
or 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 want utilities for specific exception types to be separated from utilities for any exception.
- On the other hand, I still want utilities for specific exception types to be somehow grouped together, and grouped with utilities for any exception.
I see two ways of achieving the above:
- Use nested classes, as currently done in the PR.
- Use a new
com.mongodb.internal.exception
package.
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.
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.
return extractErrorCodeNameAsBson(response).getValue(); | ||
} | ||
|
||
public static BsonArray extractErrorLabelsAsBson(final BsonDocument response) { |
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.
Optional: move this down near the other asBson
-suffixed private methods. It caused some cognitive dissonance during review to see it here.
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.
Unlike those methods, this one is public
, and I organized the methods such that private
ones are ordered after the public
methods. I reordered the methods.
JAVA-4843
* <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 { |
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 @Nested
annotation 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 Exceptions
class that must be shared with MongoCommandException
or potentially with other added classes in the future, thus warranting the coupling?
driver-core/src/test/unit/com/mongodb/internal/ExceptionsTest.java
Outdated
Show resolved
Hide resolved
JAVA-4843
JAVA-4843