ppkarwasz commented on code in PR #3322:
URL: https://github.com/apache/logging-log4j2/pull/3322#discussion_r1897233536
##########
log4j-mongodb/src/main/java/org/apache/logging/log4j/mongodb/MongoDbProvider.java:
##########
@@ -94,11 +110,18 @@ public static <B extends Builder<B>> B newBuilder() {
private final Long collectionSize;
private final boolean isCapped;
+ private final String collectionName;
+ private final String databaseName;
private final MongoClient mongoClient;
private final MongoDatabase mongoDatabase;
private final ConnectionString connectionString;
- private MongoDbProvider(final String connectionStringSource, final boolean
isCapped, final Long collectionSize) {
+ private MongoDbProvider(
+ final String connectionStringSource,
+ final boolean isCapped,
+ final Long collectionSize,
+ final String databaseName,
+ final String collectionName) {
LOGGER.debug("Creating ConnectionString {}...",
connectionStringSource);
this.connectionString = new ConnectionString(connectionStringSource);
Review Comment:
Since `new ConnectionString()` can throw, I would move this call to
`Builder.build()`, e.g.:
```java
ConnectionString connectionString;
try {
connectionString = new ConnectionString(connectionStringSource);
} catch (final IllegalArgumentException e) {
LOGGER.error("Invalid MongoDB connection string `{}`.",
connectionStringSource, e);
return null;
}
```
##########
log4j-mongodb/src/main/java/org/apache/logging/log4j/mongodb/MongoDbProvider.java:
##########
@@ -54,9 +54,15 @@ public static class Builder<B extends Builder<B>> extends
AbstractFilterable.Bui
@PluginAttribute("capped")
private boolean capped = false;
+ @PluginAttribute("collectionName")
+ private String collectionName = null;
+
+ @PluginAttribute("datbaseName")
Review Comment:
```suggestion
@PluginAttribute("databaseName")
```
##########
log4j-mongodb/src/test/java/org/apache/logging/log4j/mongodb/MongoDbCollectionNameIT.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you 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 org.apache.logging.log4j.mongodb;
+
+import com.mongodb.client.MongoClient;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.test.junit.LoggerContextSource;
+import org.apache.logging.log4j.test.junit.UsingStatusListener;
+import org.junit.jupiter.api.Test;
+
+@UsingMongoDb
+@LoggerContextSource("MongoDbCollectionNameIT.xml")
+// Print debug status logger output upon failure
+@UsingStatusListener
+class MongoDbCollectionNameIT extends AbstractMongoDbCappedIT {
Review Comment:
I am not sure if we need more integration tests. We probably just need a
**unit** test for `MongoDbProvider.Builder` that verifies at least that:
1. If `databaseName` is provided, it overrides the value in the connection
string.
2. If `databaseName` is **not** provided, the value from the connection
string is used.
3. If `collectionName` is provided, it overrides the value in the connection
string.
4. If `collectionName` is **not** provided, the value from the connection
string is used.
##########
log4j-mongodb/src/main/java/org/apache/logging/log4j/mongodb/MongoDbProvider.java:
##########
@@ -113,10 +136,19 @@ private MongoDbProvider(final String
connectionStringSource, final boolean isCap
LOGGER.debug("Creating MongoClient {}...", settings);
this.mongoClient = MongoClients.create(settings);
LOGGER.debug("Created MongoClient {}", mongoClient);
- final String databaseName = this.connectionString.getDatabase();
- LOGGER.debug("Getting MongoDatabase {}...", databaseName);
- this.mongoDatabase = this.mongoClient.getDatabase(databaseName);
+ if (databaseName == null || databaseName.isEmpty()) {
+ this.databaseName = this.connectionString.getDatabase();
+ } else {
+ this.databaseName = databaseName;
+ }
+ LOGGER.debug("Getting MongoDatabase {}...", this.databaseName);
+ this.mongoDatabase = this.mongoClient.getDatabase(this.databaseName);
LOGGER.debug("Got MongoDatabase {}", mongoDatabase);
+ if (collectionName == null || collectionName.isEmpty()) {
+ this.collectionName = this.connectionString.getCollection();
+ } else {
+ this.collectionName = collectionName;
+ }
Review Comment:
Same as above.
##########
log4j-mongodb/src/main/java/org/apache/logging/log4j/mongodb/MongoDbProvider.java:
##########
@@ -113,10 +136,19 @@ private MongoDbProvider(final String
connectionStringSource, final boolean isCap
LOGGER.debug("Creating MongoClient {}...", settings);
this.mongoClient = MongoClients.create(settings);
LOGGER.debug("Created MongoClient {}", mongoClient);
- final String databaseName = this.connectionString.getDatabase();
- LOGGER.debug("Getting MongoDatabase {}...", databaseName);
- this.mongoDatabase = this.mongoClient.getDatabase(databaseName);
+ if (databaseName == null || databaseName.isEmpty()) {
+ this.databaseName = this.connectionString.getDatabase();
+ } else {
+ this.databaseName = databaseName;
+ }
Review Comment:
I would move this logic to `Builder.build()` too and validate the name of
the database, e.g.:
```java
String effectiveDatabaseName = databaseName != null ? databaseName :
connectionString.getDatabase();
try {
MongoNamespace.checkDatabaseNameValidity(effectiveDatabaseName);
} catch (final IllegalArgumentException e) {
LOGGER.error("Invalid MongoDB database name `{}`.",
effectiveDatabaseName, e);
return null;
}
```
--
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
