Make AbstractJsonPatternParser throw an exception for invalid pattern instead of logging an ERROR status

[brenuart]

AbstractJsonPatternParser now throws a JsonPatternException in case of invalid pattern. It is up to the AbstractPatternLayoutJsonProvider to catch the exception, log an error status and decide how it should behave.

Invalid PatternLayout formats are now detected and throw a JsonPatternException as well. Detection is done by inspecting the parsed output returned by the PatternLayout when it is started, looking for the special %PARSER_ERROR marker inserted when it encounters an error.

The previous implementation used to ignore fields with an invalid pattern layout while keeping the other. This results in a partial JSON output with only the valid fields. This behaviour is now changed and the provider reverts to producing nothing in case of bad configuration.

These changes address issue #686.

[philsttr]

I'm a little worried about the Context interface evolving with new default methods, and us having to keep up with it. (same for DelegatingStatusManager) Can you think of any other option that does not require these delegating objects

[brenuart]

Errors detected by the PatternLayout fall in two categories:

• Syntax Error. Happens when the parser fails to parse the pattern for reasons like a missing closing bracket. In this case, the PatternLayoutBase logs an ERROR status and returns an empty string when its doLayout(event) method is invoked afterwards. Technically, the converter chain is empty (the head is null).

• Conversion Error. Happens when an unknown conversion word is used (%name) or when it fails to instantiate an instance of the corresponding converter. In this case, instead of throwing an exception, the Parser replaces the conversion word with the string %PARSER_ERROR[name].
  Suppose you want to refer to the logger level and use %loggerLevel instead of %level, then the PatternLayout will produce "%PARSE_ERROR[loggerLevel]" at runtime. From a technical point of view, the conversion chain contains a single LiteralConverter with a constant value.

We could rely on this behaviour and inspect the conversion chain looking for LiteralConverters with %PARSE_ERROR[...] in them. This strategy has the following limitations:

• syntax errors are not detected. We can deduce a syntax error occurred via an other mechanism but we won't be able to capture the actual cause - only the fact a syntax error occurred;
• we know a conversion error occurred but here again we don't have the cause (the cause is logged in the error status);

Having said that, this may not be a problem: the PatternLayoutBase will have logged the actual cause itself before we detect an error occurred. The StatusManager would report something like this:

11:15:32,589 |-ERROR in ch.qos.logback.core.pattern.parser.Compiler@4944252c - There is no conversion class registered for conversion word [loggerLevel]
11:15:32,590 |-ERROR in ch.qos.logback.core.pattern.parser.Compiler@4944252c - [loggerLevel] is not a valid conversion word
11:15:32,591 |-ERROR in net.logstash.logback.composite.loggingevent.LoggingEventPatternJsonProvider@2781e022 - Invalid [pattern]: Invalid JSON property '/level' (was '%loggerLevel'): failed to interpret '%loggerLevel' (see previous statuses for more information)

The current solution emits a single ERROR status with all the details:

11:35:36,541 |-ERROR in net.logstash.logback.composite.loggingevent.LoggingEventPatternJsonProvider@2781e022 - Invalid [pattern]: Invalid JSON property '/level' (was '%loggerLevel'): There is no conversion class registered for conversion word [loggerLevel]

Both approaches can do the job.
I preferred to capture errors by hooking into StatusManager#addError() instead of parsing strings looking for error patterns. This looked more reliable to me... This must be balanced against the maintenance overhead you mentionned.

---

I implemented a quick and dirty alternative based on the %PARSER_ERROR approach in the branch gh686-patternlayout-errorhandling-alternative.
See https://github.com/logstash/logstash-logback-encoder/blob/gh686-patternlayout-errorhandling-alternative/src/main/java/net/logstash/logback/pattern/PatternLayoutAdapter.java to have an idea of what it looks like...

@philsttr Tell me what you think... (I think I already know your choice 😂)

[philsttr]

As far as implementation... Both approaches are quiet "ugly" (no offense to your code, it's just what we're forced to do given logback's behavior). But I think the approach from gh686-patternlayout-errorhandling-alternative is more maintainable.

I think in either case, the user will easily realize something is wrong.

So, I'm leaning toward the gh686-patternlayout-errorhandling-alternative impl

[brenuart]

Ok. Sold.