Repository: wagtail/wagtail
Pull request: Wagtail API pages endpoint to respond with a 400 error (instead of 500 error) when entering an invalid value to a filter
Hello, I think Wagtail API pages endpoint responds with a 500 error when entering an invalid value to a filter because no check is made on models.ForeignKey [here](https://github.com/wagtail/wagtail/blob/42ddd8ad554f02e9761f1a36f45e162d3edfa7c1/wagtail/api/v2/filters.py#L37).

I added models.ForeignKey and it seems to fix it.


* Do the tests still pass? (https://docs.wagtail.io/en/latest/contributing/developing.html#testing) Yes
* Does the code comply with the style guide? (Run `make lint` from the Wagtail root) Yes
* For Python changes: Have you added tests to cover the new/fixed behaviour? Yes
    

Fixes #7011 

Problem statement:
Wagtail API pages endpoint responds with a 500 error when entering an invalid value to a filter
### Issue Summary

When querying the Wagtail API and using filters, invalid filter values cause the server to respond with a 500 error.

Given a `BlogPage` with an `author` ForeignKey.

e.g. `/api/v2/pages/?type=blog.BlogPage&author=a`

This returns a 500 error.

### Steps to Reproduce

1. Create any page type that has a foreign key
2. Query that page type using the Wagtail API, and pass an invalid value to a filter (e.g. a letter to a foreign key field)
3. Server Error 500

Any other relevant information. For example, why do you consider this a bug and what did you expect to happen instead?

* This is not a server error. It's a client error.
* We already have this error message for invalid IDs:

```
HTTP 400 Bad Request
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "message": "field filter error. 'a' is not a valid value for id (invalid literal for int() with base 10: 'a')"
}
```

* A 400 error is expected.


### Technical details

* Python version: 3.8.6.
* Django version: 3.1.
* Wagtail version: 2.12.
* Browser version: Firefox 87

Hints from issue discussion:

Commit message: Remove extra whitespaces to comply with styleguide

Patch to review:
diff --git a/wagtail/api/v2/filters.py b/wagtail/api/v2/filters.py
index 996e486943dc..d4b361ec35bb 100644
--- a/wagtail/api/v2/filters.py
+++ b/wagtail/api/v2/filters.py
@@ -34,7 +34,7 @@ def filter_queryset(self, request, queryset, view):
                 try:
                     if isinstance(field, (models.BooleanField, models.NullBooleanField)):
                         value = parse_boolean(value)
-                    elif isinstance(field, (models.IntegerField, models.AutoField)):
+                    elif isinstance(field, (models.IntegerField, models.ForeignKey, models.AutoField)):
                         value = int(value)
                 except ValueError as e:
                     raise BadRequestError("field filter error. '%s' is not a valid value for %s (%s)" % (
diff --git a/wagtail/api/v2/tests/test_pages.py b/wagtail/api/v2/tests/test_pages.py
index 4abc20ffda7b..1d4fd324bd84 100644
--- a/wagtail/api/v2/tests/test_pages.py
+++ b/wagtail/api/v2/tests/test_pages.py
@@ -454,6 +454,13 @@ def test_filtering_on_id(self):
         page_id_list = self.get_page_id_list(content)
         self.assertEqual(page_id_list, [16])
 
+    def test_filtering_on_foreign_key(self):
+        response = self.get_response(type="demosite.contactpage", feed_image=7)
+        content = json.loads(response.content.decode('UTF-8'))
+
+        page_id_list = self.get_page_id_list(content)
+        self.assertEqual(page_id_list, [12])
+
     def test_filtering_on_boolean(self):
         response = self.get_response(show_in_menus='false')
         content = json.loads(response.content.decode('UTF-8'))
@@ -489,13 +496,20 @@ def test_filtering_unknown_field_gives_error(self):
         self.assertEqual(response.status_code, 400)
         self.assertEqual(content, {'message': "query parameter is not an operation or a recognised field: not_a_field"})
 
-    def test_filtering_int_validation(self):
+    def test_filtering_id_int_validation(self):
         response = self.get_response(id='abc')
         content = json.loads(response.content.decode('UTF-8'))
 
         self.assertEqual(response.status_code, 400)
         self.assertEqual(content, {'message': "field filter error. 'abc' is not a valid value for id (invalid literal for int() with base 10: 'abc')"})
 
+    def test_filtering_foreign_key_int_validation(self):
+        response = self.get_response(type="demosite.contactpage", feed_image='abc')
+        content = json.loads(response.content.decode('UTF-8'))
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(content, {'message': "field filter error. 'abc' is not a valid value for feed_image (invalid literal for int() with base 10: 'abc')"})
+
     def test_filtering_boolean_validation(self):
         response = self.get_response(show_in_menus='abc')
         content = json.loads(response.content.decode('UTF-8'))


Code review comments:
diff_hunk: @@ -34,7 +34,7 @@ def filter_queryset(self, request, queryset, view):
                 try:
                     if isinstance(field, (models.BooleanField, models.NullBooleanField)):
                         value = parse_boolean(value)
-                    elif isinstance(field, (models.IntegerField, models.AutoField)):
+                    elif isinstance(field, (models.IntegerField, models.ForeignKey, models.AutoField)):
path: wagtail/api/v2/filters.py
text: @user1:
``ForeignKey``'s could sometimes be text or a UUID internally, I think this validation should account for that.

Could you try using `field.get_prep_value(value)` here? this should raise a `ValueError` if the value is invalid

@author:
I thought about it but I didn't know how to handle it.
I made the changes requested.
Is that it?

@author:
Tests aren't passing for mysql 3.6 appearly. 
How can it be fixed?

@user1:
I'm not sure what's going on there, maybe we could try performing `get_prep_value` on the remote field? (technically, this is where it should be, I just assumed `ForeignKey` did this automatically)

So try: `value = field.remote_field.get_prep_value(value)`

@author:
Seems that it fixes the problem. However the error message for mysql 3.6 is different which makes the tests fail. So tests need to be updated.
So I thought either make assertions only for status_code or do something like:
` self.assertIn(content, [
            {'message': "field filter error. 'abc' is not a valid value for feed_image (Field 'id' expected a number but got 'abc'.)"},
            {'message': "field filter error. 'abc' is not a valid value for feed_image (invalid literal for int() with base 10: 'abc')"}           
        ])`

@user1:
Maybe we could test the prefix? ``self.assertEqual(content['message'][:62], 'field filter error. 'abc' is not a valid value for feed_image')``

@author:
Tests pass now!

@user1:
Nice one, thanks!