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.
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
Completion mapping type throws a misleading error on null value #6926
Conversation
@@ -533,7 +534,7 @@ public void parse(ParseContext context) throws IOException { | |||
private void serializeNullValue(ParseContext context, String lastFieldName) throws IOException { | |||
// we can only handle null values if we have mappings for them | |||
Mapper mapper = mappers.get(lastFieldName); | |||
if (mapper != null) { | |||
if (mapper != null && !(mapper instanceof CompletionFieldMapper)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do instance checks here or have something similar to AbstractFieldMapper.isSortable()
to decide if a mapper supports a certain feature, is isSupportingNullValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spinscale Thanks for the suggestion, I think isSupportingNullValue
is a cleaner way to do this. Changed as suggested.
Updated PR: Now having a null value for a completion field will throw an exception. |
@@ -286,6 +286,8 @@ public static Loading parse(String loading, Loading defaultValue) { | |||
|
|||
boolean isSortable(); | |||
|
|||
boolean isSupportingNullValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or shorter: supportsNullValue
? (though I might be completely wrong as my English is what it is...)
LGTM |
@jpountz thanks for the review, I have changed the name to |
When the mapper service gets a null value for a field, it tries to parse it with the mapper of any previously seen field, if that mapper happens to be CompletionMapper, it throws an error, as it only accepts certain fields.
Closes #6399