Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve validation error messages for unknown property and json mapping exceptions #5044

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Improve validation error messages for unknown property and json mapping exceptions #5044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions ...pper-pipeline-parser/src/main/java/org/opensearch/dataprepper/validation/PluginError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
@Getter
@Builder
public class PluginError {
static final String PIPELINE_DELIMITER = ":";
static final String PIPELINE_DELIMITER = ".";
static final String PATH_TO_CAUSE_DELIMITER = ":";
static final String CAUSED_BY_DELIMITER = " caused by: ";
private final String pipelineName;
private final String componentType;
Expand All @@ -27,7 +28,7 @@ public String getErrorMessage() {
message.append(PIPELINE_DELIMITER);
}
message.append(pluginName);
message.append(PIPELINE_DELIMITER);
message.append(PATH_TO_CAUSE_DELIMITER);
message.append(getFlattenedExceptionMessage(CAUSED_BY_DELIMITER));
return message.toString();
}
Expand All @@ -37,8 +38,10 @@ private String getFlattenedExceptionMessage(final String delimiter) {
Throwable throwable = exception;

while (throwable != null) {
message.append(delimiter);
message.append(throwable.getMessage());
if (throwable.getMessage() != null) {
message.append(delimiter);
message.append(throwable.getMessage());
}
throwable = throwable.getCause();
}

Expand Down
4 changes: 2 additions & 2 deletions ...-pipeline-parser/src/test/java/org/opensearch/dataprepper/validation/PluginErrorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ void testGetErrorMessageWithPipelineName() {
.exception(exception)
.build();
assertThat(pluginError.getErrorMessage(), equalTo(
"test-pipeline:test-plugin-type:test-plugin: caused by: test error message"));
"test-pipeline.test-plugin-type.test-plugin: caused by: test error message"));
}

@Test
Expand All @@ -38,7 +38,7 @@ void testGetErrorMessageWithoutPipelineName() {
.exception(exception)
.build();
assertThat(pluginError.getErrorMessage(), equalTo(
"test-plugin-type:test-plugin: caused by: test error message"));
"test-plugin-type.test-plugin: caused by: test error message"));
}

@Test
Expand Down
1 change: 1 addition & 0 deletions data-prepper-plugin-framework/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,5 @@ dependencies {
}
implementation libs.reflections.core
implementation 'com.fasterxml.jackson.core:jackson-databind'
implementation 'org.apache.commons:commons-text:1.10.0'
}
14 changes: 12 additions & 2 deletions ...amework/src/main/java/org/opensearch/dataprepper/plugin/PluginConfigurationConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,15 @@ class PluginConfigurationConverter {
private final ObjectMapper objectMapper;
private final Validator validator;

private final PluginConfigurationErrorHandler pluginConfigurationErrorHandler;

PluginConfigurationConverter(final Validator validator,
@Named("pluginConfigObjectMapper")
final ObjectMapper objectMapper) {
final ObjectMapper objectMapper,
final PluginConfigurationErrorHandler pluginConfigurationErrorHandler) {
this.objectMapper = objectMapper;
this.validator = validator;
this.pluginConfigurationErrorHandler = pluginConfigurationErrorHandler;
}

/**
Expand Down Expand Up @@ -80,6 +84,12 @@ private Object convertSettings(final Class<?> pluginConfigurationType, final Plu
Map<String, Object> settingsMap = pluginSetting.getSettings();
if (settingsMap == null)
settingsMap = Collections.emptyMap();
return objectMapper.convertValue(settingsMap, pluginConfigurationType);

try {
return objectMapper.convertValue(settingsMap, pluginConfigurationType);
} catch (final Exception e) {
throw pluginConfigurationErrorHandler.handleException(pluginSetting, e);
}
}

}
91 changes: 91 additions & 0 deletions ...work/src/main/java/org/opensearch/dataprepper/plugin/PluginConfigurationErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package org.opensearch.dataprepper.plugin;

import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
import org.apache.commons.text.similarity.LevenshteinDistance;
import org.opensearch.dataprepper.model.configuration.PluginSetting;
import org.opensearch.dataprepper.model.plugin.InvalidPluginConfigurationException;

import javax.inject.Inject;
import javax.inject.Named;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

@Named
public class PluginConfigurationErrorHandler {

static final String UNRECOGNIZED_PROPERTY_EXCEPTION_FORMAT = "Parameter \"%s\" for plugin \"%s\" does not exist. Available options include %s.";
static final String JSON_MAPPING_EXCEPTION_FORMAT = "Parameter \"%s\" for plugin \"%s\" is invalid: %s";
static final String GENERIC_PLUGIN_EXCEPTION_FORMAT = "Plugin \"%s\" is invalid: %s";

static final Integer MIN_DISTANCE_TO_RECOMMEND_PROPERTY = 3;

private final LevenshteinDistance levenshteinDistance;

@Inject
public PluginConfigurationErrorHandler(final LevenshteinDistance levenshteinDistance) {
this.levenshteinDistance = levenshteinDistance;
}

public RuntimeException handleException(final PluginSetting pluginSetting, final Exception e) {
if (e.getCause() instanceof UnrecognizedPropertyException) {
return handleUnrecognizedPropertyException((UnrecognizedPropertyException) e.getCause(), pluginSetting);
} else if (e.getCause() instanceof JsonMappingException) {
return handleJsonMappingException((JsonMappingException) e.getCause(), pluginSetting);
}

return new InvalidPluginConfigurationException(
String.format(GENERIC_PLUGIN_EXCEPTION_FORMAT, pluginSetting.getName(), e.getMessage()));
}

private RuntimeException handleJsonMappingException(final JsonMappingException e, final PluginSetting pluginSetting) {
final String parameterPath = getParameterPath(e.getPath());

final String errorMessage = String.format(JSON_MAPPING_EXCEPTION_FORMAT,
parameterPath, pluginSetting.getName(), e.getOriginalMessage());

return new InvalidPluginConfigurationException(errorMessage);
}

private RuntimeException handleUnrecognizedPropertyException(final UnrecognizedPropertyException e, final PluginSetting pluginSetting) {
String errorMessage = String.format(UNRECOGNIZED_PROPERTY_EXCEPTION_FORMAT,
getParameterPath(e.getPath()),
pluginSetting.getName(),
e.getKnownPropertyIds());

final Optional<String> closestRecommendation = getClosestField(e);

if (closestRecommendation.isPresent()) {
errorMessage += " Did you mean \"" + closestRecommendation.get() + "\"?";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should come first.

Perhaps:

1. entry-pipeline:processor:parse_json: caused by: Parameter "destintio" for plugin "parse_json" does not exist. Did you mean "destination"? Other available options include [tags_on_failure, handle_failed_events, parse_when, source, pointer, destination, delete_source, overwrite_if_destination_exists].

When not present, it could be:

  1. entry-pipeline:processor:parse_json: caused by: Parameter "blah" for plugin "parse_json" does not exist. Available options include [tags_on_failure, handle_failed_events, parse_when, source, pointer, destination, delete_source, overwrite_if_destination_exists].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually feel like it is easier to see the recommendation when it's at the end with just a glance. If the entire message is read in order then the recommendation coming first is good, but it does seem easier to see when it's at the end.

Copy link
Member Author

@graytaylor0 graytaylor0 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entry-pipeline:processor:parse_json: caused by: Parameter "destintio" for plugin "parse_json" does not exist. Available options include [tags_on_failure, handle_failed_events, parse_when, source, pointer, destination, delete_source, overwrite_if_destination_exists]. Did you mean "destination"?

vs.

entry-pipeline:processor:parse_json: caused by: Parameter "destintio" for plugin "parse_json" does not exist. Did you mean "destination"? Other available options include [tags_on_failure, handle_failed_events, parse_when, source, pointer, destination, delete_source, overwrite_if_destination_exists].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that makes sense.

We probably need to make better use of newlines. Then it would be much cleaner.

entry-pipeline:processor:parse_json: caused by: 

Parameter "destintio" for plugin "parse_json" does not exist. Available options include [tags_on_failure, handle_failed_events, parse_when, source, pointer, destination, delete_source, overwrite_if_destination_exists]. 

Did you mean "destination"?

}

return new InvalidPluginConfigurationException(errorMessage);
}

private Optional<String> getClosestField(final UnrecognizedPropertyException e) {
String closestMatch = null;
int smallestDistance = Integer.MAX_VALUE;

for (final String field : e.getKnownPropertyIds().stream().map(Object::toString).collect(Collectors.toList())) {
int distance = levenshteinDistance.apply(e.getPropertyName(), field);

if (distance < smallestDistance) {
smallestDistance = distance;
closestMatch = field;
}
}

if (smallestDistance <= MIN_DISTANCE_TO_RECOMMEND_PROPERTY) {
return Optional.ofNullable(closestMatch);
}

return Optional.empty();
}

private String getParameterPath(final List<JsonMappingException.Reference> path) {
return path.stream()
.map(JsonMappingException.Reference::getFieldName)
.collect(Collectors.joining("."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should join with : to remain consistent with the top-level?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with consistency, but maybe the consistent thing to do would be to use . everywhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with either as long as it is consistent.

}
}
4 changes: 2 additions & 2 deletions ...epper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/PluginCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ <T> T newPluginInstance(final Class<T> pluginClass,
} catch (final IllegalAccessException | InstantiationException ex) {
LOG.error("Encountered exception while instantiating the plugin {}", pluginClass.getSimpleName(), ex);
throw new InvalidPluginDefinitionException(
"Unable to access or instantiate the plugin '" + pluginClass.getSimpleName() + ".'", ex);
"Unable to access or instantiate the plugin \"" + pluginName + "\".", ex);
} catch (final InvocationTargetException ex) {
LOG.error("Encountered exception while instantiating the plugin {}", pluginClass.getSimpleName(), ex);
throw new PluginInvocationException("Exception throw from the plugin'" + pluginClass.getSimpleName() + "'." , ex);
throw new PluginInvocationException("Exception thrown from plugin \"" + pluginName + "\".", ex.getTargetException());
}
}

Expand Down
6 changes: 6 additions & 0 deletions ...gin-framework/src/main/java/org/opensearch/dataprepper/plugin/ValidatorConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import jakarta.validation.Validation;
import jakarta.validation.Validator;
import jakarta.validation.ValidatorFactory;
import org.apache.commons.text.similarity.LevenshteinDistance;
import org.hibernate.validator.messageinterpolation.ParameterMessageInterpolator;
import org.springframework.context.annotation.Bean;

Expand All @@ -26,4 +27,9 @@ Validator validator() {
.buildValidatorFactory();
return validationFactory.getValidator();
}

@Bean
LevenshteinDistance levenshteinDistance() {
return new LevenshteinDistance();
}
}
56 changes: 52 additions & 4 deletions ...ork/src/test/java/org/opensearch/dataprepper/plugin/PluginConfigurationConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.opensearch.dataprepper.model.configuration.PluginSetting;
import org.opensearch.dataprepper.model.plugin.InvalidPluginConfigurationException;
import jakarta.validation.ConstraintViolation;
import jakarta.validation.Path;
import jakarta.validation.Validator;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.opensearch.dataprepper.model.configuration.PluginSetting;
import org.opensearch.dataprepper.model.plugin.InvalidPluginConfigurationException;

import java.util.Collections;
import java.util.UUID;
Expand All @@ -31,11 +34,16 @@
import static org.mockito.BDDMockito.given;
import static org.mockito.BDDMockito.then;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@ExtendWith(MockitoExtension.class)
class PluginConfigurationConverterTest {
private PluginSetting pluginSetting;
private Validator validator;
private final ObjectMapper objectMapper = new ObjectMapper();

@Mock
private PluginConfigurationErrorHandler pluginConfigurationErrorHandler;
private ObjectMapper objectMapper = new ObjectMapper();

static class TestConfiguration {
@SuppressWarnings("unused")
Expand All @@ -55,7 +63,7 @@ void setUp() {
}

private PluginConfigurationConverter createObjectUnderTest() {
return new PluginConfigurationConverter(validator, objectMapper);
return new PluginConfigurationConverter(validator, objectMapper, pluginConfigurationErrorHandler);
}

@Test
Expand Down Expand Up @@ -165,4 +173,44 @@ void convert_with_other_target_should_throw_exception_when_there_are_constraint_
assertThat(actualException.getMessage(), containsString(propertyPathString));
assertThat(actualException.getMessage(), containsString(errorMessage));
}

@Test
void convert_with_error_when_converting_with_object_mapper_calls_plugin_configuration_error_handler() {
objectMapper = mock(ObjectMapper.class);

final String value = UUID.randomUUID().toString();
given(pluginSetting.getSettings())
.willReturn(Collections.singletonMap("my_value", value));

final RuntimeException e = mock(RuntimeException.class);

when(objectMapper.convertValue(pluginSetting.getSettings(), TestConfiguration.class))
.thenThrow(e);

when(pluginConfigurationErrorHandler.handleException(pluginSetting, e)).thenReturn(new IllegalArgumentException());

final PluginConfigurationConverter objectUnderTest = createObjectUnderTest();

assertThrows(IllegalArgumentException.class, () -> objectUnderTest.convert(TestConfiguration.class, pluginSetting));
}

@Test
void convert_with_error_when_throws_InvalidPluginConfiguration_when_plugin_configuration_error_handler_does_not_throw() {
objectMapper = mock(ObjectMapper.class);

final String value = UUID.randomUUID().toString();
given(pluginSetting.getSettings())
.willReturn(Collections.singletonMap("my_value", value));

final RuntimeException e = mock(RuntimeException.class);

when(objectMapper.convertValue(pluginSetting.getSettings(), TestConfiguration.class))
.thenThrow(e);

when(pluginConfigurationErrorHandler.handleException(pluginSetting, e)).thenReturn(new InvalidPluginConfigurationException(UUID.randomUUID().toString()));

final PluginConfigurationConverter objectUnderTest = createObjectUnderTest();

assertThrows(InvalidPluginConfigurationException.class, () -> objectUnderTest.convert(TestConfiguration.class, pluginSetting));
}
}
Loading
Loading