Add matching pattern to error in fields #76903
Conversation
This adds the pattern into the error message returned when trying to
fetch fields. So this:
```
POST _search {
"fields": [ { "field": "*", "format": "date_time" } ]
}
```
Will return an error message like
```
error fetching [foo] which matches [*]: [keyword] doesn't support formats
```
It also centralizes the code adding the `error fetching [foo]` bit of
the error message so we don't have to be super duper careful to make
sure the field name comes back.
nik9000
added
>non-issue
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
|
I was debugging an issue this morning where folks were getting this error while requesting dozens of |
| if (isWildcardPattern) { | ||
| error.append(" which matched [").append(fieldAndFormat.field).append(']'); | ||
| } | ||
| error.append(": ").append(e.getMessage()); |
There was a problem hiding this comment.
I'm not a fan of piecing exception messages together in this way (in particular because IllegalArgumentException is a generic exception).
Also, it's unclear to me how the change in this PR will affect other call sites of the valueFetcher method (those will now not return the field name anymore and might be less precise / harder to debug).
There was a problem hiding this comment.
If I were running this in production myself I wouldn't do it, no. But I think its useful because we drop the getMessage() from the top level cause into the reason field of the error by default. I really care that the reason describes exactly what failed here.
There was a problem hiding this comment.
About the other call sites - I believe there is only one non-test call that doesn't flow through here and it is in highlighting which calls the method in a way that won't throw. But the method is public and, who knows, highlighting may throw. I'll revert this and be happy with the more verbose message. It isn't worth that change.
| @@ -57,7 +57,7 @@ public Query termQuery(Object value, SearchExecutionContext context) { | |||
| @Override | |||
| public ValueFetcher valueFetcher(SearchExecutionContext context, String format) { | |||
| if (format != null) { | |||
| throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats."); | |||
| throw new IllegalArgumentException("[" + typeName() + "] doesn't support formats."); | |||
There was a problem hiding this comment.
This check looks the same everywhere. How about factoring it out into its own method, e.g. checkNoFormats(format) in MappedFieldType that can be called in all these methods.
| fieldContexts.put(field, new FieldContext(field, valueFetcher)); | ||
| try { | ||
| ValueFetcher valueFetcher = ft.valueFetcher(context, fieldAndFormat.format); | ||
| fieldContexts.put(field, new FieldContext(field, valueFetcher)); |
There was a problem hiding this comment.
let's put the try catch only around the valueFetcher method
I think after having removed them all I think I've come around to your way of thinking - more verbose is fine here. I'll undo that. |
💔 Backport failed
To backport manually run |
This adds the pattern into the error message returned when trying to
fetch fields. So this:
```
POST _search {
"fields": [ { "field": "*", "format": "date_time" } ]
}
```
Will return an error message like
```
error fetching [foo] which matches [*]: Field [foo] of type [keyword] doesn't support formats
```
|
Backport is here: #77191 . After backport I'll need bump the skip versions too. |
This adds the pattern into the error message returned when trying to
fetch fields. So this:
```
POST _search {
"fields": [ { "field": "*", "format": "date_time" } ]
}
```
Will return an error message like
```
error fetching [foo] which matches [*]: Field [foo] of type [keyword] doesn't support formats
```
Now that we've backported elastic#76903 we can run backwards compatibility tests against all versions that have it.
Now that we've backported #76903 we can run backwards compatibility tests against all versions that have it.
This adds the pattern into the error message returned when trying to
fetch fields. So this:
Will return an error message like
It also centralizes the code adding the
error fetching [foo]bit ofthe error message so we don't have to be super duper careful to make
sure the field name comes back.