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

Can't instantiate model if a property value is passed via kwargs #4

Closed mikeroll closed 3 years ago

mikeroll commented 3 years ago

Hello Marcus! I was evaluating your library and right from the start I stumbled on the lack of this seemingly basic feature.

Given this minimal model:

class MyModel(models.Model):
    objects = QueryablePropertiesManager()

    @queryable_property
    def z(self):
        return getattr(self, "_z", None)

    @z.setter
    def z(self, value):
        self._z = value

try creating an instance setting the initial z value:

In [13]: MyModel(z=42)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-13-e3bee329faca> in <module>
----> 1 MyModel(z=42)

~/code/app/.venv/lib/python3.8/site-packages/django/db/models/base.py in __init__(self, *args, **kwargs)
    553                     pass
    554             for kwarg in kwargs:
--> 555                 raise TypeError(
    556                     "%s() got an unexpected keyword argument '%s'"
    557                     % (cls.__name__, kwarg)

TypeError: MyModel() got an unexpected keyword argument 'z'

It is actually Django that is quite strict about what kinds of attributes it will attempt to set in __init__: https://github.com/django/django/blob/225d96533a8e05debd402a2bfe566487cc27d95f/django/db/models/base.py#L500 https://github.com/django/django/blob/225d96533a8e05debd402a2bfe566487cc27d95f/django/db/models/options.py#L876-L884 And surely with a normal property it works just fine. I also got it to work with queryable_property by forcefully adding my "z" to the _property_names.

Is/was there a specific reason not to inherit from property? Do you think we could do this now so that it passes the above check?

W1ldPo1nter commented 3 years ago

Hi,

You're right, queryable properties are currently not supported in a model's initializer - which is only due to the fact that I've never really thought about implementing it and that is wasn't requested until now. I'm certainly willing to accept this as a feature request.

Is/was there a specific reason not to inherit from property?

The reason for that was that queryable properties handle some things a little differently by explicitly not allowing a deleter and by using different sets of arguments when using queryable_property without decorating. It just seemed a lot cleaner to "start from scratch" rather than inheriting from property while disabling some of its features, etc. Also, the class-based approach is quite different as well.

However, since other solutions to this problem would probably involve fiddling with Django's internals or having to use a custom Model class (both of which I try to avoid as much as possible), it might be the "lesser evil" to allow to implement this feature. I will take a closer look at it.

W1ldPo1nter commented 3 years ago

Just to keep you in the loop: I have taken a closer look at the feature request and possible solutions and plan to achieve it by following the design pattern of Django's model fields a bit more: I will probably split up the QueryableProperty class into the actual QueryableProperty definition (like Django's field objects) and a separate descriptor class (like Django's field descriptors), which will then be the actual attribute on models (Django basically does the same thing with fields). The descriptor class will then inherit from property and therefore allow the initializer usage. This allows to keep the actual property definition the way it is (which also avoids any kind of breaking change) while achieving this new feature and have a better separation of concerns as the QueryableProperty base class is currently both used for the property definition as well as the descriptor.

I don't exactly know when I'll get to it yet, but I'm hoping to get it done in the near future.

W1ldPo1nter commented 3 years ago

I've finished implementing this feature in the branch feature/model-init-kwargs. The feature also received a test, of course. I will have to add it to the docs and then it will be part of version 1.7.0, which will be released in the near future.

mikeroll commented 3 years ago

Thank you so much for this, looks great! I'll check it out in the coming days.

W1ldPo1nter commented 3 years ago

Version 1.7.0 is now on PyPI - you can test the feature by simply installing the latest version. If everything works as expected, you can close this issue.

mikeroll commented 3 years ago

Works perfectly, thank you so much for this!