DefaultPropertyComponentBuilder generates invalid "Property" Component

[JWT007]

Log4j 2.24.3
.----
The DefaultPropertyComponentBuilder does not generate a valid Property Component.

A Log4j XML configuration property should look like this:

<Property name="p1" value="foobar"/>

However, if the ConfigurationBuilder.newProperty("p1", "foobar") is called, it generates a ComponentBuilder equivalent to this.

<Property name="p1">foobar</Property>

This is because in the PropertyComponentBuilder constructor here:

public DefaultPropertyComponentBuilder(final DefaultConfigurationBuilder<? extends Configuration> builder, final String name, final String value) {
   super(builder, name, "Property", value);
 }

... the value gets passed to the super method as the element content value and not as the value attribute.

---

Correct would probably be:

public DefaultPropertyComponentBuilder(final DefaultConfigurationBuilder<? extends Configuration> builder, final String name, final String value) {
   super(builder, name, "Property", null);
   if (value != null) {
     this.addAttribute("value", value);
   }
 }

[JWT007]

@ppkarwasz can you assign to me and I will try and get a PR ready on the weekend

[ppkarwasz]

@JWT007,

The two configurations below are equivalent:

<Property name="p1" value="foobar"/>
<Property name="p1">foobar</Property>

I don't believe there is anything wrong in the way PropertyComponentBuilder currently work. Are you experiencing some problems, caused by this?

[JWT007]

@ppkarwasz - ahh I see now that the @PluginValue annotation (PluginValueVisitor) first checks the value and then the attribute.

I was looking at the code and compared it to the plugin documentatiion.

Its probably OK then - was just expecting the PropertyComponentBuilder to generate as documented.

[JWT007]

@ppkarwasz by the way, KeyValuePairComponentBuilder handles it differently.

public DefaultKeyValuePairComponentBuilder(
          final DefaultConfigurationBuilder<? extends Configuration> builder, final String key, final String value) {
      super(builder, "KeyValuePair");
      addAttribute("key", key);
      addAttribute("value", value);
  }

[ppkarwasz]

Its probably OK then - was just expecting the PropertyComponentBuilder to generate as documented.

Since XML elements can have attributes, nested elements and text content, the XML configuration format offers some flexibility on how to specify configuration attributes. In our manual we try to use a "canonical" form, but it is not always consistent.

Two Log4j plugins can have text content: <Property> and <Script>. For the first, we use an attribute in the examples:

<Property name="foo" value="bar"/>

but for the second we use a CDATA node:

<Script name="EVENT_LOGGER_FILTER" language="groovy"><![CDATA[
    return logEvent.getMarker() != null && logEvent.getMarker().isInstanceOf("FLOW");
]]></Script>

However, this is a personal style choice, I am not sure if we should align DefaultConfigurationBuilder to it or even if there is a consensus on the style. I am +0 on such a change.