vy commented on code in PR #3467:
URL: https://github.com/apache/logging-log4j2/pull/3467#discussion_r1959659783
##########
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:
> The constructor can only throw if the parameters are invalid
@ppkarwasz, right, this is a scheme practiced by several other Log4j
components: ctor fails on invalid arguments with an explanatory message – this
is not a problem.
> then each call to the constructor must be in a `try...catch`
Assuming unchecked exceptions are used, this is not needed, since
`PluginBuilder::build` already does this:
https://github.com/apache/logging-log4j2/blob/8c0e3c6c4f32ba97985efc05286e5425bfe36742/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.java#L126
Moving these checks to `MongoDb4Provider::new` will provide complete
coverage independent of the call site.
> When I made these changes to the main branch, was specifically asked to
make them to the build method. So making these change in this format keep the
approach in line with the same code changes in the main branch. It seems odd to
want to solve the same challenge in 2 different ways across the branches.
@jesmith17, apologies for the inconvenience. PRs, should target `2.x`, not
`main`, unless they target to-be-released Log4j 3. This is explicitly stated in
the PR template:
https://github.com/apache/logging-log4j2/blob/8c0e3c6c4f32ba97985efc05286e5425bfe36742/.github/pull_request_template.md#L5
> It seems odd to want to solve the same challenge in 2 different ways
across the branches.
You can also throw from a component ctor in `main` too, no need to deviate
from the scheme that we're implementing in `2.x`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
