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
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
QuerysetFilterFilterWrapper)FilterBlobSerializerclass for DRFCurrently the filtering plugin system has too much to do
FilterSerializer.FilterSerializeris tied to theFiltermodel and therefore it makes no sense to override that. We're not changing theFiltermodel.FilterSerializershould dynamically loadFilterBlobSerializerinsteadFallbackFilterWrapper,ComplexFilterWrapper,ComplexFallbackFilterWrapper. These classes should either dynamically load a FilterWrapper (or take one as a constructor argument, which I think would be nicest)INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONSandSOURCE_LOCKED_INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONS. Filtering based on query parameters does not change when customizing filterblob filteringvalidate_jsonfilterto be usedFilterConfig.readyI propose the following changes:
FilterSerializer,FallbackFilterWrapper,ComplexFilterWrapper,ComplexFallbackFilterWrapper,INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONS,SOURCE_LOCKED_INCIDENT_OPENAPI_PARAMETER_DESCRIPTIONSinstead of through the pluginFilterSerializerdynamically loadsFilterBlobSerializerfrom the pluginQuerySetFilterloads the plugin and uses it for its (filtered_incidents) method (composition over inheritance)incident_fits(filterblob, incident, event=None)). Alsois_empty(filterblob)method. Separateevent_fits()method is dropped.incident_fitsmethodvalidate_jsonfilterreadsFilterBlobSerializerfrom the plugin and uses itsis_valid()method. I think we can also inline most ofvalidate_jsonfilterintofallback_filter_checkFor Providing OpenAPI integration for FilterBlobSerializer, I have currently set up a
drf_spectacular.extensions.OpenApiSerializerExtensionwhich works well enoughSome other (related) refactors I could imagine:
IncidentFiltercould be an almost vanillaFilterSetif 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 toIncidentViewSet.filter_backends. Which would be a nicer separation of concernsQuerySetFilter.incidents_by_notificationprofiledoes many queries to the db. I think we can improve this and make a single query instead with clever use ofQ()andWHERE IN (SELECT ...)queriesFallbackFilterWrapper,ComplexFilterWrapper,ComplexFallbackFilterWrapperinto a single class, since this only used in a single combination (and at a a single place)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