-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Provide informative error message in case of unknown suggestion context. #24241
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
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
nik9000
left a comment
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.
I left a minor thing but it makes sense to me. If you'll make the change I can see about merging.
| ContextMapping contextMapping = contextNameMap.get(name); | ||
| if (contextMapping == null) { | ||
| throw new IllegalArgumentException("Unknown context name[" + name + "], must be one of " + contextNameMap.size()); | ||
| throw new IllegalArgumentException("Unknown context name[" + name + "], must be one of " + contextNameMap.keySet().toString()); |
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.
I think we'd better sort the list of keys before adding it to the string so we get consistent error messages. This'll make testing much easier and be marginally easier on the eyes.
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.
Can you add a space between "name" and "["?
|
Thanks @nik9000 I agree, sorting will have consistent behaviour. I will do both the changes as suggested and commit. |
…nknown query context.
|
@nik9000 changes done. Please verify |
nik9000
left a comment
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.
Looks good to me. I'll work on getting this merged.
|
elasticmachine, test this please. |
|
Thanks @nilabhsagar! I've merged to master and an backporting to 5.x. |
|
Thanks @nik9000! |
…xt. (#24241) Provide a list of available contexts when you send an unknown context to the completion suggester.
At present providing unknown context in a suggestion context query does not provide informative error message.
Consider below mapping and correspongding query
Mapping
Query
Executing the above query will show error message as below
"Unknown context name[product_brand], must be one of 3"
This error message is not very informative as it only convey the number of context supported. However printing all the accepted contexts will be more informative, like below:
"Unknown context name[product_brand], must be one of [product_size, product_color, product_type]"
This pull request fixes the issue mentioned.