Breaking change in logback-core - breaks Logstash Logback Encoder

[jordanjennings]

As noted in this comment here:

#908 (comment)

When upgrading to Spring Boot 3.4.2, we got the following error

java.lang.IllegalStateException: java.lang.IllegalStateException: Logback configuration error detected: 
ERROR in net.logstash.logback.composite.loggingevent.LoggingEventPatternJsonProvider@120bb5b5 - Invalid [pattern]: Invalid JSON property '//version' (was '%replace(%property{build.version}){'null',''}'): class java.lang.String cannot be cast to class java.util.function.Supplier (java.lang.String and java.util.function.Supplier are in module java.base of loader 'bootstrap') net.logstash.logback.pattern.AbstractJsonPatternParser$JsonPatternException: Invalid JSON property '//version' (was '%replace(%property{build.version}){'null',''}'): class java.lang.String cannot be cast to class java.util.function.Supplier (java.lang.String and java.util.function.Supplier are in module java.base of loader 'bootstrap')

And our configuration looks like

provider {
isOmitEmptyFields = true
this.pattern = """
{
"type": "Service",
"name": "%mdc{component}",
"version": "%replace(%property{build.version}){'null',''}"
}
"""
}

We are also observing that on upgrade to Spring Boot 3.4.2+ or 3.3.8+, Logstash Logback Encoder no longer works correctly for %property{some.system.property} patterns.

This seems to be due to a breaking change introduced by logback-core in version 1.5.13 and after.

Here's the line where it's breaking:

https://github.com/qos-ch/logback/blob/master/logback-core/src/main/java/ch/qos/logback/core/pattern/parser/Compiler.java#L100

Supplier<DynamicConverter> supplier = converterMap.get(keyword);

The root of the issue appears to be that the legacy conversion added within logback-core does not apply to the instanceConverterMap which logback-logstash-encoder uses to register its EnhancedPropertyConverter class.

See here: https://github.com/qos-ch/logback/blob/master/logback-core/src/main/java/ch/qos/logback/core/pattern/PatternLayoutBase.java#L97C9-L97C51

    public Map<String, Supplier<DynamicConverter>> getEffectiveConverterMap() {
        Map<String, Supplier<DynamicConverter>> effectiveMap = new HashMap<>();

        // ... removed for brevity ...

        caterForLegacyConverterMaps(effectiveMap);

        // ... removed for brevity ...

        // set the most specific map last
        effectiveMap.putAll(instanceConverterMap);
        return effectiveMap;
    }

Note that the caterForLegacyConverterMaps(effectiveMap) method is called before the instanceConverterMap is added to the map.

And see here for how logstash-logback-encoder is adding its value to instanceConverterMap:

https://github.com/logfellow/logstash-logback-encoder/blob/main/src/main/java/net/logstash/logback/pattern/LoggingEventJsonPatternParser.java#L41

    @Override
    protected PatternLayoutBase<ILoggingEvent> createLayout() {
        PatternLayoutBase<ILoggingEvent> layout = new PatternLayout();
        layout.getInstanceConverterMap().put("property", EnhancedPropertyConverter.class.getName());
        return layout;
    }

It's adding the class name as a String, which is the legacy way to do it, and nothing in logback-core is fixing that.

This means that when instanceConverterMap is added to effectiveMap, the value is a raw String and not a Supplier, hence the error shown by @jackie-linz above: class java.lang.String cannot be cast to class java.util.function.Supplier

Assuming I've understood the problem correctly, it seems like logback-core needs to release a fix to handle this case since a breaking change was introduced in a patch version. Although it could be fixed in logstash-logback-encoder as well, that would mean that logstash-logback-encoder would have to make a breaking change as well to fix the breaking change introduced by logback-core and that seems less ideal, and it would mean that logstash-logback-encoder would have to maintain parallel versions, one that works with the older logback-core and one that works with the newer logback-core.

[ceki]

@jordanjennings Thank you for this report.

There was significant effort made to make the "String" to "Supplier" change transparent to derived classes. However, LoggingEventJsonPatternParser modifies an existing conversion word via the getInstanceConverterMap(). Unfortunately, this use case was not taken into account and I think it is too late to fix now.

My suggestion would be to create at new issue in the logstash-logback-encoder repository asking them to upgrade to logback 1.5.13 or later. I am quite sorry but I don't see a way around this.

[jordanjennings]

@ceki Thank you for the fast response!

Is there a recommended approach that comes to mind that will be backwards and forwards compatible that logstash-logback-encoder could use to support both <=1.5.12 and >=1.5.13 by leveraging some of the internal support for the migration?

The way I'm working around it in the short term is to use a custom provider that uses reflection internally to check whether the Map uses String or Supplier so that I can support both versions with a single version of our internal logging library, but it's very brittle to changes in logback-core so I'd rather do something better and I wouldn't think logstash-logback-encoder should take this approach.

Is there a better way?

[ceki]

@jordanjennings As mentioned in my previous message, LoggingEventJsonPatternParser modifies an instance of PatternLayout by changing the mapping of a existing conversion word via the getInstanceConverterMap() method. This usage path was not taken into account and it is too late to do so now.

From a cursory cost benefit analysis, given that the change is trivial when done in logstash-logback-encoder, it should be done there.

Here is the change. Line 41 in LoggingEventJsonPatternParser should be changed

from
layout.getInstanceConverterMap().put("property", EnhancedPropertyConverter.class.getName());

to
layout.getInstanceConverterMap().put("property", EnhancedPropertyConverter::new);

and pom.xml should update logback-core version from 1.5.6 to 1.5.18 or later. The logback-access version should be updated to 2.0.6. I suggest you open a bug report in logstash-logback-encoder repository.

[jordanjennings]

Thanks again @ceki for fast and detailed responses. I'll open an issue over in logstash-logback-encoder and will continue discussions there.