-
Notifications
You must be signed in to change notification settings - Fork 1.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
Allow the usage of ' ' and '/' in field names #20142
base: master
Are you sure you want to change the base?
Conversation
...ver/src/test/java/org/graylog/plugins/pipelineprocessor/functions/FunctionsSnippetsTest.java
Outdated
Show resolved
Hide resolved
...ver/src/test/java/org/graylog/plugins/pipelineprocessor/functions/FunctionsSnippetsTest.java
Outdated
Show resolved
Hide resolved
...ver/src/test/java/org/graylog/plugins/pipelineprocessor/functions/FunctionsSnippetsTest.java
Outdated
Show resolved
Hide resolved
Before merging this, we need to ensure that all places that deal with field names properly escape them, especially URL construction and notification templates. |
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.
Please address the exhaustive testing before merging this.
6dd636e
to
c5b88ab
Compare
@janheise I noticed a problem with search query auto-completion. When searching for a field name with a slash (e.g. ... which then leads to an error when performing the search: The same happens when searching for a field name that contains a space character (e.g. |
@janheise Another observation related to the Works fine for slash, e.g. Space in a field name needs to be escaped as well (e.g. |
Some more findings around auto-completion: All query input fields with auto-completion suggest a field name with slash or space correctly, but in the suggested field names, slash and space characters are un-escaped. Hopefully this is a thing that can be fixed in a central place?
Then I found an issue in areas where we pre-populate a query input field. Slash and space in field names are un-escaped there as well. We should escape those chars in the generated query. Some areas where I observed that until now, I guess there are more:
While testing alerts, I stumbled across an inconsistency. Consider one key-value pair
When replaying a search from a fired alert, the slash and space characters are un-escaped in the search query input field, but nonetheless the search results in the widgets below are correct |
In the Events & Alerts section, we saw that a slash in the field name must be escaped in order to make the event/alert work. In the Pipeline Rules section, this is the other way around - i.e. when escaping a slash in the field name input, the rule does not catch the respective message:
|
Event definitions, custom fields: Dynamically reading the value from a field does not work when the original field name contains a space character. Given you add a custom field to an event and you want it to dynamically assign the value from another (original) field. That works fine if the original field’s name contains a slash character, but not if it contains a space character. None of the below-mentioned JMTE template strings works. The resulting
If the original field contains a slash, it works - regardless of whether the template is |
3ca4b6e
to
bdb54df
Compare
Another thing related to my previous comment, which is dealing with JMTE strings: In Notification JMTE templates, it is not possible to print the value of a field name that contains a space character. Field names with a slash work fine. Given you have a field name Tried several variations:
Error message example:
|
=> have been addressed |
IMHO, this is as it is because it's two different concepts at work here. Working in pipelines is matching on fields in a message etc. and not running queries. |
…of other code that is not part of this PR
Description
This PR adds support for
and
/
in field names. This was forbidden in earlier versions due to restrictions in ES/lucene.TODO: improve this description & changelog
Motivation and Context
The definition of Whitespace: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Character.html#isWhitespace(char)
for reference, see https://github.com/Graylog2/graylog-plugin-enterprise/issues/8169
How Has This Been Tested?
The unit tests for Messages have been modified to include whitespace & / in tests.
Screenshots (if appropriate):
Types of changes
Checklist: