Skip to content
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

explainOther fixes #109

Merged
merged 1 commit into from
Aug 11, 2022
Merged

explainOther fixes #109

merged 1 commit into from
Aug 11, 2022

Conversation

worleydl
Copy link
Contributor

@worleydl worleydl commented Aug 9, 2022

A couple issues were breaking "Show Only Rated" in quepid. Highlighting on _id after a terms match in ES causes a index out of bounds exception. Also, the API method in explainOther needed to have the right case, might make a constants file at some point.

No changes required to Quepid other than cutting a new splainer release and bumping it.

- Remove _id from possible highlight fields as this can trigger a bug in
ES
- Fix explainOther to use consistent apiMethod
@worleydl worleydl changed the title Fixes required to address issues in quepid explainOther fixes Aug 9, 2022
@@ -43,6 +43,13 @@ angular.module('o19s.splainer-search')
var hl = { fields: {} };

angular.forEach(fields, function(fieldName) {
/*
* ES doesn't like highlighting on _id if the query has been filtered on _id using a terms query.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we open a ticket with ES to get this fixed? Or is this a "works as designed" limitation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It smells like a bug to me tho highlighting on the _id field is a bit odd and probably not supported under the hood anyways. In any case, it would be better if it didn't 500 the request, or at least give a more useful message when it does.

@epugh epugh merged commit 7ba40c2 into master Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants