Skip to content

Streamline use of filtering throughout Argus and simplify filtering customization #855

Description

@elfjes

I've had a closer look at how Argus does filtering, and I think I see some opportunities, especially wrt to customizing filtering behaviour using the filterblob (Filter.filter)

The main use would be to reduce the api surface of the new filter plugin system, the api would focus on

  • "given a filterblob, create or update a queryset for Incidents that match this filter", currently incorporated in QuerysetFilter
  • "given a filterblob, does this incident (and/or event) match this filter" (currently FilterWrapper)
  • provide FilterBlobSerializer class for DRF
  • (optionally): provide OpenAPI integration for FilterBlobSerializer

Currently the filtering plugin system has too much to do

  • Have to provide FilterSerializer. FilterSerializer is tied to the Filter model and therefore it makes no sense to override that. We're not changing the Filter model. FilterSerializer should dynamically load FilterBlobSerializer instead
  • Have to provide FallbackFilterWrapper, ComplexFilterWrapper, ComplexFallbackFilterWrapper. These classes should either dynamically load a FilterWrapper (or take one as a constructor argument, which I think would be nicest)
  • Have to provide INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONS and SOURCE_LOCKED_INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONS. Filtering based on query parameters does not change when customizing filterblob filtering
  • Have to provide validate_jsonfilter to be used FilterConfig.ready

I propose the following changes:

  • Use FilterSerializer, FallbackFilterWrapper, ComplexFilterWrapper, ComplexFallbackFilterWrapper, INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONS,SOURCE_LOCKED_INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONS instead of through the plugin
  • FilterSerializer dynamically loads FilterBlobSerializer from the plugin
  • QuerySetFilter loads the plugin and uses it for its (filtered_incidents) method (composition over inheritance)
  • FilterWrapper functionality is accessed through the plugin (incident_fits(filterblob, incident, event=None)). Also is_empty(filterblob) method. Separate event_fits() method is dropped.
  • ComplexFallbackFilterWrapper takes in the plugin for its incident_fits method
  • validate_jsonfilter reads FilterBlobSerializer from the plugin and uses its is_valid() method. I think we can also inline most of validate_jsonfilter into fallback_filter_check

For Providing OpenAPI integration for FilterBlobSerializer, I have currently set up a drf_spectacular.extensions.OpenApiSerializerExtension which works well enough

Some other (related) refactors I could imagine:

  • Filter plugin as a class instead of a module. This makes it easier to check typing and autocomplete
  • IncidentFilter could be an almost vanilla FilterSet if it wasn't for the incident_pk and notificationprofile_pk parameters for which it has to call QuerySetFilter. We could take that logic out and let QuerySetFilter implement DRF BaseFilterBackend (.filter_queryset(self, request, queryset, view)) so we can add that to IncidentViewSet.filter_backends. Which would be a nicer separation of concerns
  • QuerySetFilter.incidents_by_notificationprofile does many queries to the db. I think we can improve this and make a single query instead with clever use of Q() and WHERE IN (SELECT ...) queries
  • Merge FallbackFilterWrapper, ComplexFilterWrapper, ComplexFallbackFilterWrapper into a single class, since this only used in a single combination (and at a a single place)
  • Remove unused code from Filter and NotificationProfile that is now in FilterWrapper

What do you guys think of these proposals? I could start with the lower hanging fruit while we flesh out some of the details for the more complex things

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    METAI contain multitudes

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions