W1ldPo1nter / django-queryable-properties

Write Django model properties that can be used in database queries.
BSD 3-Clause "New" or "Revised" License
71 stars 1 forks source link

Use on abstract classes raises exception #1

Closed SafaAlfulaij closed 3 years ago

SafaAlfulaij commented 3 years ago

Hi! I was looking for such an easy to use library. Awsome work!

I just found an issue. If for some reason there was a need to __str__ the QueryableProperty (for example, trying to use the wrapped queryable_property directly in lookups with no filter) , and the property is in an abstract model, then self.model will be that abstract model, and __str__ will fail as self.model._meta.app_label will return None:

    def __str__(self):
        return '.'.join((self.model._meta.app_label, self.model._meta.object_name, self.name))

I didn't find issues (for now at least) by having self.model pointing to the abstract model, and I don't think this is solvable as per this upstream bug report: https://code.djangoproject.com/ticket/27664

If I found anything, I'll file more reports :p

W1ldPo1nter commented 3 years ago

Sorry about the late reponse - I misconfigured my notifications and didn't receive one for this issue.

It seems some test cases that include properties defined on abstract model are needed. But since you didn't report further bugs, I'm hoping that this is the only problem with queryable properties on abstract models.

I wasn't aware that abstract models may not provide an app_label - I'm going to check if there's a more reliable way to find the app name that also works for abstract models. If not, I might just leave out the app information in the string representation in such cases - that would still be better than an exception.

Thanks for the report, I will look into it.

W1ldPo1nter commented 3 years ago

Okay, I looked into it and it looks like this is a special case where an abstract model is defined outside of any installed app. This is also the only case where Django allows models outside of installed apps at all (starting with Django 1.9 - before that, models were never allowed outside of apps). Therefore it is understandable that there is no app_label - which also means there is no better way for the property to determine one.

Since the string representation of properties is only really used to generate helpful exception messages if something is wrong with a property, I will change the string representation to use standard attributes from Python's data model (__module__ and __name__ to be precise), so that a consistent string representation can always be created regardless of how exactly a model is set up. That will slightly change the string representation from typically something like my_app.MyModel.my_property to my_app.models.MyModel.my_property, but since it is only used to generate helpful exceptions, this works just as well.

I'm going to release a version containing this fix as 1.4.1 and close this issue as soon as I've implemented that change.

W1ldPo1nter commented 3 years ago

This is now released on PyPI as 1.4.1.