-
Notifications
You must be signed in to change notification settings - Fork 10
fix dissector curly braces problem #921
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
base: main
Are you sure you want to change the base?
Conversation
this handles curly braces one char before end of section. I had to remove the feature to handle curly braces in field names. But ensured that curly braces in field content still works. I think the tests for handling special fields in field names are more sience and fiction than reality.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #921 +/- ##
==========================================
- Coverage 96.22% 96.13% -0.09%
==========================================
Files 213 213
Lines 13870 14050 +180
==========================================
+ Hits 13346 13507 +161
- Misses 524 543 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| STRIP_CHAR = r"(-\((?P<strip>.)\))?" | ||
| SEPERATOR = r"(\((?P<separator>\\\)|[^)]+)\))?" | ||
| TARGET_FIELD = r"(?P<target_field>[^\/\|-]*)" | ||
| TARGET_FIELD = r"(?P<target_field>[^\/\|\}-]*)" |
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.
@ekneg54 Maybe I have not properly understood the implementation, but shouldn't TARGET_FIELD and VALID_TARGET_FIELD be semantically equivalent?
Maybe not exactly, but something like this?:
| TARGET_FIELD = r"(?P<target_field>[^\/\|\}-]*)" | |
| TARGET_FIELD = rf"(?P<target_field>[^\/\|\}}-]{VALID_TARGET_FIELD}*)" |
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.
from semantics side you should be right. but from code site this doesn't work. And to be honest I have no clue why this ist written this way. I only came here to fix this bug. I have no time to dig deep into the implementation. Sorry for that.
The only thing I remember is that I had to design the validation regex a little bit broader. It is used in rule validation via attrs. But I guess it is worth it to have a look into. But not in conjunction with this bugfix.
logprep/processor/dissector/rule.py
Outdated
|
|
||
| START = r"%\{" | ||
| END = r"\}" | ||
| VALID_TARGET_FIELD = r"[^\}\%\{\}\+\/\|]*" |
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.
There is a duplicate \}
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.
fixed
this handles curly braces one char before end of section.
feel free to have a look at the pipeline output of the first commit to understand the problem.
I had to remove the feature to handle curly braces in field names.
But ensured that curly braces in field content still works.
I think the tests for handling special fields in field names are more
sience and fiction than reality.