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

[BUG] Document GET API is broken for DLS queries that need call to rewrite before createWeight #3088

Closed
msfroh opened this issue Aug 2, 2023 · 1 comment
Assignees
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. v2.10.0 Issues targeting release v2.10.0

Comments

@msfroh
Copy link

msfroh commented Aug 2, 2023

What is the bug?
Recent changes to the Lucene TermInSetQuery removed the createWeight method, which breaks the GET document API in the presence of a DLS query that use terms. The underlying issue is that DlsGetEvaluator calls createWeight without calling rewrite.

How can one reproduce the bug?
Steps to reproduce the behavior:

  1. Create a DLS query based on terms like { "terms": { "foo" : ["bar"] } }
  2. Add some documents to an index.
  3. Issue a GET document call like GET <index>/_doc/5
  4. Receive an error message like Query XXX does not implement createWeight

What is the expected behavior?
The GET document call should succeed and return the relevant document if allowed by the DLS query (or not return it if not allowed by the DLS query).

The fix should be as simple as changing this line to say:

final Weight preserveWeight = searcher.createWeight(dlsQuery.rewrite(searcher), ScoreMode.COMPLETE_NO_SCORES, 1f);

While this probably broke for terms queries as a direct result of this commit, it may have previously been broken for other MultiTermQuery subclasses. In general, Lucene assumes that code paths that call createWeight call rewrite first.

What is your host/environment?

  • OS: N/A
  • Version: OpenSearch 2.8
  • Plugins: N/A

Do you have any screenshots?
Nope

Do you have any additional context?

  • Lucene change that made TermInSetQuery extend MultiTermQuery and removed the createWeight method from the (non-rewritten) Query class: apache/lucene@3809106
  • Conversation from OpenSearch Slack that motivated this issue: https://opensearch.slack.com/archives/C0539F41Z5X/p1690714779140659
  • Note that in that Slack thread, I suggested a workaround of replacing the DLS query with a small number of terms with a single term query if only a single term is being matches or a bool query with should clauses for the relevant terms, since that's what the rewrite method rewrites to anyway (if there are 16 or fewer terms).
@msfroh msfroh added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 2, 2023
@west117
Copy link

west117 commented Aug 2, 2023

Thanks for raising this @msfroh

@peternied peternied added v2.10.0 Issues targeting release v2.10.0 and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 5, 2023
@cwperks cwperks added the triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. label Aug 7, 2023
cwperks added a commit to cwperks/security that referenced this issue Aug 14, 2023
Fixes an error on document retrieval for users that are mapped to roles
with DLS rules including a terms query. The bug was introduced by a
change in Lucene and reported on Slack
[here](https://opensearch.slack.com/archives/C0539F41Z5X/p1690714779140659).
This fix adds a test to catch an error like this at build time and
assert the intended behavior.

* Category (Enhancement, New feature, Bug fix, Test fix, Refactoring,
Maintenance, Documentation)

Bug fix

opensearch-project#3088

- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit 88b6d23)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. v2.10.0 Issues targeting release v2.10.0
Projects
None yet
Development

No branches or pull requests

4 participants