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

[ignoreFilterIfFieldNotInIndex] Parse query string filters to determine if fields match an index #6126

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

neodescis
Copy link
Contributor

@neodescis neodescis commented Mar 12, 2024

Description

For "query_string" filters, parse the query string as lucene to determine if the query's fields matches any given index. This is pertinent when the ignoreFilterIfFieldNotInIndex advanced setting is enabled, as filters are only applied to applicable indexes.

Also, for filter bar items, the "Warning" when a filter does not match any indexes has been updated to use the same logic.

Issues Resolved

closes #6036

Screenshot

Changelog

  • feat: Parse query string filters to determine if fields match an index when ignoreFilterIfFieldNotInIndex is enabled

Testing the changes

  1. Enable ignoreFilterIfFieldNotInIndex
  2. Go to a dashboard
  3. Click Add Filter
  4. Click Edit as Query DSL
  5. Enter a "query string" filter, such as: { "query": { "query_string": { "query": "foo: bar" } } }
  6. Click Save
  7. Notice that the filter should now be applied to matching indexes, whereas before it was not
  8. Also notice that if the filter does not have an index pattern specifically associated with it, "Warning" will not appear in the filter item's label, whereas before it would

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@neodescis neodescis changed the title Parse query string filter to determine fields ignoreFilterIfFieldNotInIndex: Parse query string filters to determine if fields match an index Mar 12, 2024
@neodescis neodescis changed the title ignoreFilterIfFieldNotInIndex: Parse query string filters to determine if fields match an index [ignoreFilterIfFieldNotInIndex] Parse query string filters to determine if fields match an index Mar 12, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 61.90476% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 67.15%. Comparing base (d2d410b) to head (3cfde8d).
Report is 8 commits behind head on main.

❗ Current head 3cfde8d differs from pull request most recent head 403e0df. Consider uploading reports for the commit 403e0df to get more accurate results

Files Patch % Lines
...rch_query/opensearch_query/filter_matches_index.ts 61.90% 2 Missing and 6 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6126       +/-   ##
===========================================
+ Coverage   32.93%   67.15%   +34.22%     
===========================================
  Files        2260     3326     +1066     
  Lines       45769    64421    +18652     
  Branches     7200    10368     +3168     
===========================================
+ Hits        15075    43264    +28189     
+ Misses      29984    18621    -11363     
- Partials      710     2536     +1826     
Flag Coverage Δ
Linux_1 31.72% <4.76%> (-1.22%) ⬇️
Linux_2 55.19% <4.76%> (?)
Linux_3 44.76% <61.90%> (?)
Linux_4 35.06% <4.76%> (?)
Windows_1 31.74% <4.76%> (?)
Windows_2 55.16% <4.76%> (?)
Windows_3 44.76% <61.90%> (?)
Windows_4 35.06% <4.76%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kavilla
Copy link
Member

kavilla commented Apr 8, 2024

Hello @neodescis, apologies about the delay. Going to go through this. I understand the issue, but could you provide some quick insight as that setting has caused some trouble in the past. Do we see any issue with bringing up that helper function and using it where you have it. Any impact to performance that is noticeable?

Thank you!

@neodescis
Copy link
Contributor Author

neodescis commented Apr 9, 2024

Hello @neodescis, apologies about the delay. Going to go through this. I understand the issue, but could you provide some quick insight as that setting has caused some trouble in the past. Do we see any issue with bringing up that helper function and using it where you have it. Any impact to performance that is noticeable?

Thank you!

Do you mean using filterMatchesIndex in FilterItem? I would not expect it to have much of a performance impact. We've been using it like this for a few years. Aside from the parsing I added for "query_string" filters, the logic is largely the same anyway. Conceptually, it should be using the same logic: the goal is to warn users of filters that are not applicable, and filterMatchesIndex is what determines if they are.

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Hello @neodescis,

Sorry about the delay. Played with it for a while. Works great! The filter bar warning change makes sense to me I believe.

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

@kavilla kavilla added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Apr 17, 2024
@kavilla kavilla self-requested a review April 17, 2024 17:59
@kavilla
Copy link
Member

kavilla commented Apr 17, 2024

@neodescis, I think perhaps updates to the PR is disabled for this PR?

There's some workflows that are failing and I believe the branch just needs to be updated. Could you enable it or just rebase from main?

Otherwise, all good for approval upon passing workflow. Let me know if you need any assistance. Thank you so much!

@kavilla kavilla removed the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Apr 17, 2024
@neodescis neodescis requested a review from xinruiba as a code owner April 18, 2024 19:01
@neodescis
Copy link
Contributor Author

neodescis commented Apr 18, 2024

Hmm, it seems that GitHub pull requests from another "organization" cannot be set to allow edits by you. That is unfortunate! At any rate, I did just update the branch, so we'll see if that gets the workflow to pass. It seems you have to approve running them every time though, which is also unfortunate. Do you need a CHANGELOG entry, or anything else? I was holding off on that one just because merging it is never fun.

@kavilla
Copy link
Member

kavilla commented Apr 18, 2024

Hmm, it seems that GitHub pull requests from another "organization" cannot be set to allow edits by you. That is unfortunate! At any rate, I did just update the branch, so we'll see if that gets the workflow to pass. It seems you have to approve running them every time though, which is also unfortunate.

That is good to know. I was curious why I couldn't update the branch via GitHub. Thank you for updating it! Re-triggered the workflows.

Do you need a CHANGELOG entry, or anything else? I was holding off on that one just because merging it is never fun.

I have updated the PR with the template. So after updating the branch you should be good and don't need to change any file. If you don't mind checking the Changelog section I have added to the PR to make sure it's accurate that would be great.

@neodescis
Copy link
Contributor Author

Workflow passed. Changelog entry in the PR description looks good to me. Thanks!

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Awesome thank you. Going to try to get this into 2.14.

Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

LGTM

@ananzh ananzh merged commit 24177b2 into opensearch-project:main Apr 23, 2024
65 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-6126-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 24177b248c74ecf078b592b6ac20cf1698768ec6
# Push it to GitHub
git push --set-upstream origin backport/backport-6126-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6126-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.14 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.14 2.14
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.14
# Create a new branch
git switch --create backport/backport-6126-to-2.14
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 24177b248c74ecf078b592b6ac20cf1698768ec6
# Push it to GitHub
git push --set-upstream origin backport/backport-6126-to-2.14
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.14

Then, create a pull request where the base branch is 2.14 and the compare/head branch is backport/backport-6126-to-2.14.

LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request May 7, 2024
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request May 7, 2024
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request May 7, 2024
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request May 7, 2024
ananzh pushed a commit that referenced this pull request May 7, 2024
Signed-off-by: Nick Steinbaugh <[email protected]>
(cherry picked from commit 24177b2)

Co-authored-by: neodescis <[email protected]>
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request May 7, 2024
ashwin-pc pushed a commit that referenced this pull request May 7, 2024
Signed-off-by: Nick Steinbaugh <[email protected]>
(cherry picked from commit 24177b2)

Co-authored-by: neodescis <[email protected]>
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve ignoreFilterIfFieldNotInIndex functionality for multi-field query string filters
4 participants