-
Notifications
You must be signed in to change notification settings - Fork 759
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
OrderingFilter triggers a 500 error if a single comma is given as the input #1597
Comments
So the widget should be returning an empty list (rather than a list of what, empty strings? 🤔) Can you put together a test case demonstrating this issue and we can see what the tweak might be? Thanks. |
class F(FilterSet):
order_by = OrderingFilter(
fields=("title",),
)
class Meta:
model = Book
fields = ("title",)
qs = Book.objects.all()
f = F(data={"order_by": ","}, queryset=qs)
f.qs # this will raise a FieldError when it tries to do qs.order_by("")
# instead I would instead expect a validation error was raised in f.form.errors["order_by"] or that
# f.form.cleaned_data["order_by"] would be an empty list It does seem the widget is returning a list of empty strings so returning an empty list would fix it. |
Ok, fancy putting that as a test case in a pull request? |
Hi @munnsmunns — I've been playing with this. Can I ask that you show me a set up that triggers the error here? Adjusting the test case from your PR, like so… diff --git a/django_filters/filters.py b/django_filters/filters.py
index bbf950c..16ada96 100644
--- a/django_filters/filters.py
+++ b/django_filters/filters.py
@@ -719,7 +719,7 @@ class OrderingFilter(BaseCSVFilter, ChoiceFilter):
"""
- base_field_class = BaseOrderingField
+# base_field_class = BaseOrderingField
descending_fmt = _("%s (descending)")
def __init__(self, *args, **kwargs):
diff --git a/tests/test_filtering.py b/tests/test_filtering.py
index b048b56..11e4854 100644
--- a/tests/test_filtering.py
+++ b/tests/test_filtering.py
@@ -1979,17 +1979,26 @@ class OrderingFilterTests(TestCase):
def test_csv_input(self):
class F(FilterSet):
- o = OrderingFilter(widget=forms.Select, fields=("username",))
+ o = OrderingFilter(widget=forms.Select, fields=("username",),)
class Meta:
model = User
fields = ["username"]
qs = User.objects.all()
- f = F({"o": ","}, queryset=qs)
- f.qs
- value = f.form.cleaned_data["o"]
- self.assertEqual(value, [])
+ tests = [
+ {"o": ","},
+ QueryDict("order_by=%2c"),
+ QueryDict("order_by=,"),
+ ]
+ for data in tests:
+ with self.subTest(data=data):
+ f = F(data, queryset=qs)
+# self.assertIs(None, f.form["o"].field.widget.value_from_datadict(f.data, {}, 'o'))
+ self.assertIs(True, f.is_valid())
+ names = f.qs.values_list("username", flat=True)
+ self.assertEqual(list(names), ['alex', 'jacob', 'aaron', 'carl'])
+
class MiscFilterSetTests(TestCase):
def setUp(self): Both the You wrote:
But in this case I'm expecting to the Thanks! |
@carltongibson The tests with Here's the test I ran: def test_csv_input(self):
class F(FilterSet):
order_by = OrderingFilter(widget=forms.Select, fields=("username",),)
class Meta:
model = User
fields = ["username"]
qs = User.objects.all()
tests = [
QueryDict("order_by=%2c"),
QueryDict("order_by=,"),
]
for data in tests:
with self.subTest(data=data):
f = F(data, queryset=qs)
# self.assertIs(None, f.form["o"].field.widget.value_from_datadict(f.data, {}, 'o'))
self.assertIs(True, f.is_valid())
names = f.qs.values_list("username", flat=True)
self.assertEqual(list(names), ['alex', 'jacob', 'aaron', 'carl']) |
@carltongibson Sorry I haven't responded in a while, @scott-8 's post is perfect for triggering the error I was talking about |
Great. Thanks. I'll take another look. |
One other test case that might be useful to add would be a trailing comma to a filter. Adding this query to the |
A trailing comma would cause a crash trying to map an empty value to a field name. Ensure sub-values are filtered for EMPTY_VALUES in addition to the main filter data. Closes #1597. Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es>
A trailing comma would cause a crash trying to map an empty value to a field name. Ensure sub-values are filtered for EMPTY_VALUES in addition to the main filter data. Closes #1597. Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es>
A trailing comma would cause a crash trying to map an empty value to a field name. Ensure sub-values are filtered for EMPTY_VALUES in addition to the main filter data. Closes #1597. Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es>
A trailing comma would cause a crash trying to map an empty value to a field name. Ensure sub-values are filtered for EMPTY_VALUES in addition to the main filter data. Closes #1597. Co-authored-by: munnsmunns <mmunns16@gmail.com>
When
','
is given as the value for an OrderingFilter in a filterset form, a FieldError is thrown:Cannot resolve keyword '' into field. Choices are: . . .
It appears that bots are hitting a search page on our site with
?order_by=%2c
in the url which triggers this behaviorI would expect the input to be rejected without throwing an uncaught exception, which is the behavior when an invalid field name is given as the input for the OrderingFilter. It appears it's being interpreted as a CSV input instead.
Our use of the OrderingFilter class:
The text was updated successfully, but these errors were encountered: