Skip to content
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.

Sign up for GitHub

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

Jump to bottom

AsyncWaitStrategyFactoryConfig - potential NPE when creating programatically #3159

Open
JWT007 opened this issue Nov 4, 2024 · 4 comments · May be fixed by #3462
Open

AsyncWaitStrategyFactoryConfig - potential NPE when creating programatically #3159

JWT007 opened this issue Nov 4, 2024 · 4 comments · May be fixed by #3462

Comments

@JWT007
Copy link
Contributor

JWT007 commented Nov 4, 2024 •
edited
Loading

AsyncWaitStrategyFactoryConfig (Log4j 2.24.1)

This @Plugin doesn't behave like most others where if the configuration is invalid, it will return a null from the Builder.

The @PluginBuilderAttribute factoryClassName (for the XML class attribute) is marked as @Required, but if creating programatically the missing value is not checked and the constructor will throw a NPE.

@Override
public AsyncWaitStrategyFactoryConfig build() {
    return new AsyncWaitStrategyFactoryConfig(factoryClassName);
}

public AsyncWaitStrategyFactoryConfig(final String factoryClassName) {
    this.factoryClassName = Objects.requireNonNull(factoryClassName, "factoryClassName");
}

Typical behaviour here would be to return null from the build() method if misconfigured.

  • adding a NotEmpty check to the Builder#withFactoryClassName(...) method might be sensible.
  • adding a NotEmpty check to the constructor might be sensible (an empty string here makes no sense)
@JWT007 JWT007 mentioned this issue Nov 4, 2024
Closed
@JWT007
Copy link
Contributor Author

JWT007 commented Nov 9, 2024

@ppkarwasz I am not sure why the bot didn't mark this as "waiting for maintainer" - not sure if it pops up for you?

@ppkarwasz
Copy link
Contributor

I am seriously considering deprecating AsyncWaitStrategyFactoryConfig. I will discuss it with the other maintainers (especially @remkop, who introduced it), but it could probably be replaced with the log4j2.asyncLoggerConfigWaitStrategy Log4j configuration property.

The presence of both a Log4j configuration property and a direct child of <Configuration> IMHO needlessly complexifies the logic to determine which waiting strategy to use. In main we have a simple DI system and the WaitStrategy beans are the only ones that need a Configuration parameter.

@ppkarwasz
Copy link
Contributor

I am not sure why the bot didn't mark this as "waiting for maintainer" - not sure if it pops up for you?

You are a CONTRIBUTOR now! 😉
The labels are pretty much a work in progress and "waiting for maintainer" is mostly there to make a triage and ask the user some basic questions. Your issues usually don't need more information, so you can consider them accepted by default.

JWT007 pushed a commit to JWT007/logging-log4j2 that referenced this issue Feb 12, 2025
@jethomas-tsi
Improve validation for AsyncWaitStrategyFactoryConfig for null/empty …
9dfb111 
…'factoryClassName' (apache#3159)
JWT007 pushed a commit to JWT007/logging-log4j2 that referenced this issue Feb 12, 2025
@jethomas-tsi
Moved and updated 3159 changelog entry. (apache#3159)
1fd5495
@JWT007 JWT007 linked a pull request Feb 12, 2025 that will close this issue
Open
@JWT007
Copy link
Contributor Author

JWT007 commented Feb 12, 2025

@ppkarwasz - recreated the PR based on current code base

@ppkarwasz ppkarwasz linked a pull request Feb 16, 2025 that will close this issue
Improve validation for AsyncWaitStrategyFactoryConfig for null/empty 'factoryClassName'. (#3159) #3462
Open
JWT007 pushed a commit to JWT007/logging-log4j2 that referenced this issue Feb 16, 2025
@jethomas-tsi
Moved changelog to .2.x.x per other PR Code-Review (apache#3159)
7ad6736
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@ppkarwasz @JWT007