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

Allow the usage of ' ' and '/' in field names #20142

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

janheise
Copy link
Contributor

@janheise janheise commented Aug 9, 2024

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@janheise janheise requested a review from kroepke August 13, 2024 14:45
@janheise janheise requested review from systemboogie and a team November 6, 2024 11:22
@janheise janheise marked this pull request as ready for review November 6, 2024 11:22
@janheise janheise removed the request for review from kroepke November 6, 2024 11:26
@kroepke
Copy link
Member

kroepke commented Nov 6, 2024

Before merging this, we need to ensure that all places that deal with field names properly escape them, especially URL construction and notification templates.

Copy link
Member

@kroepke kroepke left a 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.

@janheise janheise force-pushed the fix/allow-whitespace-and-slash-in-field-names branch from f1b5edb to 2b69867 Compare November 6, 2024 13:35
@janheise janheise force-pushed the fix/allow-whitespace-and-slash-in-field-names branch from 6dd636e to c5b88ab Compare November 6, 2024 13:37
@systemboogie
Copy link

systemboogie commented Nov 15, 2024

@janheise I noticed a problem with search query auto-completion. When searching for a field name with a slash (e.g. abc/slash), auto-completion correctly suggests the field, but does not escape the slash in the suggestion...

Screenshot 2024-11-15 at 15 30 06

... which then leads to an error when performing the search:

Screenshot 2024-11-15 at 15 41 26

The same happens when searching for a field name that contains a space character (e.g. abc space)

@systemboogie
Copy link

systemboogie commented Nov 15, 2024

@janheise Another observation related to the _exists_ operator.

Works fine for slash, e.g. _exists_:abc\/slash.

Space in a field name needs to be escaped as well (e.g. _exists_:abc\ space) and the search yields the correct results. However, the UI shows warnings.

Screenshot 2024-11-15 at 16 25 51

@systemboogie
Copy link

systemboogie commented Nov 21, 2024

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?

  • Search page > Query input field
  • Search page > Search result list (message table) > Add to query
  • Search page > Search filter > Query input field
  • Enterprise > My search filters

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:

  • Search page > Create an event definition from a value > Leads to the event definition config with query input field populated
  • Alert page > Replay search

While testing alerts, I stumbled across an inconsistency. Consider one key-value pair abc/slash:"with slash" and another one abc/slash/2:"with slash"

  • An event definition that looks for abc/slash:"with slash" does not fire an alert. Pretty expected due to the un-escaped slash
  • An event definition that looks for abc\/slash:"with slash" does fire an alert. Also expected
  • An event definition that looks for abc/slash/2:"with slash does fire an alert as well, despite the un-escaped slashes. And the alert correctly lists only messages with the abc/slash/2 field
  • That made me check the behavior of the search page... and the displayed results are different there. A search query like abc/slash/2:"with slash" does not show a validation error (only a warning), so a user is able to submit the query. However, the search does not only show messages that contain abc/slash/2, but also unexpected ones, e.g. abc/slash and also /. Seems like the parser does not account for more than one slash in a field name or we use different search endpoints in different places or maybe even both applies

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants