ppkarwasz commented on code in PR #3322:
URL: https://github.com/apache/logging-log4j2/pull/3322#discussion_r1899105151
##########
log4j-mongodb/pom.xml:
##########
@@ -141,7 +142,11 @@
<artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
-
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <scope>test</scope>
+ </dependency>
</dependencies>
Review Comment:
We don't use JUnit 4 any more, only JUnit 5.
##########
log4j-mongodb/src/test/java/org/apache/logging/log4j/mongodb/MongoDbProviderTest.java:
##########
@@ -0,0 +1,108 @@
+package org.apache.logging.log4j.mongodb;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
Review Comment:
Please use the JUnit 5 `org.junit.jupiter.api.Assertions` class instead.
##########
log4j-mongodb/pom.xml:
##########
@@ -76,6 +76,7 @@
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
+ <version>3.0.0-SNAPSHOT</version>
Review Comment:
```suggestion
```
There will be no `log4j-api` version `3.0.0`. The correct version of
`log4j-api` (`2.24.3`) is managed by the gran-parent of this POM file, so no
need to specify the version explicitly.
##########
src/site/antora/modules/ROOT/pages/manual/appenders/database.adoc:
##########
@@ -817,6 +817,28 @@ for its format.
**Required**
+| [[MongoDbProvider-attr-databaseName]]databaseName
+| 'string'
+|
+|
+It specifies the name of the database for the logger to use.
+
+Overrides the value provided in the connection string if present.
+
+**Required**
+
+| [[MongoDbProvider-attr-collectionName]]collectionName
+| 'string'
+|
+|
+It pecifies the name of the collection for the logger to use.
+
+Overrides the value provided in the connection string if present.
Review Comment:
```suggestion
For backward compatibility, the collection name can also be specified in the
https://mongodb.github.io/mongo-java-driver/5.0/apidocs/mongodb-driver-core/com/mongodb/ConnectionString.html[Java-specific
connection string]
```
As far as I understand the MongoDB connection string **does not contain** a
collection name. The Java driver uses an alternative syntax that can contain a
collection name, but we should encourage users to use the [official connection
string
syntax](https://www.mongodb.com/docs/manual/reference/connection-string/), not
the language-specific one.
##########
src/site/antora/modules/ROOT/pages/manual/appenders/database.adoc:
##########
@@ -817,6 +817,28 @@ for its format.
**Required**
+| [[MongoDbProvider-attr-databaseName]]databaseName
+| 'string'
+|
+|
+It specifies the name of the database for the logger to use.
Review Comment:
```suggestion
It specifies the name of the database for the appender to use.
```
##########
log4j-mongodb/src/main/java/org/apache/logging/log4j/mongodb/MongoDbProvider.java:
##########
@@ -57,12 +58,40 @@ public static class Builder<B extends Builder<B>> extends
AbstractFilterable.Bui
@PluginAttribute("collectionName")
private String collectionName = null;
- @PluginAttribute("datbaseName")
+ @PluginAttribute("databaseName")
private String databaseName = null;
@Override
public MongoDbProvider build() {
- return new MongoDbProvider(connectionStringSource, capped,
collectionSize, databaseName, collectionName);
+ LOGGER.debug("Creating ConnectionString {}...",
connectionStringSource);
+ 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);
+ databaseName = effectiveDatabaseName;
Review Comment:
_Nit_: This modifies the `databaseName` field of the builder. I would prefer
to keep the `build()` method side-effect-free (except logging of course).
##########
log4j-mongodb/src/main/java/org/apache/logging/log4j/mongodb/MongoDbProvider.java:
##########
@@ -57,12 +58,40 @@ public static class Builder<B extends Builder<B>> extends
AbstractFilterable.Bui
@PluginAttribute("collectionName")
private String collectionName = null;
- @PluginAttribute("datbaseName")
+ @PluginAttribute("databaseName")
private String databaseName = null;
@Override
public MongoDbProvider build() {
- return new MongoDbProvider(connectionStringSource, capped,
collectionSize, databaseName, collectionName);
+ LOGGER.debug("Creating ConnectionString {}...",
connectionStringSource);
+ 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);
+ databaseName = effectiveDatabaseName;
+ } catch (final IllegalArgumentException e) {
+ LOGGER.error("Invalid MongoDB database name `{}`.",
effectiveDatabaseName, e);
+ return null;
+ }
+ // Validate the provided collectionName property
+ try {
+
MongoNamespace.checkCollectionNameValidity(effectiveCollectionName);
+ collectionName = effectiveCollectionName;
Review Comment:
Same as above.
--
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
