2791 Fix handling of `onMatch` and `onMismatch` attributes in the properties configuration format #3505

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.

...src/main/java/org/apache/logging/log4j/core/config/builder/impl/DefaultComponentBuilder.java

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

@Version

+ 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

...t/java/org/apache/logging/log4j/core/appender/rolling/RollingFileAppenderUpdateDataTest.java

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


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

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

log4j-core/src/main/java/org/apache/logging/log4j/core/config/builder/api/ComponentBuilder.java

     */
-    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

.../main/java/org/apache/logging/log4j/core/config/builder/api/AppenderRefComponentBuilder.java

+     * @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

log4j-core/src/main/java/org/apache/logging/log4j/core/config/builder/api/Component.java


 /**
 * Container for building Configurations. This class is not normally directly manipulated by users
 * 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

log4j-core/src/main/java/org/apache/logging/log4j/core/config/builder/api/Component.java

+     * @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

...src/main/java/org/apache/logging/log4j/core/config/builder/impl/DefaultComponentBuilder.java

    @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

...src/main/java/org/apache/logging/log4j/core/config/builder/impl/DefaultComponentBuilder.java

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


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

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

...src/main/java/org/apache/logging/log4j/core/config/builder/impl/DefaultComponentBuilder.java

+     * 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

...ain/java/org/apache/logging/log4j/core/config/properties/PropertiesConfigurationBuilder.java

@@ -334,7 +332,7 @@ private static <B extends ComponentBuilder<B>> ComponentBuilder<B> createCompone
            final ComponentBuilder<?> parent, final String key, final Properties properties) {
        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

src/changelog/.2.x.x/2791_rework_ComponentBuilder_API.xml

+  <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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2791 Fix handling of `onMatch` and `onMismatch` attributes in the properties configuration format #3505

2791 Fix handling of `onMatch` and `onMismatch` attributes in the properties configuration format #3505

JWT007 commented Feb 27, 2025 •

edited

Loading

github-actions bot commented Feb 27, 2025 •

edited

Loading

ppkarwasz left a comment

ppkarwasz left a comment

ppkarwasz Mar 2, 2025

ppkarwasz Mar 2, 2025

ppkarwasz Mar 2, 2025

ppkarwasz Mar 2, 2025

ppkarwasz Mar 2, 2025

ppkarwasz Mar 3, 2025

ppkarwasz Mar 3, 2025

ppkarwasz Mar 3, 2025

ppkarwasz Mar 3, 2025

ppkarwasz Mar 3, 2025

		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.

2791 Fix handling of onMatch and onMismatch attributes in the properties configuration format #3505

Are you sure you want to change the base?

2791 Fix handling of onMatch and onMismatch attributes in the properties configuration format #3505

Conversation

JWT007 commented Feb 27, 2025 • edited Loading

github-actions bot commented Feb 27, 2025 • edited Loading

Generated by gradle/develocity-actions

ppkarwasz left a comment

Choose a reason for hiding this comment

ppkarwasz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2791 Fix handling of `onMatch` and `onMismatch` attributes in the properties configuration format #3505

2791 Fix handling of `onMatch` and `onMismatch` attributes in the properties configuration format #3505

JWT007 commented Feb 27, 2025 •

edited

Loading

github-actions bot commented Feb 27, 2025 •

edited

Loading