Enhanced lat/long error handling#16833
Conversation
Addresses issue elastic#16137. NumberFormatExceptions caused by non-double lat/long values are now handled when the ignore_malformed flag is set to true.
…alidateLatLonValues See issue elastic#16137
| return point.resetFromGeoHash(geohash); | ||
| } | ||
| } else if (numberFormatException != null) { | ||
| throw new ElasticsearchParseException("[{}] and [{}] must be valid double values", LATITUDE, LONGITUDE); |
There was a problem hiding this comment.
This swallows the numberFormatException but it should instead be wrapped by the ElasticsearchParseException.
|
Thanks @jasontedor, I hadn't familiarized myself with the |
|
@jbertouch Can you add tests in |
…idateLatLonValues See issue elastic#16137
|
Hi @jasontedor, I added additional test failing cases to |
|
FWIW, I ran a quick regex over the ES test packages - these are the tests with unvalidated/empty catch clauses for exceptions if you want to review them in future:
|
|
This is looking good; can you add one more thing? In particular, assertions on the exception message? |
|
Hi Jason, I added/tested the extra assertions. |
| .bytes()); | ||
| fail(); | ||
| } catch (MapperParsingException e) { | ||
| assertTrue(e.getRootCause() instanceof NumberFormatException); |
There was a problem hiding this comment.
Can you use the dedicated instanceOf matcher instead of assertTrue? So, assertThat with instanceOf? The advantage of using the dedicated matchers is that it gives very nice error messages when the assertion fails (so with assertTrue all that we see when a test fails is something like "expected true but was false" and with the assertThat and a dedicated matcher something like "expected an instance of NumberFormatException but was IllegalArgumentException").
@jbertouch Thanks, I left a comment that applies to all of the assertions. Getting close. :) |
|
@jasontedor Done! |
Great, thank you so much! I took a quick peek and it looks good. I'll take a final closer look later and wrap this up accordingly. |
| return point.resetFromGeoHash(geohash); | ||
| } | ||
| } else if (numberFormatException != null) { | ||
| throw new ElasticsearchParseException("[{}] and [{}] must be valid double values", numberFormatException, LATITUDE, LONGITUDE); |
There was a problem hiding this comment.
This line will break our precommit checks because it violates the 140-character line-length limit.
|
@jbertouch The change looks great, thank you! There is one line that has to be fixed to meet the line-length limits or it will break the build on integration. There are also some lines where the indentation doesn't match our code style. I marked them in the PR review; do you mind fixing those too when you hit the line-length limit? It's fine if not, I'll handle them with an extra commit upon integration but since you have to push one more commit anyway I think you can hit them then? 😄 Finally, do you mind if I squash all of these commits into one when I integrate this PR? Thanks again! |
Closes #16137.