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

Upgrade to 0.8.0 'unknown output field' #79

Closed
snewcomer opened this issue Mar 22, 2016 · 24 comments · Fixed by #80
Closed

Upgrade to 0.8.0 'unknown output field' #79

snewcomer opened this issue Mar 22, 2016 · 24 comments · Fixed by #80
Milestone

Comments

@snewcomer
Copy link

Cannot resolve unknown expression type, unknown output field

Any thoughts on what I am missing in my app? I see a utils.py file was added, but having trouble tracing down the problem.
I'm on the latest Django & django filter, and rest framework packages. Any thoughts on what I am missing b/w the jump from 0.7 to 0.8?

class PersonFilter(FilterSet):

    full_name = AllLookupsFilter(name='full_name')

    class Meta:
        model = Person


class PersonViewSet(viewsets.ModelViewSet):

    resource_name = 'people'
    model = Person
    queryset = Person.objects.all()
    filter_class = PersonFilter
    permission_classes = (permissions.IsAuthenticated,)
    serializer_class = ps.PersonListSerializer 
    filter_backends = (backends.DjangoFilterBackend, filters.SearchFilter,)
    search_fields = ('username', 'first_name', 'last_name', 'title', 'status__name')
@rpkilby
Copy link
Collaborator

rpkilby commented Mar 22, 2016

Hi @snewcomer - is full_name computed or a queryset annotation? Could you provide a more complete example of the model and manager classes?

@snewcomer
Copy link
Author

full_name is a field on the model

class PersonQuerySet(models.query.QuerySet):
    pass


class PersonManager(UserManager):
    def get_queryset(self):
        return PersonQuerySet(self.model, using=self._db).filter(deleted__isnull=True)


class Person(AbstractUser, BaseModel):
    title = models.CharField(max_length=50, blank=True, null=True)
    full_name = models.CharField(max_length=50, blank=True)
    status = models.ForeignKey(BaseStatus, blank=True, null=True)
    locale = models.ForeignKey(Locale, blank=True, null=True)
    phone_numbers = GenericRelation(PhoneNumber)

    # Managers
    objects = PersonManager()
    objects_all = UserManager()

    class JSONAPIMeta:
        resource_name = 'people'

    class Meta:
        ordering = ('full_name',)

    def save(self, *args, **kwargs):
        self._update_defaults()
        return super(Person, self).save(*args, **kwargs)

    def _update_defaults(self):
        if not self.locale:
            self.locale = Locale.objects.system_default()

@rpkilby
Copy link
Collaborator

rpkilby commented Mar 22, 2016

Hm. stacktrace?

@snewcomer
Copy link
Author

lookup(model_field) prints out as person.Person.full_name

Traceback (most recent call last):
File "/Users/snewcomer/Github/test-app/test-app-django/test-app/person/tests/test_views.py", line 30, in setUp
self.response = self.client.get('/api/admin/people')
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/test/client.py", line 503, in get
**extra)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/test/client.py", line 304, in get
return self.generic('GET', path, secure=secure, **r)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/test/client.py", line 380, in generic
return self.request(**r)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/test/client.py", line 449, in request
response = self.handler(environ)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/test/client.py", line 123, in __call__
response = self.get_response(request)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/core/handlers/base.py", line 230, in get_response
response = self.handle_uncaught_exception(request, resolver, sys.exc_info())
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/core/handlers/base.py", line 292, in handle_uncaught_exception
if resolver.urlconf_module is None:
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/utils/functional.py", line 33, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/core/urlresolvers.py", line 410, in urlconf_module
return import_module(self.urlconf_name)
File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/importlib/__init__.py", line 37, in import_module
__import__(name)
File "/Users/snewcomer/Github/test-app/test-app-django/test-app/test-app/urls.py", line 26, in <module>
from person import import views as person_views
File "/Users/snewcomer/Github/test-app/test-app-django/test-app/person/views.py", line 12, in <module>
class PersonFilter(FilterSet):
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/rest_framework_filters/filterset.py", line 59, in __new__
for lookup_expr in utils.lookups_for_field(field):
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/rest_framework_filters/utils.py", line 19, in lookups_for_field
in lookups_for_field(lookup(model_field).output_field)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/utils/functional.py", line 33, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File "/Users/snewcomer/.virtualenvs/venv/lib/python2.7/site-packages/django/db/models/expressions.py", line 229, in output_field
raise FieldError("Cannot resolve expression type, unknown output_field")
FieldError: Cannot resolve expression type, unknown output_field


@rpkilby
Copy link
Collaborator

rpkilby commented Mar 22, 2016

Thanks - am taking a quick look to see if I can spot anything obvious.

@rpkilby
Copy link
Collaborator

rpkilby commented Mar 22, 2016

Hm, I'll have to dive into this further, but to give you an idea, lookups_for_field is trying to recursively generate all valid lookup expressions for a given field. eg, given a charfield instance, you would get lookups such as exact, iexact, contains, icontains, etc... For a datetime, you would also have transformed lookups such as year__gte.

@rpkilby
Copy link
Collaborator

rpkilby commented Mar 22, 2016

Hi @snewcomer, I tried to recreate this but had to make a few modifications.

  • Simplified PersonManager.get_queryset() as there is no deleted field to filter on.
  • Commented out ForeignKeys to unprovided models (Status, Locale, PhoneNumber)
  • Commented out additional NewPerson model attributes and methods.

The test branch is here, but I was unable to recreate any issues. Feel free to submit PRs to that branch or add code comments here.

@rpkilby
Copy link
Collaborator

rpkilby commented Mar 29, 2016

Hi @snewcomer, were you able to resolve this?

@snewcomer
Copy link
Author

Been out of town. Hopefully by the end of the week. Sry about that!

@rpkilby
Copy link
Collaborator

rpkilby commented Mar 29, 2016

No worries. Just ping back when you have new info. Thanks.

@snewcomer
Copy link
Author

It's actually this line....remove it and all is good / tests pass. Any thoughts? Can't get a failing test on your fork...

full_name = AllLookupsFilter(name='full_name')

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 2, 2016

Hm. I'm out of options to try on my end without more information.

  • What is BaseModel? I just used models.Model. If they're different, try inheriting from that instead.
  • Do other fields have problems? eg, does AllLookupsFilter(name='title') fail in your code?
  • Is the code you're working on open source? Would I be able to run your tests?
  • The list of lookups should resolve as ['regex', 'startswith', 'search', 'gt', 'gte', 'iregex', 'contains', 'endswith', 'iendswith', 'icontains', 'iexact', 'isnull', 'lt', 'istartswith', 'lte', 'in', 'range', 'exact']

So, instead of

class PersonFilter(FilterSet):
    full_name = AllLookupsFilter(name='full_name')

    class Meta:
        model = Person

try

class PersonFilter(FilterSet):

    class Meta:
        model = Person
        fields = {
            'full_name': ['regex', 'startswith', 'search', 'gt', 'gte', 'iregex', 'contains', 'endswith', 'iendswith', 'icontains', 'iexact', 'isnull', 'lt', 'istartswith', 'lte', 'in', 'range', 'exact'],
        }

Is an exception raised from django-filter instead?

@snewcomer
Copy link
Author

Yep that worked. AllLookupsFilter fails in tests and prod.

One of the values from the class_lookups(model_field) is unaccent which is a subclass of the Transform lookup and produces the error. Didn't see that as one of the options from above.
https://github.com/philipn/django-rest-framework-filters/blob/master/rest_framework_filters/utils.py#L15

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 3, 2016

That makes a lot more sense. The list from above is the set of default lookups registered to CharFields. It sounds like unaccent is being registered by a third party (I don't see it in Django's source). Can you get the class path of the transform? What module/app that defines it?

Here's an example of some transforms that Django automatically registers.

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 3, 2016

Side note mostly to myself - the error handling here could be improved. We could try to get the output_field ahead of time, then reraise the FieldError with a message that contains the offending transform's name. That said, Django should probably have a more informative message here.

@snewcomer
Copy link
Author

Ya! Looks like this is the place where unaccent is registered with the CharField since I have django.contrib.postgres registered in my app. Not sure what the sol'n here is...

https://github.com/django/django/blob/53ccffdb8c8e47a4d4304df453d8c79a9be295ab/django/contrib/postgres/apps.py#L16

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 4, 2016

I think I figured it out. I forgot to wrap the model field in an Expression, which turns it into a proper lhs that can be passed to transforms/lookups. This isn't usually an issue, as most of the Transforms override the default output_field.

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 4, 2016

You'll notice that in django-filter, I did wrap the lhs in an Expression, so the behavior should be correct. I'm working on fixing this right now, but in the short term my recommendation is to not use AllLookupsFilter for CharField/TextFields. Instead, use the explicit Meta.fields syntax from above.

rpkilby pushed a commit to rpkilby/django-rest-framework-filters that referenced this issue Apr 4, 2016
rpkilby pushed a commit to rpkilby/django-rest-framework-filters that referenced this issue Apr 4, 2016
rpkilby pushed a commit that referenced this issue Apr 4, 2016
Fix #79, add transform recursion prevention
@rpkilby
Copy link
Collaborator

rpkilby commented Apr 4, 2016

Alright, this should be fixed. Could you install the master branch and verify?

@snewcomer
Copy link
Author

@rpkilby yep that did it for me. Thx!

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 6, 2016

Great. I guess just use 885b3a5 until the next release. I'll try to remember to ping the thread when it is.

@rpkilby rpkilby added this to the v0.8.1 milestone Apr 20, 2016
@NotSqrt
Copy link

NotSqrt commented Apr 26, 2016

@rpkilby Just encountered this issue when creating my own Transform with 0.8.0.

The traceback was a little more helpful:

  File "ve/site-packages/rest_framework_filters/utils.py", line 20, in lookups_for_field
    in lookups_for_field(lookup(model_field).output_field)
TypeError: Error when calling the metaclass bases
    __init__() takes exactly 3 arguments (2 given)

, because a Transform requires 2 arguments at __init__, not 1.

No more problems when using the master.

When do you intend to release v0.8.1 ?

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 26, 2016

There's one outstanding issue on the milestone, however Philip controls the actual release process.

@rpkilby
Copy link
Collaborator

rpkilby commented Sep 16, 2016

btw - v0.8.1 has been released.

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

Successfully merging a pull request may close this issue.

3 participants