Skip to content

Honor ConfigurationFactory active state - #4199

Open
DragonFSKY wants to merge 1 commit into
apache:2.xfrom
DragonFSKY:fix-3896-configuration-factory-active
Open

Honor ConfigurationFactory active state#4199
DragonFSKY wants to merge 1 commit into
apache:2.xfrom
DragonFSKY:fix-3896-configuration-factory-active

Conversation

@DragonFSKY

@DragonFSKY DragonFSKY commented Jul 23, 2026

Copy link
Copy Markdown

Fixes #3896.

This change makes ConfigurationFactory active-state handling consistent:

  • Factories expose their supported types regardless of active state.
  • Inactive factories return null, and aggregate lookup does not invoke them to create configurations.
  • Default resource discovery preserves the existing file-specific diagnostic for matching inactive factories and continues fallback lookup.
  • JSON and YAML factories honor overridden isActive() implementations, while Log4j 1.x compatibility factories track the compatibility property dynamically.

Tests cover aggregate lookup paths, inactive-factory diagnostics and fallback, dynamic activation, overridden active state, and missing JSON/YAML dependencies. Existing Log4j 1.x configuration tests explicitly enable compatibility mode where required, and the affected OSGi package versions reflect the protected overrides.

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw -pl log4j-core-test,log4j-1.2-api -am verify succeeds (the build instructions)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests are provided

@ramanathan1504

Copy link
Copy Markdown
Contributor

@DragonFSKY I've just taken a quick look at your PR and noticed there are multiple new test cases. Since it touches multiple files, please give us a little time. We will review it ASAP!

@ramanathan1504 ramanathan1504 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result: with jackson-dataformat-yaml absent, getSupportedTypes() still returns the YAML suffixes, so today a log4j2.yaml match logs that ERROR naming the file and the likely cause. After this change the factory is skipped before that point and nothing is logged. Please log before the continue.


final ConfigurationSource source = ConfigurationSource.fromResource(configName, loader);
if (source != null) {
if (!factory.isActive()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Found configuration file {} for the inactive {} ... might be inactive due to a missing dependency" ERROR from ConfigurationFactory. That line is commit
5d47e93 by @vy, which raised it from WARN to ERROR and added the missing-dependency wording. It only became unreachable because inactive factories are now skipped at the top
of the loop.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing the changes again, I noticed that my latest approach weakens the contract requested in #3896: an inactive factory is expected to reject getSupportedTypes(). The existing file-specific diagnostic can only be produced after querying the factory’s supported suffixes and finding a matching resource. Logging before the early continue would instead produce a factory-level message even when no matching configuration file exists. I have restored the strict inactive behavior for now. Could you please confirm whether this is the intended trade-off, or whether you would prefer another way to retain the file-specific diagnostic?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU, what without this fix, when someone has a project with log4j.json, but without Jackson in the classpath, they were getting an ERROR-level log message stating that log4j.json could not be loaded due to a missing dependency. With this change, log4j.json will not be loaded and user will have no indication of this happened and why. I think this is not desirable from a user's pov. Shall we change the approach to the following:

  • Correctly implement isActive() everywhere
  • Make getSupportedTypes() work, always, regardless of the isActive() state
  • Make getConfiguration() return null if inactive — this matches the existing behavior in CF
  • Use isActive correctly & wisely in CF such that, if a CF is inactive, don't use it, but still warn the user if an inactive CF matches an existing configuration file

@ramanathan1504, @DragonFSKY, WDYT?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this direction makes sense. I have updated the PR accordingly: supported types remain discoverable, inactive factories return null, and aggregate lookup does not invoke them to create configurations, while preserving the existing file-specific diagnostic and fallback behavior. Please let me know if anything else needs adjusting.

@github-project-automation github-project-automation Bot moved this to Changes requested in Log4j pull request tracker Aug 1, 2026
@DragonFSKY

Copy link
Copy Markdown
Author

Thanks for catching this! I've fixed it. Please let me know if anything else needs adjusting.

@DragonFSKY
DragonFSKY force-pushed the fix-3896-configuration-factory-active branch 3 times, most recently from d8f4a05 to 7b7c20b Compare August 1, 2026 06:58
@DragonFSKY
DragonFSKY force-pushed the fix-3896-configuration-factory-active branch from 7b7c20b to 6b8ec39 Compare August 3, 2026 02:09
@ramanathan1504

Copy link
Copy Markdown
Contributor

@DragonFSKY don't use force push.it will completely clear the old review..Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Changes requested

Development

Successfully merging this pull request may close these issues.

Fix ConfigurationFactory::isActive overrides

3 participants