Skip to content

Commit

Permalink
Add validator to check model classes for de/serialization issues (#18277
Browse files Browse the repository at this point in the history
)

* Add validator to check model classes for de/serialization issues
* Enable model validator
* Fix JsonSubtype issues
* Add re-usable assertion
* Remove duplicate type field of LookupCacheConfiguration
* Remove duplicate type field of LookupDataAdapterConfiguration
* Remove duplicate type field of SortSpec
* Rename "assertThat" to "assertThatDto"
  That way we can use static imports to shorten usages.
* Add missing JsonProperty annotation in HighlightingColor
* Improve recursion prevention
* Only log errors for now, so we can have a full test run
* Use static imports
* Remove redundant type info for TimeRange
  Add TimeRange test.
* Add missing JsonProperty annotation
* Extend validator to check for issues with abstract classes
* Scope abstract class check to Id.NAME usages
* Enable exceptions and add unit test
* Disable #check call in collection factories
  Several DB service aren't singletons yet and that would make the check
  run every time one of those services gets instantiated.

---------

Co-authored-by: Othello Maurer <[email protected]>
  • Loading branch information
bernd and thll authored Feb 20, 2024
1 parent d623aae commit cd3c7a9
Show file tree
Hide file tree
Showing 19 changed files with 683 additions and 70 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.graylog.plugins.views.migrations.V20191203120602_MigrateSavedSearchesToViewsSupport.savedsearch;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.google.common.base.Splitter;
Expand All @@ -38,6 +39,7 @@ public abstract class Query {
private final String TIMESTAMP_FIELD = "timestamp";
private final List<String> DEFAULT_FIELDS = ImmutableList.of(TIMESTAMP_FIELD, "source", "message");

@JsonProperty("rangeType")
abstract String rangeType();
abstract Optional<String> fields();
public abstract String query();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ public interface ExportJob {
@JsonProperty(FIELD_ID)
String id();

@JsonProperty
@JsonProperty(FIELD_TYPE)
String type();

@JsonProperty(FIELD_CREATED_AT)
DateTime createdAt();
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.google.auto.value.AutoValue;
Expand All @@ -30,15 +28,12 @@
import org.graylog.plugins.views.search.rest.SearchTypeExecutionState;
import org.graylog.plugins.views.search.searchfilters.model.UsedSearchFilter;
import org.graylog.plugins.views.search.timeranges.DerivedTimeRange;
import org.graylog.plugins.views.search.timeranges.OffsetRange;
import org.graylog2.contentpacks.EntityDescriptorIds;
import org.graylog2.contentpacks.model.entities.MessageListEntity;
import org.graylog2.contentpacks.model.entities.SearchTypeEntity;
import org.graylog2.decorators.Decorator;
import org.graylog2.decorators.DecoratorImpl;
import org.graylog2.plugin.indexer.searches.timeranges.AbsoluteRange;
import org.graylog2.plugin.indexer.searches.timeranges.KeywordRange;
import org.graylog2.plugin.indexer.searches.timeranges.RelativeRange;
import org.graylog2.plugin.indexer.searches.timeranges.TimeRange;
import org.graylog2.rest.models.messages.responses.DecorationStats;
import org.graylog2.rest.models.messages.responses.ResultMessageSummary;
Expand Down Expand Up @@ -169,13 +164,6 @@ public static Builder createDefault() {
public abstract Builder fields(List<String> fields);

@JsonProperty
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", visible = false)
@JsonSubTypes({
@JsonSubTypes.Type(name = AbsoluteRange.ABSOLUTE, value = AbsoluteRange.class),
@JsonSubTypes.Type(name = RelativeRange.RELATIVE, value = RelativeRange.class),
@JsonSubTypes.Type(name = KeywordRange.KEYWORD, value = KeywordRange.class),
@JsonSubTypes.Type(name = OffsetRange.OFFSET, value = OffsetRange.class)
})
public Builder timerange(@Nullable TimeRange timerange) {
return timerange(timerange == null ? null : DerivedTimeRange.of(timerange));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@
package org.graylog.plugins.views.search.searchtypes.pivot;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.google.auto.value.AutoValue;
Expand All @@ -29,22 +26,16 @@
import org.graylog.plugins.views.search.engine.BackendQuery;
import org.graylog.plugins.views.search.rest.SearchTypeExecutionState;
import org.graylog.plugins.views.search.searchfilters.model.UsedSearchFilter;
import org.graylog.plugins.views.search.searchtypes.pivot.buckets.Values;
import org.graylog.plugins.views.search.timeranges.DerivedTimeRange;
import org.graylog.plugins.views.search.timeranges.OffsetRange;
import org.graylog2.contentpacks.EntityDescriptorIds;
import org.graylog2.contentpacks.model.entities.PivotEntity;
import org.graylog2.contentpacks.model.entities.SearchTypeEntity;
import org.graylog2.plugin.indexer.searches.timeranges.AbsoluteRange;
import org.graylog2.plugin.indexer.searches.timeranges.KeywordRange;
import org.graylog2.plugin.indexer.searches.timeranges.RelativeRange;
import org.graylog2.plugin.indexer.searches.timeranges.TimeRange;

import javax.annotation.Nullable;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.Set;
import java.util.UUID;

Expand Down Expand Up @@ -182,13 +173,6 @@ public Builder sort(SortSpec... sort) {
public abstract Builder filters(List<UsedSearchFilter> filters);

@JsonProperty
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", visible = false)
@JsonSubTypes({
@JsonSubTypes.Type(name = AbsoluteRange.ABSOLUTE, value = AbsoluteRange.class),
@JsonSubTypes.Type(name = RelativeRange.RELATIVE, value = RelativeRange.class),
@JsonSubTypes.Type(name = KeywordRange.KEYWORD, value = KeywordRange.class),
@JsonSubTypes.Type(name = OffsetRange.OFFSET, value = OffsetRange.class)
})
public Builder timerange(@Nullable TimeRange timerange) {
return timerange(timerange == null ? null : DerivedTimeRange.of(timerange));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

@JsonTypeInfo(
use = JsonTypeInfo.Id.NAME,
include = JsonTypeInfo.As.PROPERTY,
include = JsonTypeInfo.As.EXISTING_PROPERTY,
property = SortSpec.TYPE_FIELD,
visible = true)
public interface SortSpec {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.graylog.plugins.views.search.views.formatting.highlighting;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;

Expand All @@ -25,5 +26,6 @@
})
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type")
public interface HighlightingColor {
@JsonProperty("type")
String type();
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.google.auto.value.AutoValue;
Expand All @@ -30,13 +28,9 @@
import org.graylog.plugins.views.search.searchtypes.MessageList;
import org.graylog.plugins.views.search.searchtypes.Sort;
import org.graylog.plugins.views.search.timeranges.DerivedTimeRange;
import org.graylog.plugins.views.search.timeranges.OffsetRange;
import org.graylog2.contentpacks.model.entities.references.ValueReference;
import org.graylog2.decorators.Decorator;
import org.graylog2.decorators.DecoratorImpl;
import org.graylog2.plugin.indexer.searches.timeranges.AbsoluteRange;
import org.graylog2.plugin.indexer.searches.timeranges.KeywordRange;
import org.graylog2.plugin.indexer.searches.timeranges.RelativeRange;
import org.graylog2.plugin.indexer.searches.timeranges.TimeRange;

import javax.annotation.Nullable;
Expand Down Expand Up @@ -128,13 +122,6 @@ public static Builder createDefault() {
public abstract Builder filters(List<UsedSearchFilter> filters);

@JsonProperty
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", visible = false)
@JsonSubTypes({
@JsonSubTypes.Type(name = AbsoluteRange.ABSOLUTE, value = AbsoluteRange.class),
@JsonSubTypes.Type(name = RelativeRange.RELATIVE, value = RelativeRange.class),
@JsonSubTypes.Type(name = KeywordRange.KEYWORD, value = KeywordRange.class),
@JsonSubTypes.Type(name = OffsetRange.OFFSET, value = OffsetRange.class)
})
public Builder timerange(@Nullable TimeRange timerange) {
return timerange(timerange == null ? null : DerivedTimeRange.of(timerange));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.google.auto.value.AutoValue;
Expand All @@ -33,11 +31,7 @@
import org.graylog.plugins.views.search.searchtypes.pivot.SortSpec;
import org.graylog.plugins.views.search.searchtypes.pivot.buckets.Values;
import org.graylog.plugins.views.search.timeranges.DerivedTimeRange;
import org.graylog.plugins.views.search.timeranges.OffsetRange;
import org.graylog2.contentpacks.model.entities.references.ValueReference;
import org.graylog2.plugin.indexer.searches.timeranges.AbsoluteRange;
import org.graylog2.plugin.indexer.searches.timeranges.KeywordRange;
import org.graylog2.plugin.indexer.searches.timeranges.RelativeRange;
import org.graylog2.plugin.indexer.searches.timeranges.TimeRange;

import javax.annotation.Nullable;
Expand Down Expand Up @@ -160,13 +154,6 @@ public static Builder createDefault() {
public abstract Builder filters(List<UsedSearchFilter> filters);

@JsonProperty
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", visible = false)
@JsonSubTypes({
@JsonSubTypes.Type(name = AbsoluteRange.ABSOLUTE, value = AbsoluteRange.class),
@JsonSubTypes.Type(name = RelativeRange.RELATIVE, value = RelativeRange.class),
@JsonSubTypes.Type(name = KeywordRange.KEYWORD, value = KeywordRange.class),
@JsonSubTypes.Type(name = OffsetRange.OFFSET, value = OffsetRange.class)
})
public Builder timerange(@Nullable TimeRange timerange) {
return timerange(timerange == null ? null : DerivedTimeRange.of(timerange));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/*
* Copyright (C) 2020 Graylog, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the Server Side Public License, version 1,
* as published by MongoDB, Inc.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* Server Side Public License for more details.
*
* You should have received a copy of the Server Side Public License
* along with this program. If not, see
* <http://www.mongodb.com/licensing/server-side-public-license>.
*/
package org.graylog2.jackson;

import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.databind.BeanDescription;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationConfig;
import com.fasterxml.jackson.databind.annotation.JsonTypeIdResolver;
import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition;
import com.fasterxml.jackson.databind.jsontype.NamedType;
import com.fasterxml.jackson.databind.ser.BeanPropertyWriter;
import com.fasterxml.jackson.databind.ser.BeanSerializerModifier;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.UncheckedIOException;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

import static org.graylog2.shared.utilities.StringUtils.f;

/**
* Validates model or DTO objects for correct annotation usage. It mainly checks if the configured annotations
* for subtypes use the correct settings.
*/
public class JacksonModelValidator {
private static final Logger LOG = LoggerFactory.getLogger(JacksonModelValidator.class);

/**
* Validates a class by inspecting its Jackson specific annotations. Can be used to validate classes on
* application startup. The bean serializer modifier only runs when the first object serialization happens.
*
* @param collectionName the collection name that the given class is stored in
* @param objectMapper the Jackson object mapper
* @param clazz the class that should be validated
*/
public static void check(String collectionName, ObjectMapper objectMapper, Class<?> clazz) {
if (LOG.isDebugEnabled()) {
LOG.debug("CHECK [{}] {}", collectionName, clazz.getCanonicalName());
}

final var config = objectMapper.getSerializationConfig();
final var ai = config.getAnnotationIntrospector();
final var beanDesc = config.introspect(objectMapper.constructType(clazz));

try {
objectMapper.getSerializerProviderInstance().findTypedValueSerializer(clazz, true, null);
} catch (JsonMappingException e) {
throw new UncheckedIOException(e);
}

// AnnotationIntrospector#findSubtypes finds all subtypes going back up the parent class chain. That can lead
// to recursion, so we only try to find subtypes for classes that are annotated with JsonSubTypes.
if (beanDesc.getBeanClass().isAnnotationPresent(JsonSubTypes.class)) {
if (LOG.isDebugEnabled()) {
LOG.debug("ITERATE SUBTYPES [{}] {}", collectionName, clazz.getCanonicalName());
}
final List<NamedType> subtypes = ai.findSubtypes(beanDesc.getClassInfo());
if (subtypes != null) {
for (NamedType subtype : subtypes) {
if (LOG.isDebugEnabled()) {
LOG.debug("CHECK SUBTYPE [{}] {} -> {}", collectionName, clazz.getCanonicalName(), subtype.getType().getCanonicalName());
}
check(collectionName, objectMapper, subtype.getType());
}
}
}
}

public static BeanSerializerModifier getBeanSerializerModifier() {
return new ModelValidationBeanSerializerModifier();
}

private static class ModelValidationBeanSerializerModifier extends BeanSerializerModifier {
@Override
public List<BeanPropertyWriter> changeProperties(SerializationConfig config, BeanDescription beanDesc, List<BeanPropertyWriter> beanProperties) {
final var annotatedClass = beanDesc.getClassInfo();

if (annotatedClass.hasAnnotation(JsonTypeInfo.class)) {
final var fieldNames = beanDesc.findProperties().stream()
.map(BeanPropertyDefinition::getName)
.collect(Collectors.toSet());

final var jsonTypeInfo = annotatedClass.getAnnotation(JsonTypeInfo.class);
switch (jsonTypeInfo.include()) {
case PROPERTY -> {
if (fieldNames.contains(jsonTypeInfo.property())) {
// When the property for the subtype conflicts with an existing field in the class,
// Jackson will generate the field twice.
//
// Example: {"type": "foo", "type": "foo"}
//
// This is not an issue if both fields have the same value, but it can become
// problematic when the values differ. Specifically, when using abstract classes
// (auto-value) for the JsonSubType.Type values, Jackson might generate two different
// values.
//
// Example: {"type": "AutoValue_Foo", "type": "foo"}
//
// (see below where we check for existing type name annotations and abstract classes)
throw new RuntimeException(f("JsonTypeInfo#property value conflicts with existing property: %s (class %s)", jsonTypeInfo.property(), annotatedClass.getName()));
}
if (jsonTypeInfo.use() == JsonTypeInfo.Id.NAME
&& annotatedClass.hasAnnotation(JsonSubTypes.class)
&& !annotatedClass.hasAnnotation(JsonTypeIdResolver.class)) {
// When using abstract classes that don't have a @JsonTypeName annotation as the value
// for @JsonSubTypes.Type annotations, Jackson cannot look up the "name" value for the
// subtype and will use the class name as a fallback. (e.g., {"type": "AutoValue_ClassName"})
// This is not an issue when a custom @JsonTypeIdResolver is present on the superclass.
final var invalidClasses = Arrays.stream(annotatedClass.getAnnotation(JsonSubTypes.class).value())
.map(JsonSubTypes.Type::value)
.map(config::constructType)
.filter(JavaType::isAbstract)
.map(JavaType::getRawClass)
.filter(clazz -> !clazz.isAnnotationPresent(JsonTypeName.class))
.map(Class::getCanonicalName)
.toList();

if (!invalidClasses.isEmpty()) {
throw new RuntimeException(f("@JsonSubTypes.Type values that are abstract classes (e.g., auto-value) must have a @JsonTypeName annotation or a custom @JsonTypeIdResolver. Affected classes: %s", invalidClasses));
}
}
}
case EXISTING_PROPERTY -> {
if (!fieldNames.contains(jsonTypeInfo.property())) {
// Jackson cannot deserialize values where the existing property is not present.
// This check helps to detect this on serialization already.
throw new RuntimeException(f("JsonTypeInfo#property value doesn't exist as property: %s (class %s)", jsonTypeInfo.property(), annotatedClass.getName()));
}
}
default -> {
// Nothing to do
}
}
}
return beanProperties;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import org.graylog.plugins.views.search.timeranges.OffsetRange;
import org.joda.time.DateTime;

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", visible = false)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type")
@JsonSubTypes({
@JsonSubTypes.Type(name = AbsoluteRange.ABSOLUTE, value = AbsoluteRange.class),
@JsonSubTypes.Type(name = RelativeRange.RELATIVE, value = RelativeRange.class),
@JsonSubTypes.Type(name = KeywordRange.KEYWORD, value = KeywordRange.class)
@JsonSubTypes.Type(name = KeywordRange.KEYWORD, value = KeywordRange.class),
@JsonSubTypes.Type(name = OffsetRange.OFFSET, value = OffsetRange.class)
})
public abstract class TimeRange {

Expand Down
Loading

0 comments on commit cd3c7a9

Please sign in to comment.