search

carltongibson / django-filter

A generic system for filtering Django QuerySets based on user selections
https://django-filter.readthedocs.io/en/main/
Other
4.39k stars 760 forks source link

Catch JSONField error to prevent not being able to show the response data #1666

Open loeeess opened 1 month ago

loeeess commented 1 month ago

Hello,

I use django-filter in combination with DRF and I have created a custom base view for GET requests that is used for various models, and returns a list of all object instances of the selected model. Some of these models have a JSONField, others don't. I use django-filter for each field of the models. Because JSONField is not covered by django-filter by default, it causes a 500 error when I retrieve the list of model instances for a model that includes a JSONField in the response, see the following response:

{
    "detail": "<class 'AssertionError'>: AutoFilterSet resolved field 'model_field_name' with 'exact' lookup to an unrecognized field type JSONField. Try adding an override to 'Meta.filter_overrides'. See: https://django-filter.readthedocs.io/en/main/ref/filterset.html#customise-filter-generation-with-filter-overrides"
}

I am aware of the fact that I can indeed write a custom filter (which I did, works fine). But without this custom implementation, the error prevents me from viewing the GET response in the first place.

My proposed solutions would be to either;

  1. 'Ignore' the JSONField when creating the filters for the model attributes. Then a user can still retrieve their requested data, the only downside being that the JSONField attributes are not included in the filter list.
  2. Implement JSONField in the package, where it is possible to define which key/value pairs can be filtered on.

Thank you.

carltongibson commented 2 weeks ago

Hi @loeeess — thanks for the report.

So I think a FilterSet option to do one of three things when encountering an unknown field type is the way forward here: Raise (current behaviour, should be the default), Warn (pass over, but emit a warning), Ignore (silently pass over). Folks could then set either Warn or Ignore if they hit your use-case.

Would you fancy taking that on?

loeeess commented 2 weeks ago

Thanks for your reply, @carltongibson! Sounds like a good solution to me. And yes, I can look into it.

loeeess commented 1 week ago

I just added a PR (#1675) for this issue. I realize the documentation for the FilterSet options should be updated as well. If these changes look alright, I don't mind taking on the task of updating these docs.

carltongibson commented 1 week ago

@loeeess Great work thanks. I left an initial comment. Let's carry on there. 👍