vy commented on code in PR #3467:
URL: https://github.com/apache/logging-log4j2/pull/3467#discussion_r1958784560
##########
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Connection.java:
##########
@@ -119,4 +121,12 @@ public String toString() {
"Mongo4Connection [connectionString=%s, collection=%s,
mongoClient=%s]",
connectionString, collection, mongoClient);
}
+
+ /*
+ * This method is exposed to help support unit tests for the
MongoDbProvider class.
+ *
+ */
+ public MongoCollection<Document> getCollection() {
Review Comment:
Please don't add a public method (which becomes a liability for decades)
unless there is a practical runtime use for it. You can simply extract this
value for tests using reflection, it is okay.
##########
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Connection.java:
##########
@@ -67,21 +67,23 @@ public MongoDb4Connection(
final ConnectionString connectionString,
final MongoClient mongoClient,
final MongoDatabase mongoDatabase,
+ final String collectionName,
Review Comment:
This is a breaking change, try running `./mvnw verify -DskipTests -pl
:log4j-mongodb4`. Would you mind
1. Creating a new _canonical_ ctor that accepts all arguments
2. Delegate from all other ctors to the new canonical ctor using
`this(connectionString, ...)`
3. Mark all other ctors `@Deprecated`
4. Migrate places using `@Deprecated` ctors to the new canonical ctor
##########
log4j-mongodb4/src/main/java/org/apache/logging/log4j/mongodb4/MongoDb4Provider.java:
##########
@@ -60,14 +61,47 @@ public static class Builder<B extends Builder<B>> extends
AbstractFilterable.Bui
@PluginBuilderAttribute("capped")
private boolean capped = false;
+ @PluginBuilderAttribute("collectionName")
+ private String collectionName;
+
+ @PluginBuilderAttribute("databaseName")
+ private String databaseName;
+
+
+
@Override
public MongoDb4Provider build() {
StatusLogger.getLogger().warn("The {} Appender is deprecated, use
the MongoDb Appender.", PLUGIN_NAME);
+
+ ConnectionString connectionString;
+ try {
+ connectionString = new
ConnectionString(connectionStringSource);
+ } catch (final IllegalArgumentException e) {
+ LOGGER.error("Invalid MongoDB connection string `{}`.",
connectionStringSource, e);
+ return null;
+ }
+
+ String effectiveDatabaseName = databaseName != null ? databaseName
: connectionString.getDatabase();
+ String effectiveCollectionName = collectionName != null ?
collectionName : connectionString.getCollection();
+ // Validate the provided databaseName property
+ try {
+
MongoNamespace.checkDatabaseNameValidity(effectiveDatabaseName);
+ } catch (final IllegalArgumentException e) {
+ LOGGER.error("Invalid MongoDB database name `{}`.",
effectiveDatabaseName, e);
+ return null;
+ }
+ // Validate the provided collectionName property
+ try {
+
MongoNamespace.checkCollectionNameValidity(effectiveCollectionName);
+ } catch (final IllegalArgumentException e) {
+ LOGGER.error("Invalid MongoDB collection name `{}`.",
effectiveCollectionName, e);
+ return null;
+ }
Review Comment:
Shouldn't this check be better placed to `MongoDb4Provider::new`, which
receives both the connection URI and the database/collection name from
different code paths?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
