Skip to content

Don't allow field extractors to match field name suffixes #1447

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 18, 2021
Merged

Don't allow field extractors to match field name suffixes #1447

merged 2 commits into from
Mar 18, 2021

Conversation

lava
Copy link
Member

@lava lava commented Mar 15, 2021 •
edited by tobim
Loading

📔 Description

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

@lava lava force-pushed the story/ch22710/type-resolution-regex branch from 8aed1ef to 216aa4a Compare March 15, 2021 12:17
@lava lava marked this pull request as ready for review March 15, 2021 12:18
@lava lava force-pushed the story/ch22710/type-resolution-regex branch from 216aa4a to cf8eb82 Compare March 15, 2021 16:17
@lava
Copy link
Member Author

lava commented Mar 16, 2021

After thinking about this PR a bit, I think this might break queries like zeek.* == "xxx". Since there's no unit test for these and they would have only very rarely worked previously, I'm not sure if this was supposed to work. But as a user, it seems like something I'd intuitively expect to work, if wildcards are supported at all.

@tobim
Copy link
Member

tobim commented Mar 16, 2021

After thinking about this PR a bit, I think this might break queries like zeek.* == "xxx". Since there's no unit test for these and they would have only very rarely worked previously, I'm not sure if this was supposed to work. But as a user, it seems like something I'd intuitively expect to work, if wildcards are supported at all.

The bug that this PR fixes prevented those queries to work as well, so I don't think there is any harm done.

@mavam
Copy link
Member

mavam commented Mar 16, 2021

But as a user, it seems like something I'd intuitively expect to work, if wildcards are supported at all.

Yeah, I agree. let's treat this as a separate feature that we don't support yet.

If we stick with component-based suffix matching, we're good for this PR.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Mar 16, 2021
tobim
tobim requested changes Mar 17, 2021
View reviewed changes
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

This is still missing a changelog entry, otherwise it is good to go.

tobim
tobim reviewed Mar 17, 2021
View reviewed changes
lava added 2 commits March 18, 2021 09:58
@lava @tobim
Require full fields for suffix matching
d5fc8f1 
Don't allow queries like `oo.x` to match a field `foo.x` anymore,
since that can lead to unintuitive results for the user. Instead,
require that full field names are matched. (So in the previous
example, only `foo.x` and `x` would be valid suffix matches)
@lava @tobim
Add CHANGELOG entry
4677d3d
@tobim tobim force-pushed the story/ch22710/type-resolution-regex branch from 049ea9a to 4677d3d Compare March 18, 2021 09:00
@tobim tobim enabled auto-merge March 18, 2021 09:02
@tobim tobim merged commit ad8d850 into master Mar 18, 2021
@tobim tobim deleted the story/ch22710/type-resolution-regex branch March 18, 2021 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiklohmann dominiklohmann dominiklohmann left review comments

@tobim tobim tobim approved these changes

Assignees

@tobim tobim

Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lava @tobim @mavam @dominiklohmann