-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add matching pattern to error in fields #76903
Changes from 4 commits
8c9cccc
e3ed135
1814043
78b1cba
e9e05ea
3dfc5ce
f10c0dc
830f0c1
0f15553
d65037c
ac7de99
10c8ef5
fd67f2c
be7c84b
9434fec
8b98a9b
06f448f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,8 +92,17 @@ private static FieldFetcher create(SearchExecutionContext context, | |
} | ||
// only add concrete fields if they are not beneath a known nested field | ||
if (nestedParentPath == null) { | ||
ValueFetcher valueFetcher = ft.valueFetcher(context, fieldAndFormat.format); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's put the try catch only around the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. |
||
} catch (IllegalArgumentException e) { | ||
StringBuilder error = new StringBuilder("error fetching [").append(field).append(']'); | ||
if (isWildcardPattern) { | ||
error.append(" which matched [").append(fieldAndFormat.field).append(']'); | ||
} | ||
error.append(": ").append(e.getMessage()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of piecing exception messages together in this way (in particular because 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I were running this in production myself I wouldn't do it, no. But I think its useful because we drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
throw new IllegalArgumentException(error.toString(), e); | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check looks the same everywhere. How about factoring it out into its own method, e.g.
checkNoFormats(format)
inMappedFieldType
that can be called in all these methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#77089