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

[RAM] Fix filtering tags by special characters #135673

Conversation

JiaweiWu
Copy link
Contributor

@JiaweiWu JiaweiWu commented Jul 4, 2022

Summary

Resolves: #135599

Fixes the inability to filter on tags with special characters

Checklist

@JiaweiWu JiaweiWu added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Alerting/RulesManagement Issues related to the Rules Management UX ci:deploy-cloud v8.4.0 v8.3.2 labels Jul 4, 2022
@JiaweiWu JiaweiWu marked this pull request as ready for review July 4, 2022 23:37
@JiaweiWu JiaweiWu requested a review from a team as a code owner July 4, 2022 23:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

filters.push(`alert.attributes.tags:(${tagsFilter.join(' or ')})`);
filters.push(
`alert.attributes.tags:(${tagsFilter
.map((tag) => tag.replace(/([\)\(\<\>\}\{\"\:\\])/gm, '\\$&'))
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it is relevant to the PR but you can use escapeKuery from @kbn/es-query

Copy link
Contributor Author

@JiaweiWu JiaweiWu Jul 5, 2022

Choose a reason for hiding this comment

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

I tried to use escapeKuery but it seems like that does not escape {}. Looking at the source code, it does seem to support that assumption.

function escapeSpecialCharacters(str: string) {
  return str.replace(/[\\():<>"*]/g, '\\$&'); // $& means the whole matched string
}

Also I noticed this regex pattern is used in security solutions: x-pack/plugins/security_solution/public/management/common/utils.ts. But yes I do think it would be ideal if we can just use the same regex across the code base. So I wouldn't mind just using escapeKuery

@XavierM
Copy link
Contributor

XavierM commented Jul 11, 2022

@elasticmachine merge upstream

@XavierM XavierM requested a review from jcger July 11, 2022 15:09
Copy link
Contributor

@jcger jcger left a comment

Choose a reason for hiding this comment

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

👏

@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 89.6KB 89.7KB +55.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@JiaweiWu
Copy link
Contributor Author

fixed by this pr: #136312

@JiaweiWu JiaweiWu closed this Jul 18, 2022
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged bug Fixes for quality problems that affect the customer experience ci:cloud-deploy Create or update a Cloud deployment Feature:Alerting/RulesManagement Issues related to the Rules Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.3.2 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAM] Cannot filter on rules using tags with special chars in them
8 participants