2791 Fix handling of `onMatch` and `onMismatch` attributes in the properties configuration format by JWT007 · Pull Request #3505 · apache/logging-log4j2

JWT007 · 2025-02-27T19:19:25Z

#2791

Updated DefaultComponentBuilder put(String, String) to remove attributte if value is null
Updated DefaultComponentBuilder guarded putAttribute(...) implementations with object values against NPE
updated javadoc
added changelog

(edit)

per @ppkarwasz review added JVerify annotations to config.builder and config.builder.api packages
optimized builder APIs and Default***ComponentBuilder implementations
changed (deprecated) 'addAttribute' methods to 'setAttribute' to better match other builder APIs
added convencience APIs for setting general attributes on ***ComponentBuilder interfaces
javadoc'd both packages 100%
updated callers of now deprecated APIs
updated changelog

github-actions · 2025-02-27T19:23:44Z

Job	Requested goals	Build Tool Version	Build Outcome
build-macos-latest	clean install	3.9.8	✅
build-ubuntu-latest	clean install	3.9.8	✅
build-windows-latest	clean install	3.9.8	✅

Generated by gradle/develocity-actions

ppkarwasz

@JWT007,

Thanks! 💯

I think we could profit from the occasion and add JSpecify annotation to all classes in this package. As far as I can tell only a few parameters and fields can be null, mostly those named name and value.

…Attributte (apache#2791) + more appropriate method name

@NullMarked

+ made all Default***Builder implementations @ProviderrType + added setXXXAttribute method to some component builder interfaces with default implementations + added JSpecify @NullMarked to packages and @nullable where approrpiate to fields/mehods (per last PR review) + deprecated ComponentBuilder#addAttribute(...) methods and replaced with ComponentBuilder#setAttribute(...) methods in line with other builders (i.e. Plugins) - also since they don't just add but also remove if the value is null + reorganized ConfigurationBuilder to group getters, setters, builder factory methods + incremented package-info.java @Version annoations + added some null-guards in BuiltConfiguration#setup() + moved DeffaultConfigurationBuilder XML generation code from top to bottom of source file + removed "extra" attributes in Default***Builder constructors and replaced with setXXXAttribute chained calls in DefaultConfigurrationBuilder#newXXX(...) convenience methods + javadoc'd both packages (fixing documentation errors)

…ache#2791)

…instead of void (apache#2791) + added @BaselineIgnore due to API change (but breaks no code)

ppkarwasz

Thanks for the PR! 💯

Overall it looks OK, I added some notes below:

ppkarwasz · 2025-03-02T21:42:20Z

-        try (final LoggerContext loggerContext1 =
+        try (final LoggerContext tLoggerContext1 =


This isn't really necessary (and the 3 changes below).

ppkarwasz · 2025-03-02T21:48:08Z

+     * @throws NullPointerException if the given {@code key} is {@code null}
     */
-    T addAttribute(String key, Level level);
+    T setAttribute(String key, @Nullable Level level);


We can probably provide a default implementation for these methods.

ppkarwasz · 2025-03-02T21:51:03Z

+     * @param level the level
+     * @return this builder (for chaining)
+     */
+    default AppenderRefComponentBuilder setLevelAttribute(@Nullable String level) {


IMHO setLevel would be a better naming choice: it is not ambiguous and it is shorter.

Should we remove the Attribute suffix from all new methods?

ppkarwasz · 2025-03-02T21:54:49Z

 * of the Assembler API.
 * @since 2.4
 */
+@ProviderType


Suggested change

@ProviderType

This class is not meant to be extended by neither consumers nor providers, so no annotation is needed.

ppkarwasz · 2025-03-02T21:57:08Z

+     * @param newValue the new value
+     * @return the previous value or {@code null} if none was set
+     */
+    protected @Nullable String putAttribute(final String key, final @Nullable String newValue) {


Why is this protected?

It has a better sounding name than addAttribute. Maybe addAttribute should be deprecated (possibly with an Error Prone @InlineMe annotation) and this should be public?

ppkarwasz · 2025-03-03T10:02:48Z

    @Override
-    public T addAttribute(final String key, final int value) {
-        return put(key, Integer.toString(value));
+    public @NonNull CB getBuilder() {


Suggested change

public @NonNull CB getBuilder() {

public CB getBuilder() {

Isn't @NonNull the default?

ppkarwasz · 2025-03-03T10:09:27Z

+        synchronized (this) {
+            attributes.clear();
+            components.clear();
+        }


Since the class is not thread-safe anyway, do we need this?

ppkarwasz · 2025-03-03T10:11:04Z

+     * Gets the list of child components.
+     * @return an <i>immutable</i> list of the child components
+     */
+    protected List<Component> getComponents() {


Suggested change

protected List<Component> getComponents() {

protected List<? extends Component> getComponents() {

ppkarwasz · 2025-03-03T10:19:52Z

        final String name = (String) properties.remove(CONFIG_NAME);
        final String type = (String) properties.remove(CONFIG_TYPE);
-        if (Strings.isEmpty(type)) {
+        if (type == null || Strings.isEmpty(type)) {


Suggested change

if (type == null || Strings.isEmpty(type)) {

if (Strings.isEmpty(type)) {

Strings.isEmpty is null-safe.

ppkarwasz · 2025-03-03T10:20:43Z

+  <issue id="2791" link="https://github.com/apache/logging-log4j2/issues/2791"/>
+  <description format="asciidoc">
+    Fix problem when null attribute values are set on DefaultComponentBuilder. GitHub issue #2791.
+    Added JVerify annotations to config.builder.* packages.


Suggested change

Added JVerify annotations to config.builder.* packages.

Added JSpecify annotations to config.builder.* packages.

vy · 2026-04-28T21:29:59Z

@ramanathan1504, is this something you might want to pick up?

ramanathan1504 · 2026-04-29T05:37:43Z

@vy, Yes, I can pick this up. I will review this PR and the issue discussion and come up with updated one.

vy · 2026-05-28T07:35:12Z

Superseded by #4116.

Fix potential NPEs in DefaultComponentBuilder in addAttribute methods (…

322cc1d

…apache#2791)

JWT007 requested a review from ppkarwasz February 27, 2025 19:19

ppkarwasz requested changes Feb 27, 2025

View reviewed changes

JWT007 added 7 commits February 27, 2025 23:26

Added JVerify annotations per review (apache#2791)

3aa1ac2

Added DefaultComponentBuilderTest (apache#2791)

406b097

Replaced DefaultComponentBuilder#put with DefaultComponentBuilder#put…

5bc9762

…Attributte (apache#2791) + more appropriate method name

Update references to deprecations with new component builder APIs (ap…

fcc5d3e

…ache#2791)

Changed ConfigurationBuilder#setLoggerContext(...) to return builder …

8415abf

…instead of void (apache#2791) + added @BaselineIgnore due to API change (but breaks no code)

Adapt changelog entry for additional changes (apache#2791)

d7526eb

ppkarwasz requested changes Mar 3, 2025

View reviewed changes

ppkarwasz added this to Log4j bug tracker Apr 3, 2025

github-project-automation Bot moved this to To triage in Log4j bug tracker Apr 3, 2025

ppkarwasz added this to the 2.25.0 milestone Apr 13, 2025

ppkarwasz assigned JWT007 May 16, 2025

ppkarwasz modified the milestones: 2.25.0, 2.25.1 May 25, 2025

ppkarwasz removed this from the 2.25.1 milestone Jun 21, 2025

vy assigned ramanathan1504 Apr 29, 2026

ramanathan1504 mentioned this pull request May 6, 2026

Fix handling of onmatch and onmismatch #4116

Open

vy closed this May 28, 2026

github-project-automation Bot moved this from To triage to Done in Log4j bug tracker May 28, 2026

		try (final LoggerContext loggerContext1 =
		try (final LoggerContext tLoggerContext1 =

	protected List<Component> getComponents() {
	protected List<? extends Component> getComponents() {

	if (type == null \|\| Strings.isEmpty(type)) {
	if (Strings.isEmpty(type)) {

	Added JVerify annotations to config.builder.* packages.
	Added JSpecify annotations to config.builder.* packages.

Uh oh!

Conversation

JWT007 commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Generated by gradle/develocity-actions

Uh oh!

ppkarwasz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ppkarwasz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vy commented Apr 28, 2026

Uh oh!

ramanathan1504 commented Apr 29, 2026

Uh oh!

vy commented May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JWT007 commented Feb 27, 2025 •

edited

Loading

github-actions Bot commented Feb 27, 2025 •

edited

Loading