Skip to content

Commit

Permalink
Make AbstractJsonPatternParser throw an exception for invalid pattern…
Browse files Browse the repository at this point in the history 
…s instead of logging an ERROR status

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 configuring the PatternLayout instances with a custom Logback context that throws an exception when an ERROR status is logged.

The former 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.
  • Loading branch information
@brenuart
brenuart committed Nov 3, 2021
1 parent 666d169 commit 414f482
Show file tree
Hide file tree
Showing 16 changed files with 853 additions and 532 deletions.
16 changes: 13 additions & 3 deletions src/main/java/net/logstash/logback/composite/AbstractPatternJsonProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.Objects;

import net.logstash.logback.pattern.AbstractJsonPatternParser;
import net.logstash.logback.pattern.AbstractJsonPatternParser.JsonPatternException;
import net.logstash.logback.pattern.NodeWriter;

import ch.qos.logback.access.spi.IAccessEvent;
Expand Down Expand Up @@ -79,19 +80,28 @@ public void start() {
if (jsonFactory == null) {
throw new IllegalStateException("JsonFactory has not been set");
}
initializeNodeWriter();

try {
this.nodeWriter = initializeNodeWriter();
} catch (JsonPatternException e) {
this.nodeWriter = null;
addError("Invalid [pattern]: " + e.getMessage(), e);
}

super.start();
}


/**
* Parses the pattern into a {@link NodeWriter}.
*
* @return a {@link NodeWriter}
* @throws JsonPatternException thrown in case of invalid pattern
*/
private void initializeNodeWriter() {
private NodeWriter<Event> initializeNodeWriter() throws JsonPatternException {
AbstractJsonPatternParser<Event> parser = createParser(this.jsonFactory);
parser.setOmitEmptyFields(omitEmptyFields);
this.nodeWriter = parser.parse(pattern);
return parser.parse(pattern);
}


Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/logstash/logback/composite/accessevent/AccessEventPatternJsonProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class AccessEventPatternJsonProvider extends AbstractPatternJsonProvider<

@Override
protected AbstractJsonPatternParser<IAccessEvent> createParser(JsonFactory jsonFactory) {
return new AccessEventJsonPatternParser(this, jsonFactory);
return new AccessEventJsonPatternParser(getContext(), jsonFactory);
}

}
2 changes: 1 addition & 1 deletion ...ain/java/net/logstash/logback/composite/loggingevent/LoggingEventPatternJsonProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class LoggingEventPatternJsonProvider extends AbstractPatternJsonProvider

@Override
protected AbstractJsonPatternParser<ILoggingEvent> createParser(JsonFactory jsonFactory) {
return new LoggingEventJsonPatternParser(this, jsonFactory);
return new LoggingEventJsonPatternParser(getContext(), jsonFactory);
}

}
161 changes: 89 additions & 72 deletions src/main/java/net/logstash/logback/pattern/AbstractJsonPatternParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,13 @@
import net.logstash.logback.composite.JsonWritingUtils;
import net.logstash.logback.util.StringUtils;

import ch.qos.logback.core.Context;
import ch.qos.logback.core.pattern.PatternLayoutBase;
import ch.qos.logback.core.spi.ContextAware;
import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParseException;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonPointer;
import com.fasterxml.jackson.core.TreeNode;
import com.fasterxml.jackson.core.filter.FilteringGeneratorDelegate;
import com.fasterxml.jackson.core.filter.TokenFilter;
Expand All @@ -55,13 +56,14 @@ public abstract class AbstractJsonPatternParser<Event> {

/**
* Pattern used to parse and detect {@link AbstractJsonPatternParser.Operation} in a string.
* An operation starts with a #, followed by a name and a pair of {} with possible arguments in between.
*/
public static final Pattern OPERATION_PATTERN = Pattern.compile("\\# (\\w+) (?: \\{ (.*) \\} )?", Pattern.COMMENTS);
public static final Pattern OPERATION_PATTERN = Pattern.compile("\\# (\\w+) (?: \\{ (.*) \\} )", Pattern.COMMENTS);

private final ContextAware contextAware;
private final Context context;
private final JsonFactory jsonFactory;

private final Map<String, Operation<?>> operations = new HashMap<>();
private final Map<String, Operation<Event, ?>> operations = new HashMap<>();


/**
Expand All @@ -70,8 +72,8 @@ public abstract class AbstractJsonPatternParser<Event> {
*/
private boolean omitEmptyFields;

public AbstractJsonPatternParser(final ContextAware contextAware, final JsonFactory jsonFactory) {
this.contextAware = Objects.requireNonNull(contextAware);
AbstractJsonPatternParser(final Context context, final JsonFactory jsonFactory) {
this.context = Objects.requireNonNull(context);
this.jsonFactory = Objects.requireNonNull(jsonFactory);
addOperation("asLong", new AsLongOperation());
addOperation("asDouble", new AsDoubleOperation());
Expand All @@ -85,70 +87,52 @@ public AbstractJsonPatternParser(final ContextAware contextAware, final JsonFact
* @param name the name of the operation
* @param operation the {@link Operation} instance
*/
protected void addOperation(String name, Operation<?> operation) {
protected void addOperation(String name, Operation<Event, ?> operation) {
this.operations.put(name, operation);
}

protected abstract class Operation<T> {
private final boolean requiresData;

Operation(boolean requiresData) {
this.requiresData = requiresData;
}

public boolean requiresData() {
return requiresData;
}

public abstract ValueGetter<T, Event> createValueGetter(String data);
protected interface Operation<Event, T> {
/**
* Create a {@link ValueGetter} implementing the operation on the supplied arguments ({@code data}).
* Throws an {@link IllegalArgumentException} when the supplied data is invalid.
*
* @param data the data to pass to the operation
* @return a {@link ValueGetter} implementing the operation
* @throws IllegalArgumentException if the supplied data is invalid
*/
ValueGetter<T, Event> createValueGetter(String data);
}

protected class AsLongOperation extends Operation<Long> {
public AsLongOperation() {
super(true);
}

protected class AsLongOperation implements Operation<Event, Long> {
@Override
public ValueGetter<Long, Event> createValueGetter(String data) {
return makeLayoutValueGetter(data).andThen(Long::parseLong);
}
}

protected class AsDoubleOperation extends Operation<Double> {
public AsDoubleOperation() {
super(true);
}

protected class AsDoubleOperation implements Operation<Event, Double> {
@Override
public ValueGetter<Double, Event> createValueGetter(String data) {
return makeLayoutValueGetter(data).andThen(Double::parseDouble);
}
}

protected class AsJsonOperation extends Operation<JsonNode> {
public AsJsonOperation() {
super(true);
}

protected class AsJsonOperation implements Operation<Event, JsonNode> {
@Override
public ValueGetter<JsonNode, Event> createValueGetter(String data) {
return makeLayoutValueGetter(data).andThen(this::convert);
return makeLayoutValueGetter(data).andThen(this::convert); //FIXME if constant -> throw exception on invalid json
}

private JsonNode convert(final String value) {
try {
return JsonReadingUtils.readFully(jsonFactory, value);
} catch (IOException e) {
throw new IllegalStateException("Unexpected IOException when reading JSON value (was '" + value + "')", e);
throw new IllegalStateException("Unparsable JSON value (was '" + value + "')", e);
}
}
}

protected class TryJsonOperation extends Operation<Object> {
public TryJsonOperation() {
super(true);
}

protected class TryJsonOperation implements Operation<Event, Object> {
@Override
public ValueGetter<Object, Event> createValueGetter(String data) {
return makeLayoutValueGetter(data).andThen(this::convert);
Expand Down Expand Up @@ -186,13 +170,9 @@ private ValueGetter<?, Event> makeComputableValueGetter(String pattern) {
? matcher.group(2)
: null;

Operation<?> operation = this.operations.get(operationName);
Operation<Event, ?> operation = this.operations.get(operationName);
if (operation != null) {
if (operation.requiresData() && operationData == null) {
contextAware.addError("No parameter provided to operation: " + operationName);
} else {
return operation.createValueGetter(operationData);
}
return operation.createValueGetter(operationData);
}
}
return makeLayoutValueGetter(pattern);
Expand Down Expand Up @@ -223,18 +203,21 @@ protected ValueGetter<String, Event> makeLayoutValueGetter(final String data) {
}


/**
* Initialize a PatternLayout with the supplied format and throw an {@link IllegalArgumentException}
* if the format is invalid.
*
* @param format the pattern layout format
* @return a configured and started {@link PatternLayoutAdapter} instance around the supplied format
* @throws IllegalArgumentException if the supplied format is not a valid PatternLayout
*/
protected PatternLayoutAdapter<Event> buildLayout(String format) {
PatternLayoutBase<Event> layout = createLayout();

if (layout.isStarted()) {
throw new IllegalStateException("PatternLayout should not be started");
}
PatternLayoutAdapter<Event> adapter = new PatternLayoutAdapter<>(createLayout());
adapter.setPattern(format);
adapter.setContext(context);
adapter.start();

layout.setContext(contextAware.getContext());
layout.setPattern(format);
layout.setPostCompileProcessor(null); // Remove EnsureLineSeparation which is there by default

return new PatternLayoutAdapter<>(layout);
return adapter;
}


Expand Down Expand Up @@ -294,8 +277,9 @@ public String getValue(final Event event) {
*
* @param pattern the JSON pattern to parse
* @return a {@link NodeWriter} configured according to the pattern
* @throws JsonPatternException denotes an invalid pattern
*/
public NodeWriter<Event> parse(String pattern) {
public NodeWriter<Event> parse(String pattern) throws JsonPatternException {
if (StringUtils.isEmpty(pattern)) {
return null;
}
Expand All @@ -304,11 +288,10 @@ public NodeWriter<Event> parse(String pattern) {
try (JsonParser jsonParser = jsonFactory.createParser(pattern)) {
node = JsonReadingUtils.readFullyAsObjectNode(jsonFactory, pattern);
} catch (IOException e) {
contextAware.addError("[pattern] is not a valid JSON object", e);
return null;
throw new JsonPatternException("pattern is not a valid JSON object", e);
}

NodeWriter<Event> nodeWriter = new RootWriter<>(parseObject(node));
NodeWriter<Event> nodeWriter = new RootWriter<>(parseObject(JsonPointer.compile("/"), node));
if (omitEmptyFields) {
nodeWriter = new OmitEmptyFieldWriter<>(nodeWriter);
}
Expand All @@ -319,18 +302,24 @@ public NodeWriter<Event> parse(String pattern) {
* Parse a {@link JsonNode} and produce the corresponding {@link NodeWriter}.
*
* @param node the {@link JsonNode} to parse.
* @return a {@link NodeWriter} corresponding to the given json node
* @return a {@link NodeWriter} corresponding to the given JSON node
* @throws JsonPatternException denotes an invalid pattern
*/
private NodeWriter<Event> parseNode(JsonNode node) {
private NodeWriter<Event> parseNode(JsonPointer location, JsonNode node) throws JsonPatternException {
if (node.isTextual()) {
ValueGetter<?, Event> getter = makeComputableValueGetter(node.asText());
return new ValueWriter<>(getter);
try {
ValueGetter<?, Event> getter = makeComputableValueGetter(node.asText());
return new ValueWriter<>(getter);
} catch (RuntimeException e) {
String msg = "Invalid JSON property '" + location + "' (was '" + node.asText() + "'): " + e.getMessage();
throw new JsonPatternException(msg, e.getCause());
}
}
if (node.isArray()) {
return parseArray((ArrayNode) node);
return parseArray(location, (ArrayNode) node);
}
if (node.isObject()) {
return parseObject((ObjectNode) node);
return parseObject(location, (ObjectNode) node);
}
// Anything else, we will be just writing as is (nulls, numbers, booleans and whatnot)
return new ValueWriter<>(g -> node);
Expand All @@ -342,24 +331,28 @@ private NodeWriter<Event> parseNode(JsonNode node) {
*
* @param node the {@link ArrayNode} to parse
* @return a {@link ArrayWriter}
* @throws JsonPatternException denotes an invalid pattern
*/
private ArrayWriter<Event> parseArray(ArrayNode node) {
private ArrayWriter<Event> parseArray(JsonPointer location, ArrayNode node) throws JsonPatternException {
List<NodeWriter<Event>> children = new ArrayList<>();

int index = 0;
for (JsonNode item : node) {
children.add(parseNode(item));
children.add(parseNode(appendPath(location, Integer.toString(index++)), item));
}

return new ArrayWriter<>(children);
}


/**
* Parse an OBJECT json node
* Parse an JSON object node
*
* @param node the {@link ObjectNode} to parse
* @return a {@link ObjectWriter}
* @throws JsonPatternException denotes an invalid pattern
*/
private ObjectWriter<Event> parseObject(ObjectNode node) {
private ObjectWriter<Event> parseObject(JsonPointer location, ObjectNode node) throws JsonPatternException {
ObjectWriter<Event> writer = new ObjectWriter<>();

for (Iterator<Map.Entry<String, JsonNode>> nodeFields = node.fields(); nodeFields.hasNext();) {
Expand All @@ -368,14 +361,26 @@ private ObjectWriter<Event> parseObject(ObjectNode node) {
String fieldName = field.getKey();
JsonNode fieldValue = field.getValue();

NodeWriter<Event> fieldWriter = parseNode(fieldValue);
NodeWriter<Event> fieldWriter = parseNode(appendPath(location, fieldName), fieldValue);
writer.addField(fieldName, fieldWriter);
}

return writer;
}


/**
* Append a path to an existing {@link JsonPointer}
*
* @param ptr the pointer to add the path
* @param path the path to add
* @return a new {@link JsonPointer}
*/
private static JsonPointer appendPath(JsonPointer ptr, String path) {
return ptr.append(JsonPointer.compile("/" + path));
}


//
// -- NodeWriters -----------------------------------------------------------------------------
//
Expand Down Expand Up @@ -546,4 +551,16 @@ public boolean isOmitEmptyFields() {
public void setOmitEmptyFields(boolean omitEmptyFields) {
this.omitEmptyFields = omitEmptyFields;
}


@SuppressWarnings("serial")
public static class JsonPatternException extends Exception {
public JsonPatternException(String message, Throwable cause) {
super(message, cause);
}

public JsonPatternException(String message) {
super(message);
}
}
}
Loading

0 comments on commit 414f482

Please sign in to comment.