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.

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@areek
Copy link
Contributor

areek commented Jul 18, 2014

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

@areek areek added the review label Jul 18, 2014

@spinscale

View changes

src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java Outdated
@@ -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)) {

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 21, 2014

Member

Should we do instance checks here or have something similar to AbstractFieldMapper.isSortable() to decide if a mapper supports a certain feature, is isSupportingNullValue?

This comment has been minimized.

Copy link
@areek

areek Jul 21, 2014

Author Contributor

@spinscale Thanks for the suggestion, I think isSupportingNullValue is a cleaner way to do this. Changed as suggested.

@spinscale

View changes

src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchTests.java Outdated

client().prepareIndex(INDEX, TYPE, "2").setSource(FIELD, null, FIELD + "_1", "nulls make me sad")
.setRefresh(true).get();

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 22, 2014

Member

another general question here. Does it make more sense to not silently ignore this but throw an exception (unless specified differently in the mapping)?

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 22, 2014

Contributor

yeah wonder about that too?

@s1monw s1monw removed the review label Jul 22, 2014

@areek

This comment has been minimized.

Copy link
Contributor Author

areek commented Jul 25, 2014

Updated PR: Now having a null value for a completion field will throw an exception.

@jpountz

View changes

src/main/java/org/elasticsearch/index/mapper/FieldMapper.java Outdated
@@ -286,6 +286,8 @@ public static Loading parse(String loading, Loading defaultValue) {

boolean isSortable();

boolean isSupportingNullValue();

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

or shorter: supportsNullValue? (though I might be completely wrong as my English is what it is...)

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 1, 2014

LGTM

@jpountz jpountz removed the review label Aug 1, 2014

@areek

This comment has been minimized.

Copy link
Contributor Author

areek commented Aug 1, 2014

@jpountz thanks for the review, I have changed the name to supportsNullValue (I like it better). I will commit this shortly if there are not objections.

@areek areek closed this Aug 1, 2014

@clintongormley clintongormley changed the title Completion mapping type throws a misleading error on null value Suggesters: Completion mapping type throws a misleading error on null value Sep 10, 2014

@clintongormley clintongormley changed the title Suggesters: Completion mapping type throws a misleading error on null value Completion mapping type throws a misleading error on null value Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.