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

Don't override originalQuery with request filters #15793

Merged
merged 3 commits into from Jan 12, 2016

Conversation

Projects
None yet
3 participants
@nik9000
Copy link
Contributor

commented Jan 6, 2016

These filters leak into highlighting and probably other places and cause things like the type name to be highlighted when using requireFieldMatch=false. We could have special hacks to keep them out of highlighting but it feals better to keep them out of any variable named "originalQuery".

Closes #15689

if (request.rewrite()) {
explanation = getRewrittenQuery(searcher.searcher(), searchContext.query());
} else if (request.explain()) {
explanation = searchContext.filteredQuery().query().toString();

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 6, 2016

Author Contributor

Sorry for moving the code around here. The big change is using filteredQuery instead of parsedQuery.

@clintongormley clintongormley added the >bug label Jan 10, 2016

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2016

@javanna is this something you can review? I'm hunting reviewers....

@javanna

View changes

core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchIT.java Outdated
}
}


This comment has been minimized.

Copy link
@javanna

javanna Jan 12, 2016

Member

one too many new line? :)

@javanna

This comment has been minimized.

Copy link
Member

commented Jan 12, 2016

I found this a bit scary at first as it adds a new method to be used to retrieve the query that we previously relied on (the filtered one) while the default one (parsedQuery) is not filtered anymore. That said I looked at where we call what and it seems ok, also if tests are green I suppose we are good :) LGTM then

Don't override originalQuery with request filters
These filters leak into highlighting and probably other places and cause
things like the type name to be highlighted when using
requireFieldMatch=false. We could have special hacks to keep them out of
highlighting but it feals better to keep them out of any variable named
"originalQuery".

Closes #15689

@nik9000 nik9000 force-pushed the nik9000:highlight_typename branch to d3a4a9f Jan 12, 2016

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2016

a bit scary

Yeah! I believe it! Its weird to have these three, but I'm not sure of a better way to do this. I think we lucked out that this worked properly before the query/filter merger.

nik9000 added a commit that referenced this pull request Jan 12, 2016

Merge pull request #15793 from nik9000/highlight_typename
Don't override originalQuery with request filters

@nik9000 nik9000 merged commit da63c87 into elastic:master Jan 12, 2016

1 check passed

CLA Commit author has signed the CLA
Details
@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2016

Backporting.....

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2016

I'm so not used to maven....

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2016

And cherry-picked to 2.x: 6ddd233 53d41a1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.