Reject Invalid/Unknown Filter Fields

[bryanculver]

Object list filtering in Nautobot (either in the UI, such as https://demo.nautobot.com/dcim/devices/?q=&site=ams01&foobar=baz, or in the REST API, such as https://demo.nautobot.com/api/dcim/devices/?foobar=baz&limit=5) currently ignores any query parameters that it doesn't understand.

If we want in 2.0 to be able to make breaking changes to our FilterSet fields (renaming fields, etc.) then this current behavior would be undesirable as it would result in existing saved queries silently returning unexpected data (e.g, if we split the has_primary_ip filter into separate has_primary_ip4 and has_primary_ip6 filters, a query for /dcim/devices?has_primary_ip=false will start returning all devices, instead of only those that lack an assigned primary IP).

The proposal here is to change our filtering behavior now so that unknown query parameters will begin to be rejected as an invalid query, which will make it so that on migration to 2.0, any queries that rely on no-longer-supported filter parameters will fail noisily and obviously instead of silently returning the wrong data.

[glennmatthews]

Quick proof of concept:

diff --git a/nautobot/core/views/generic.py b/nautobot/core/views/generic.py
index 533f707ec..b9558d659 100644
--- a/nautobot/core/views/generic.py
+++ b/nautobot/core/views/generic.py
@@ -191,7 +191,15 @@ class ObjectListView(ObjectPermissionRequiredMixin, View):
         content_type = ContentType.objects.get_for_model(model)
 
         if self.filterset:
-            self.queryset = self.filterset(request.GET, self.queryset).qs
+            filterset = self.filterset(request.GET, self.queryset)
+            if filterset.is_valid():
+                self.queryset = filterset.qs
+            else:
+                messages.error(
+                    request,
+                    mark_safe(f"Invalid filters were specified: {filterset.errors}"),
+                )
+                self.queryset = filterset.qs.none()
 
         # Check for export template rendering
         if request.GET.get("export"):
diff --git a/nautobot/dcim/filters.py b/nautobot/dcim/filters.py
index 69e5b1e70..94f120542 100644
--- a/nautobot/dcim/filters.py
+++ b/nautobot/dcim/filters.py
@@ -709,7 +709,7 @@ class DeviceFilterSet(NautobotFilterSet, TenancyFilterSet, LocalContextFilterSet
     device_bays = has_device_bays
     tag = TagFilter()
 
-    class Meta:
+    class Meta(BaseFilterSet.Meta):
         model = Device
         fields = [
             "id",
diff --git a/nautobot/utilities/filters.py b/nautobot/utilities/filters.py
index 95817e8e9..6bb25a033 100644
--- a/nautobot/utilities/filters.py
+++ b/nautobot/utilities/filters.py
@@ -461,6 +461,18 @@ class BaseFilterSet(django_filters.FilterSet):
         return filters
 
 
+    class Meta:
+        class StrictForm(forms.Form):
+            def is_valid(self):
+                result = super().is_valid()
+                for extra_key in set(self.data.keys()).difference(set(self.cleaned_data.keys())):
+                    self.add_error(None, f'Unknown filter field "{extra_key}"')
+                    result = False
+                return result
+
+        form = StrictForm
+
+
 class NameSlugSearchFilterSet(django_filters.FilterSet):
     """
     A base class for adding the search method to models which only expose the `name` and `slug` fields

[lampwins]

(moving PR comment here for potential discussion)

I like and agree with this, but is it not a breaking change? Consider I have a script which calls the API and has a typo in a filter field name. I get a response today and that might be perfectly fine.

Take for example (specifically one that I know about) in which a client is making an API call to populate a dropdown in another system. The query contains an invalid filter field but the client still gets a valid response because if the filter had worked, the query would have just been a subset of current result set. This is a safe behavior because we are only populating a drop down, not launching nukes; the only negative impact is the user has more choices to look at.

I will conceded this should probably be considered a bug, but I do feel it is a breaking change in behavior that has downstream impact. To that end, if no one else feels strongly about this, I am okay with this going in 1.4.

[glennmatthews]

Implemented for 1.4 by #1794.