Throw better exception on wrong dynamic_templates syntax
#51783
Conversation
|
CLA signature done ! |
dliappis
added
the
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
|
@elasticmachine ok to test |
There was a problem hiding this comment.
Hi @feifeiiiiiiiiiii, thanks for opening this PR. The change generally looks good to me, however I left two comments around a small style issue and the exception message. Also it would be good if you could add a test around this new exception to e.g. RootObjectMapperTests. You can take a look at "testDynamicTemplates" and either add another case there that checks for the exception (there is a nice expectThrows helper method in LuceneTestCase that makes this easy) or add a new test case for that.
| } | ||
| ] | ||
| */ | ||
| if (!(fieldNode instanceof List)) { |
There was a problem hiding this comment.
small style nit: can you use ((fieldNode instanceof List) == false) instead? We prefer that kind of more verbose state to the negate operator for better readability throughout the codebase.
There was a problem hiding this comment.
@cbuescher ok I will fix for you suggest
| ] | ||
| */ | ||
| if (!(fieldNode instanceof List)) { | ||
| throw new MapperParsingException("Dynamic template syntax error"); |
There was a problem hiding this comment.
I think it would be good to provide some more information the user what went wrong. In this case it would be great to tell the user know that dynamic_templates expects an array of named objects or something along those lines.
cbuescher
changed the title
dynamic_templates syntax
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
| } | |
| } |
This closing bracket is too much now, class closed in line 203 already so the test doesn't compile. Please remove the last two lines so the class compiles.
server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java
Outdated
Show resolved
Hide resolved
|
@feifeiiiiiiiiiii thanks for adding the test. Unfortunately the test class doesn't compile now, I also left a small note regarding the exception message. Otherwise looks good, please correct these two things so we can merge once the CI tests pass. |
|
@cbuescher sorry my mistake |
…Mapper.java Co-Authored-By: Christoph Büscher <[email protected]>
Currently, a mappings update request, where dynamic_mappings is an object instead of an array, results in a http response with a 500 code. This PR checks for this condition and throws a MapperParsingException like we do for other malformed mapping cases. Closes #51486
6 participants
Fix issue #51486