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

[backend] new logic engine for resolving FilterGroups for event streams (#4536) #4847

Merged
merged 28 commits into from Nov 14, 2023

Conversation

labo-flg
Copy link
Member

@labo-flg labo-flg commented Nov 3, 2023

Proposed changes

This code is completely independent and is only used by unit tests for now.
It implements our new filtering logic with nested, recursive Filter groups, in the case of stream events.

It's closely related to @Archidoit's work and will require some integration, so things might change on the edges.

I strongly suggest to read the code in parallel with unit tests, and to start "from the bottom"

  1. boolean-logic-engine.ts
  2. stix-testers.ts
  3. stix-filtering.ts

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

This implementation separates the responsibilities properly:

  • New boolean logic engine

    • takes a FilterGroup and some data, to test the filter against the data
    • Is independent of the actual model of the data
    • provides functions to test a Filter against dates, strings, booleans, numbers
  • A set of individual testers functions, one for each filter key currently possible

    • knows the stix model and how to extract the data
    • has no filtering logic, only "stix logic"
  • final function isStixMatchFilterGroups

    • meant to replace isStixMatchFilter, the one used for streams, triggers and playbooks when we want to check a filter against a stix object
    • only handles the context (user, filter adaptation..) but no filtering logic

@labo-flg labo-flg added the filigran team use to identify PR from the Filigran team label Nov 3, 2023
/**
* Gives the right tester function according to the filter key.
* If the key is not handled, returns a function that always return false.
* TODO: make it dependent on the schema.
Copy link
Member Author

Choose a reason for hiding this comment

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

The whole code in this file could be replaced by a more generic approach:

  • update the schema to add the stix paths, value type and multiplicity
  • extract the data thanks to the schema, according to the attribute key

And we would remove this ugly part of the code.
Now, it's far easier to say than done, so this version is MVP : it is based on the current filters available in the UI and handle each case specifically. We expect no regression.

In a next version we could work on this complex task involving schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

for the record, I think it could be like the stix converter or the representative inside an entity in the schema.

A field stixFilter in AttributeDefinition for the the individual tester logic ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll tackle this later, once a first working version is out. That's the priority.

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (d1abb33) 0.00% compared to head (42e9afa) 87.27%.

Files Patch % Lines
...graphql/src/utils/stix-filtering/stix-filtering.ts 72.00% 14 Missing ⚠️
...i-graphql/src/utils/stix-filtering/stix-testers.ts 92.98% 8 Missing ⚠️
...l/src/utils/stix-filtering/boolean-logic-engine.ts 89.28% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4847       +/-   ##
===========================================
+ Coverage        0   87.27%   +87.27%     
===========================================
  Files           0        3        +3     
  Lines           0      220      +220     
  Branches        0       55       +55     
===========================================
+ Hits            0      192      +192     
- Misses          0       28       +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@labo-flg labo-flg changed the title New logic engine for resolving FilterGroups for event streams [backend] new logic engine for resolving FilterGroups for event streams (#4536) Nov 8, 2023
@labo-flg
Copy link
Member Author

labo-flg commented Nov 8, 2023

all comments addressed in last commit.

@labo-flg
Copy link
Member Author

Last commits address most of the changes requested by @richard-julien after our pair review.

  • resolution map is built from cache in nominal case, but an intermediate function allows to mock this mapping in unit tests
  • trim string before comparison
  • parse numbers as float instead of just integers
  • validate filters and throw errors if not compatible (multi key, unrecognized key)

The following changes will be done AFTER merge with Cathia's feature branch

  • move the filter key validation from the isStixMatchFilterGroup function to the creation/update of saved filters
  • all plumbing works to call the new code, check the filter key renamings, etc.

@labo-flg labo-flg merged commit f06d4e6 into master Nov 14, 2023
8 checks passed
@labo-flg labo-flg deleted the issue/4536-stream-filters branch November 14, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants