Fix error applying ignore_malformed to boolean values
#41261
Conversation
The `ignore_malformed` option currently works on numeric fields only when the bad value isn't a string value but not if it is a boolean. In this case we get a parsing error from the xContent parser which we need to catch in addition to the field mapper. Closes elastic#11498
cbuescher
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java
Outdated
Show resolved
Hide resolved
| @@ -1042,7 +1044,7 @@ protected void parseCreateField(ParseContext context, List<IndexableField> field | |||
| } else { | |||
| try { | |||
| numericValue = fieldType().type.parse(parser, coerce.value()); | |||
| } catch (IllegalArgumentException e) { | |||
| } catch (IllegalArgumentException | JsonParseException e) { | |||
There was a problem hiding this comment.
I haven't tested this, but I wanted to check that this will not introduce too much leniency. In particular, I'm wondering what happens if we try to pass a JSON object to a numeric field with ignore_malformed enabled.
There was a problem hiding this comment.
The way the parser works, anything that's not a number or string token after the field name should throw an error. If "swallowing" that token with ignore_malformed leads to unparseable xcontent afterwards we will still throw an error. I think that's what should happen.
There was a problem hiding this comment.
Thanks for clarifying. What would you think about adding a check whether the token is a value, as in if (ignoreMalformed.value() && parser.currentToken().isValue()) { ... }? That way a user would get a clear, localized error like "Current token (START_OBJECT) not numeric", as opposed to a more difficult one to track down like "Malformed content, found extra data after parsing: END_OBJECT".
There was a problem hiding this comment.
The problem with that is that the code actually throwing the exception is in the jackson library. The only way I could think of to do this is either have some logic taking apart the exception or pre-checking the token in NumberFieldMapper. I'd try to avoid the former, was the later what you had in mind? We'd need to test for (and accept) all value tokens here basically?
|
@jtibshirani thanks for the review, I added the check for the token type you suggested and added a test where the malformed field value is an object. The current implementation will throw erros in numeric fields regardless of the |
There was a problem hiding this comment.
Thanks @cbuescher, this looks good to me. My intuition is that the whole object should be skipped, but it might make sense to address the object case in a future PR since there is a separate issue for it with quite a bit of discussion (#12366).
|
@jtibshirani thanks for the review, and +1 to tackling the object case as a follow up, e.g. when tackling the issue you linked. |
The `ignore_malformed` option currently works on numeric fields only when the bad value isn't a string value but not if it is a boolean. In this case we get a parsing error from the xContent parser which we need to catch in addition to the field mapper. Closes #11498
The `ignore_malformed` option currently works on numeric fields only when the bad value isn't a string value but not if it is a boolean. In this case we get a parsing error from the xContent parser which we need to catch in addition to the field mapper. Closes #11498
The `ignore_malformed` option currently works on numeric fields only when the bad value isn't a string value but not if it is a boolean. In this case we get a parsing error from the xContent parser which we need to catch in addition to the field mapper. Closes elastic#11498
The
ignore_malformedoption currently works on numeric fields only when thebad value isn't a string value but not if it is a boolean. In this case we get a
parsing error from the xContent parser which we need to catch in addition to the
field mapper.
Closes #11498