-
Notifications
You must be signed in to change notification settings - Fork 37
fix: extra filter not needed param #443
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
Original issue: custom requests from https://github.com/tu-graz-library/invenio-curations are now shown when searching for a user's requests. Cause: apparently this line. But I could not replicate the problem in the standard invenio implementation. Reasons: Following these lines: https://github.com/inveniosoftware/invenio-records-resources/blob/a9389e4cb179d6ab515282d37b60983bef8c06b0/invenio_records_resources/services/records/service.py#L140 this extra filter prevents requests that come with Debug: |
@@ -292,7 +292,6 @@ def search_user_requests( | |||
extra_filter=dsl.Q( | |||
"bool", | |||
must=[~dsl.Q("term", **{"status": "created"})], | |||
minimum_should_match=1, |
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.
Hey @edivalentinitu , thanks for the commit! I understand that the minimum_should_match=1
is redundant with the must
operation but I think don't understand how it solved the problem you described.... Could you please elaborate a bit more on what is the issue?
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.
Because our usecase does not generate search filters based on permissions, the only filter left was this extra filter and the search is somehow not evaluated correctly. There are no created requests, yet still the search returns no values. I am no opensearch expert and at first I just observed that if I remove this redundant param, the search for our usecase works ok, but I am open to suggestions if there could be another way of dealing with this.
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.
@slint do you see maybe any hidden potential problem?
Just from reading https://docs.opensearch.org/docs/latest/query-dsl/minimum-should-match/#default-minimum_should_match-value, since it doesn't document what happens when you pass |
❤️ Thank you for your contribution!
Description
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: