-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Cases] Removing escapeKuery calls in nodeBuilder #159815
[Cases] Removing escapeKuery calls in nodeBuilder #159815
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
@@ -589,163 +689,6 @@ describe('utils', () => { | |||
}); | |||
}); | |||
|
|||
describe('buildNestedFilter', () => { |
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.
buildNestedFilter
wasn't used anywhere so removing it and tests.
Object { | ||
"isQuoted": false, | ||
"type": "literal", | ||
"value": "awesome:()\\\\<>\\"*", |
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.
It's interesting that some of this is escaped, in the integration tests the slash still seem to work though 🤷♂️
)}: ${escapeKuery(filter)} }` | ||
) | ||
) | ||
filtersAsArray.map((filter) => nodeBuilder.is(`${type}.attributes.${field}`, filter)) |
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.
So now there is no filtering done on this external input?
edit: I think my question is more like, so why was this here to begin with? 😄
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.
It was added by accident haha. I added it when I saw we used it for the timerange filters but we didn't really need it.
@@ -189,6 +189,30 @@ export default ({ getService }: FtrProviderContext): void => { | |||
}); | |||
}); | |||
|
|||
it('filters cases using an assignee uid with a colon in it', async () => { |
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.
filter by assignees is only available to trial?
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.
Good point. It's the suggestion api that is not in the basic license. This test doesn't use that so I'll move it to the find_cases
file.
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.
Actually never mind you can't assign users unless you have a platinum license either. So I think this test should probably stay within the trial folder.
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.
Ok it is the assigning
operation though that needs the license. Find cases can always be done? I was wondering because I didn't find anything relative to it in the find
code.
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.
That's correct, assigning and using the suggestion API requires a platinum license. Filtering by assignee doesn't require a platinum license. The issue with moving this test to the basic directory is that I wouldn't be able to test retrieving the case with weird characters in the assignee uid because I wouldn't be able to assign a user with a weird uid.
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.
The issue with moving this test to the basic directory is that I wouldn't be able to test retrieving the case with weird characters in the assignee uid because I wouldn't be able to assign a user with a weird uid.
yy that's fine, you can leave it 🙌
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.
Tested and works.
My questions are more for me to understand the logic.
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
To update your PR or re-run it, just comment with: |
Fixes: #159741
This PR removes calls to
escapeKuery
where the result is passed directly tonodeBuilder
.nodeBuilder
expects the raw contents that should not be escaped. Escaping is only need when the result is passed tofromKueryExpression
orbuildNode
.Testing
Try creating a case with tags that have kql characters like:
\\():<>"*
and try filtering the cases list page for those tags. The case should be displayed in the table.Release Notes
Cases fixed an issue where the following special characters could not be included in the case tags:
\\():<>"*
because it resulted in a bug where the case would not be displayed in the cases table when filtered for those tags. These characters are now handled correctly and the cases will be shown in the table.