carltongibson / django-filter

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

Unrecognized field type GeneratedField #1673

Open roniemartinez opened 4 months ago

roniemartinez commented 4 months ago

I've got this error which is caused by using a GeneratedField. Before migrating to GeneratedField, we used to use a migration script to create the generated field and django-filter has no issue with it.

AssertionError: MyFilter resolved field 'my_field' with 'exact' lookup to an unrecognized field type GeneratedField. 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 think it should be possible to know what filter to use since GeneratedField has an output_field.

carltongibson commented 4 months ago

@roniemartinez Could you put together a proof-of-concept showing what you mean here, perhaps with tests showing different examples? Thanks.

violuke commented 4 months ago

If you have a field in your model like:

code_full = models.GeneratedField(
        expression=If(Q(code_number__isnull=True), Value(None), Concat("code_prefix", "code_number")),
        db_persist=True,
        output_field=models.CharField(blank=True, null=True, max_length=15),
    )

Then an error like @roniemartinez posted above will be raised.

This can be fixed with overriding filter_for_field() (note the # Handle GeneratedFields block):

@classmethod
def filter_for_field(cls, field, field_name, lookup_expr=None):
    if lookup_expr is None:
        lookup_expr = settings.DEFAULT_LOOKUP_EXPR

    # Handle GeneratedFields
    if isinstance(field, GeneratedField):
        new_field = field.output_field
        new_field.model = field.model
        field = new_field

    field, lookup_type = resolve_field(field, lookup_expr)

    default = {
        "field_name": field_name,
        "lookup_expr": lookup_expr,
    }

    filter_class, params = cls.filter_for_lookup(field, lookup_type)
    default.update(params)

    assert filter_class is not None, (
                                         "%s resolved field '%s' with '%s' lookup to an unrecognized field "
                                         "type %s. 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"
                                     ) % (cls.__name__, field_name, lookup_expr, field.__class__.__name__)

    return filter_class(**default)

It might not be the perfect fix, but it's worked well for me in my testing.

carltongibson commented 4 months ago

So there's two parts to this.

The is the error. That should be addressed by #1675, which will enable skipping unknown fields — if you haven't used filter_overrides.

The second is the generated field handling itself:

    # Handle GeneratedFields
    if isinstance(field, GeneratedField):
        new_field = field.output_field
        new_field.model = field.model
        field = new_field

Is it as simple as that? Can we add built-in support?

Disclaimer: I haven't used GeneratedField yet, so haven't looked into what works and doesn't here at all.

violuke commented 4 months ago

Is it as simple as that? Can we add built-in support?

Built-in support would be great! I can't find a problem with this solution so far, and it's been deployed to production today, so 🤞

I'll let you know if we find any issues, but I think this is all that's needed.

carltongibson commented 3 months ago

OK, but the correct place to add this would be to the FILTER_FOR_DBFIELD_DEFAULTS, probably with a specific Filter subclass.

If someone wants to take that on as an addition (with docs and tests) that would be very welcome.

violuke commented 2 months ago

Just to confirm, this change has been working problem-free for use for over a month now 👍

carltongibson commented 2 months ago

@violuke Would you fancy creating a PR, but adding a new Filter subclass, rather than inline in filter_for_field()?

theodor-franke commented 1 month ago

I think that you can not simply add it to FILTER_FOR_DBFIELD_DEFAULTS because you need to unwrap the actual field-type that is generated in the generated field first.

carltongibson commented 1 month ago

@theodor-franke The field is available to inspect via the lambda passed as the extra key in FILTER_FOR_DBFIELD_DEFAULTS.

theodor-franke commented 1 month ago

Ah okay, I will adjust the PR accordingly

theodor-franke commented 3 weeks ago

@carltongibson I had a deeper look into this problem. Yes i can pass the actual field with the extra key in FILTER_FOR_DBFIELD_DEFAULTS but than I still need to map the correct field_class to the given output_field this is already implementet in BaseFilterSet.filter_for_lookup() and I don't want to implement this logic twice (I cant use this method because than I have cyclic imports). I could implement something like this in the BaseFilterSet.filter_for_lookup() method:

if filter_class == GeneratedFieldFilter:
    return cls.filter_for_lookup(field.output_field, lookup_type)

but iam not a huge fan of this. Should i extract BaseFilterSet.filter_for_lookup() into utils.py? Is there something that iam missing?

carltongibson commented 3 weeks ago

@theodor-franke thanks for looking in it. Let me have a play