Skip to content
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

Unrecognized field type GeneratedField #1673

Open
roniemartinez opened this issue Jun 26, 2024 · 12 comments
Open

Unrecognized field type GeneratedField #1673

roniemartinez opened this issue Jun 26, 2024 · 12 comments

Comments

@roniemartinez
Copy link

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
Copy link
Owner

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

@violuke
Copy link

violuke commented Jul 19, 2024

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
Copy link
Owner

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
Copy link

violuke commented Jul 19, 2024

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
Copy link
Owner

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
Copy link

violuke commented Sep 3, 2024

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

@carltongibson
Copy link
Owner

carltongibson commented Sep 4, 2024

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

@theodor-franke
Copy link

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
Copy link
Owner

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

@theodor-franke
Copy link

Ah okay, I will adjust the PR accordingly

@theodor-franke
Copy link

theodor-franke commented Oct 21, 2024

@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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants