Improve validation error messages for unknown property and json mapping exceptions #5044
Conversation
graytaylor0
requested review from
chenqi0805,
engechas,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
graytaylor0
force-pushed
the
ValidationErrorImprovements
branch
from
16aea47 to
06f7197
Compare
| final Optional<String> closestRecommendation = getClosestField(e); | ||
|
|
||
| if (closestRecommendation.isPresent()) { | ||
| errorMessage += " Did you mean \"" + closestRecommendation.get() + "\"?"; |
There was a problem hiding this comment.
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:
- 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].
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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"?
| try { | ||
| return objectMapper.convertValue(settingsMap, pluginConfigurationType); | ||
| } catch (final Exception e) { | ||
| pluginConfigurationErrorHandler.handleException(pluginSetting, e); |
There was a problem hiding this comment.
| pluginConfigurationErrorHandler.handleException(pluginSetting, e); | |
| throw pluginConfigurationErrorHandler.handleException(pluginSetting, e); |
You can change handleException to return an Exception.
Then you should be able to remove the throw below.
There was a problem hiding this comment.
Will make this change
| private String getParameterPath(final List<JsonMappingException.Reference> path) { | ||
| return path.stream() | ||
| .map(JsonMappingException.Reference::getFieldName) | ||
| .collect(Collectors.joining(".")); |
There was a problem hiding this comment.
Maybe we should join with : to remain consistent with the top-level?
There was a problem hiding this comment.
I agree with consistency, but maybe the consistent thing to do would be to use . everywhere?
There was a problem hiding this comment.
I'm ok with either as long as it is consistent.
graytaylor0
force-pushed
the
ValidationErrorImprovements
branch
from
06f7197 to
7d6dd90
Compare
…ng exceptions Signed-off-by: Taylor Gray <[email protected]>
graytaylor0
force-pushed
the
ValidationErrorImprovements
branch
from
7d6dd90 to
aa3cb24
Compare
Description
This change improves the error messaging for unknown plugin properties and json mapping exceptions
Old error for unknown property
New error for unknown property
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.