Add getters and setters to model fields

[jerf@…]

Whenever you have two distinct ways to update a variable, you introduce the opportunity to have bugs. This becomes increasingly true as the system grows in size.

It is often the case that when changing one field on an object, you want to be able to automatically run code of some kind; this is the same basic motivation behind the "property" builtin in Python. However, the obvious way of doing that in Django doesn't work:

class Something(models.Model):
    field = models.BooleanField(...)

    ...

    def set_field(self, value):
        # do something

    field = property(set_field)

The second field overrides the first, and in the process of constructing the model Django never gets a chance to see the models.BooleanField.

This patch adds a 'getter' and 'setter' attribute to all fields, which takes a string of a method to call when a field is retrieved or set. It turns out that it is fairly easy to add a property to the class during Django's initialization, at which point it has already retrieved the field information. This example from the enclosed tests shows the basics of its usage:

class GetSet(models.Model):
    has_getter = models.CharField(maxlength=20, getter='simple_getter')
    has_setter = models.CharField(maxlength=20, setter='simple_setter')
    has_both = models.CharField(maxlength=20, getter='simple_getter',
                                setter='updater')
    updated_length_field = models.IntegerField(default=0)

    def simple_getter(self, value):
        return value + "_getter"
    def simple_setter(self, value):
        return value + "_setter"
    def updater(self, value):
        self.updated_length_field = len(value)
        return value

This defines a getter on has_getter that returns a filtered value from the DB, a setter on has_setter that processes the value to add "_setter" to it in all cases, and on has_both we see a setter than implements the use case of updating another field when a property is set. (As is often the case, this is a trivial example; in my real code, for instance, the value being updated is actually in a related object.)

A getter receives as its argument the current "real" value (the one that either came from the database, object construction, or a prior setting of the value), and what the getter returns is actually what the user gets back from the attribute.

A setter receives as its argument the value that the user is setting the property to, and what it returns is what the property will actually be set to. The pattern for just using that as a hook is to return what is passed in after you've done whatever it is your hook does, as shown above.

These properties are only created for fields that have getters or setters, so the backwards-compatibility impact of this should be zero, and the performance impact should be a check for getters/setters per field once at startup, which is minimal. I'm a little less certain about exactly how these getters and setters will interact with all the various field types, but due to the way in which it hooks in it should, in theory, have minimal impact, because no Python code should fail to go through the property.

Getters and setters do not operate during the creation of the object, be it by retrieval from the database or creation by instantiating the class; this avoids a lot of tricky issues with property initialization order and double-application of some common setters, but will need to be documented.

I'd be happy to add the appropriate documentation but I do not see where to contribute that.

[jerf@…]

patch to add getters and setters

[jerf@…]

add documentation, fix minor whitespace issue, supercede previous patch

[Adrian Holovaty]

Before checking in this new functionality, I'd be very interested in seeing whether it would be possible to allow for "normal" property usage on models, rather than inventing a new way of doing it.

Seems like it would be tricky to figure it out, but that shouldn't stop us from experimenting... Who wants to try?

[jerf@…]

figured out what the numbers in the test cases meant; documentation example added for getter/setter

[anonymous]

Explained why I don't think there is a better way in ​this post.

[enrobberors]

If you've got a problem chum, think how it could be.

[enrobberors]

If you've got a problem chum, think how it could be.

[Wintermute]

Hi,

Has this issue been forgotten? I think there must be quite many developers who wish to do processing based on field changes. At least I do and this patch would provide the solution.

[Marc Fargas]

New patch using python's property(), includes docs and tests

[Marc Fargas]

Hi there,
I found that ticket looking for a way to achieve exactly what this ticket asks for and saw adrian's comment above about trying to use normal properties to improve the patch.

I've just attached a new patch with tests and docs that does just this. In essence you'd do:

    from django.db import models
    class Person(models.Model):
        def _get_name(self):
            return self.__name
        def _set_name(self, value):
            self.__name = value

        name = models.CharField(max_length=30, property=(_get_name, _set_name))

And django will create "Person.name" as a property() using the tuple given.

[Marc Fargas]

Moving to Accepted because:

• The only "core" request was in comment 3 (from adrian) about doing this "more pythonic" and it is about "doing it better" not "I'm not sure if it fits well in Django".
• There's nothing in the comments that imply "Design Decision", meaning that what the tickets requests/does does not seem to imply this status.
• Trying to give a bit of visibility to the ticket (Reasons 1 & 2 are only excuses! ;))

[Chris Beaven]

Reviewing your patch:

1. Using the reserved word "property" sounds like it'll get you in to trouble. In fact, I can't see how the patch would even work since you try to setattr to property(getter, setter, delter, docter)) but property would be defined as the tuple! How about using property_ or use_property or something.

2. why do you need to keep the property tuple around? (i.e. self.property = property)

3. instead of your add magic, how about something simpler, like:

   if len(use_property) < 2
       # getter and setter are both required.
       raise ValueError("You must specify at least a getter and a setter method with use_property") 
   # Create a property on ``cls`` with the methods given. 
   setattr(cls, self.name, property(*use_property)) 
   

[Marc Fargas]

Replying to SmileyChris:

Before anything, if you look at the attached patch from TRAC it might seem that everything is on the same function (Field.__init__(),
but that's not the case, there are two functions patched here: Field.__init__() and Field.contribute_to_class(). (See the line marked with dots in TRAC's diff viewer).

1. Using the reserved word "property" sounds like it'll get you in to trouble. In fact, I can't see how the patch would even work since you try to setattr to property(getter, setter, delter, docter)) but property would be defined as the tuple! How about using property_ or use_property or something.

Well, there are tests included with the patch and they don't fail! ;)
Anyway; It does not crash because the callable property() is not used in Field.__init__() for anything. So it's safe to use it there.
Then we save the parameter in Field.property which can never be accesed as property outside of Field.__init__(), it will always be self.property.

The reason I used property= instead of something else was to make it clear what the parameter is about, and give an idea of how it works (just exactly as property())

2. why do you need to keep the property tuple around? (i.e. self.property = property)

Because property is read in Field.__init__() and used then in Field.contribute_to_class() to create the property in the model. So I need to save the property parameter from init() to use it in contribute_to_class(). Also I wanted to be consistent, the first Thing Field.__init__() does is save all the kwargs on self. Anyway, it's needed for contribute_to_class.

3. instead of your add magic, how about something simpler, like:

   if len(use_property) < 2
       # getter and setter are both required.
       raise ValueError("You must specify at least a getter and a setter method with use_property") 
   # Create a property on ``cls`` with the methods given. 
   setattr(cls, self.name, property(*use_property)) 
   

Uhm.. really nice idea, I'll update the patch later with this change.
Before updating, do you really thing the parameter to Field should not be property? As explained aboved, it will never cause a namespace collision until somebody wants to use property() in Field.__init__() is is unlikely to happen due to what Field.__init__() does.

[Chris Beaven]

Replying to telenieko:

Before anything, if you look at the attached patch from TRAC it might seem that everything is on the same function (Field.__init__(),
but that's not the case, there are two functions patched here: Field.__init__() and Field.contribute_to_class(). (See the line marked with dots in TRAC's diff viewer).

Oops, rather big oversight on my part :P I should probably apply patches rather than just reviewing them from trac's visual diff...
So I guess you can ignore all of points 2 and 3.

Uhm.. really nice idea, I'll update the patch later with this change.
Before updating, do you really thing the parameter to Field should not be property?

Nah, I'm cool with it then - for example, Models have an id property so the precedence has been set :)

[Malcolm Tredinnick]

Change the "property" name, please. There are lots of words in the English language; no need to court an accidental collision by using effectively reserved names.

[Marc Fargas]

Updated diff according to comments.

[Marc Fargas]

Here it is,
The option is now: use_property; Inside the Field class it is saved as self.model_property so it's clear what it stands for.

I also removed the add magic as per SmileyChris comment, and used his * thing, nice :)

Hope it makes you all happy!! ;)
Tests passed fine :)

[Marc Fargas]

Fixed small typo in the docs patch.

[Adam]

Any update on this?

[Marc Fargas]

Updated patch (merge conflict resolved) to latest trunk.

[Marc Fargas]

Replying to Adam:

Any update on this?

I always keep an updated patch on my laptop but don't upload it too often.

Hope the updated patch works a bit longer than the last one! hhehe.

[Marty Alchin]

Doing some quick looking at this (I'll admit, I haven't applied it yet), I came up with a 7-line utility function that handles everything the patch does, but without touching core at all. If there's interest, I can post it at djangosnippets, but I think there's something else to consider.

While the latest patch is a considerable cleanup from the original patch, I think ​one of the original author's considerations has been lost in translation. Again, I haven't applied it, but it looks like it does exactly what my little utility function does, and mine certainly suffers from the double-setter problem he describes.

Malcolm's [source:django/trunk/django/db/models/fields/subclassing.py field subclassing work] does a good job of avoiding this, but it uses a descriptor, which would interfere with the property, unless a new descriptor is made that takes the double-setter issue *and* the property stuff into account. Ultimately, I think it's a matter of providing the simple, common case, like Malcolm did, and expecting those who need more to know what they need. I don't know if that's enough for everybody, but that's why I was working on a non-core option for this. I'd rather people do a little hunting on the problem they have before they just pick a "solution" that happens to be in core and get confused why it doesn't work the way it should.

Of course, one other thing SubfieldBase takes into account is serialization. Again, I haven't applied the patch or tested it out (I'm heck-deep in other stuff at the moment), but I'm guessing there are going to be some surprises, since I've run into a few things in the past when an object's __dict__ didn't line up with what some properties were advertising. I'd certainly at least want to see that looked at by someone who's passionate about this patch.

[terrex2]

updated patch for rev.8463

[Marc Fargas]

The docs-refactor broke the patch, always up-to-date patch ​here in colours and ​raw

[Jacob]

I'm busy and don't have time to explain this fully. However, as a note to myself and anyone who wants to try to channel my thinking, the syntax I'd like here would look like::

class MyModel(Model):
    foo = CharField(...)

    @foo.getter
    def get_foo(self):
        ...

    @foo.setter
    def set_foo(self, value):
        ...

I'll try to circle 'round back to this and explain more fully when I have time.

[Marty Alchin]

Replying to jacob:

I'm busy and don't have time to explain this fully. However, as a note to myself and anyone who wants to try to channel my thinking, the syntax I'd like here would look like

It seems simple enough to channel that, actually. I assume this would work identically to the ​new property methods added in Python 2.6. I've been wondering about this myself lately, but didn't spend the time to work on it because I forgot this ticket already exists. I'll see what I can throw together this week.

I can think of a few questions that might need to be addressed along the way (at least one that definitely will), but I'll hold off until I have some working code before bringing them up. On a side note, if we mimic the property methods, I assume we'd also want to include @foo.deleter as well, which should be simple enough to throw in.

[Marty Alchin]

After doing some work on this, I'll throw in one particular comment about naming. Python's built-in @property methods require that all the methods have the same name, because things go pretty wonky if you don't. They have their reasons, I'm sure, but I don't think that requirement makes a lot of sense in our situation. Instead, I'm going with the (much more obvious, IMO) approach of leaving any user-created methods in place and updating the field object to work as a descriptor. In the context of what a model definition looks like, I think that makes the most sense.

[Daniel Hahler]

I think this a fundamental feature, and although not fluent in Python and Django, I've started working on it.

The proposed/attached patch implements the method suggested by jacob.

I've added tests for it and all the current tests still pass (there are oddities with admin_scripts, but that's with and without the patch).

[Daniel Hahler]

Patch against trunk r11791 - new approach

[Alex Gaynor]

Some feedback:

a) A decorator should always return a callable, in this case it should return the original function (as Gulopine sugested), since the alternative leaves None on the class, which is really awkward IMO.

b) As Gulopine suggested I think the better approach is to make the Field object itself the descriptor when getter/setter/deleter are provided, not a property object.

c) This should be handled by Field.contribute_to_class, not the Metaclass.

d) I'd like to see significantly more extensive tests, particularly with things like:

• Providing only a subset of the available options.
• Providing a getter/setter with fields that have their own descriptor (FileField and the RelatedFields). This I suspect may require a large refactor of related.py, as Gulopine may have already found :)

[Alex Gaynor]

Oh, and

e) Documentation! :)

[Daniel Hahler]

Thanks for your feedback.
a) getter/setter/deleter now return the original function
b) I do not get this: using a decorator should make the object behave as descriptor? Would that mean to have get/set/del always, but check there if a decorator should get called?
c) OK, moved. Much better. It took me a while to come that close already though.. ;)
d) 1. This should fail, at least when going the property route, my understanding is that you need to define all, otherwise the field is e.g. not writable. This would be different when using descriptors only, of course.

2. Only fields derived from Field are supported currently. FileField does not come through Field - at least that's what my testing shows. Should they get handled?

e) I think the other points should get worked out before.. :)

[Daniel Hahler]

Slightly updated patch

[Alex Gaynor]

The reason you aren't seeing FileField/RelatedFields working right now is that they provide their own descriptor, which overwrites yours I imagine. This is why Gulopine and I discussed integrating the descriptor into the Field itself (and only conditionally attaching it to the class) and then integrating the existing descriptors for Files and related objects into the class itself so everything works together correctly. You can find our discussion of this here: ​http://botland.oebfare.com/logger/django-dev/2009/12/6/1/#22:41-1918558 (extends onto the next page).

[Alex Gaynor]

Thanks for all your work on this BTW!

[Daniel Hahler]

FileField/RelatedFields work as before (at least passing tests - I have not tried it for real) - they are not affected by getter/setter currently (AFAICS).

My understanding had been that I'm not using a descriptor, but property/ies instead - I've just read that it's considered to be a descriptor, too.

The discussion you've linked sounds good, but still I'm not sure where you're heading.. :)

Therefore, a last patch from me (which does not use property() anymore), but adds a FieldDescriptor class. There's a lot to fix/straighten for sure, but just to get this out for discussion.

I'll now wait until Gulopine and others provide some feedback on this.

[Daniel Hahler]

Approach using FieldDescriptor class (and using decorators to decide if fields get wrapped into it or not)

[jerf]

Using the model:

class GetSet(models.Model):
    field = models.CharField(max_length=200)

    @field.setter
    def field_setter(self, value):
        self._field = value + "_test_transform"
    @field.getter
    def field_getter(self):
        return self._field

we obtain with the latest patch "django-3148-approach-via-descriptors.diff" the following python REPL output:

Python 2.6.4 (r264:75706, Nov  8 2009, 19:34:30)
[GCC 4.3.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from mysite.modeltest.models import GetSet
>>> example = GetSet(field = "test")
>>> example.field
'test_test_transform'
>>> example.save()
>>> example.id
1
>>> del example
>>> example = GetSet.objects.get(id = 1)
>>> example.field
u'test_test_transform_test_transform'
>>> example.save()
>>> del example
>>> example = GetSet.objects.get(id = 1)
>>> example.field
u'test_test_transform_test_transform_test_transform'
>>>

That is not shippable behavior. The setter must not apply during construction from a database load. Loading and immediately saving an object with no changes must be a no-op, in terms of the stored data. Anything else is asking for disaster.

I find myself wondering if perhaps the traffic on this bug is telling us that the interaction of properties when the values are being backed by a database store is just too complicated a concept to be worthwhile. When the setters and getters would apply is more subtle than it first appears, and perhaps "simple enough that it works as it first appears" (i.e., "no property behavior") is a better idea.

I am not being sarcastic or passive aggressive. I am seriously wondering this. I now think the documentation would have to explain this in more detail than I originally thought, which adds a chunk of complexity right where Django's documentation needs it least, and I'm no longer convinced that's worth it. (Plus, after reviewing the latest SVN code, I still do not see a way to do this correctly without setting the flag during construction like I did in my original patch, which was not an... appreciated addition to the model loading code.)

[Ilya Semenov]

Wrong. Lack of a correct implementation does not mean the initial problem doesn't worth solving it. Logically, the desired behaviour is simple as hell - do not run setters/getters when Django does its ORM things (that is: object fetching, construction, saving, deleting, caching, etc.), but always call getters/setters when a model is used from from an external (business logic) code. I don't see anything strange or hard or inconsistent from the architectural point of view either. The ORM - which is aware of the essence of the models - should work with the data directly using internal structures (__dict__ or whatever), and all the other unaware code should be using the "frontend" properties.

The "simple enough that it works as it first appears" approach is ultimately doomed. If everyone was adopting this kind of thinking, there would be no ORM at all and we'd be running manual SQL queries which are "simple enough that they work as they first appear". Abstractions always leak and there will always be trade-offs, but that doesn't mean we should stop creating them. We should only stop when the gain is less than the cost, which is clearly not the case for getters and setters.

By the way, I am more and more thinking that the part of the confusion comes from that everyone is talking about BOTH getters and setters. In my opinion, we should have only setters. As the data is backed by a database, reading should always return what the database actually holds (with respect to Field.to_python(), of course). However, setting is different and needs to be able to be overriden indeed - because that allows to implement additional validation, transformation (e.g. proper formatting), and other things like logging.

One of the clear examples is using Model Forms. Right now, the whole powerful concept is barely useful for any non-trivial models, because you can't just call modelform.save() and expect it to call all setters properly -- instead, you always have to code all that manually. However, I can't imagine any example where a getter would be of any use.

[Ilya Semenov]

I beg my pardon for not reading the "simple enough that it works as it first appears" sentence clearly. I'm taking my words back. Of course, the things should work as they appear. That just doesn't mean we can't achieve that behavior with custom setters. :)

[josh.smeaton@…]

One of my pet peeves is that Django models are almost always used as simple data containers. This is true for many frameworks and technologies though. What is wrong with defining methods on the class explicitly that are responsible for setting the correct fields, where there is logic associated with them? Allow the users of the model to decide whether they call the documented api of the model, or go directly to the fields.

[Alex Gaynor]

Unassigning, I haven't had the time to work on this.

[berdario]

This approach works with m2m fields and inherited fields

[berdario]

slightly more complex approach, that on the other hand manages to obtain the same behaviour with m2m fields

[berdario]

Fixes to the previous patch

[Anssi Kääriäinen]

I have a feeling this ticket needs to be wontfixed. The reason is that it seems the setter should not be called in model.__init__ if the object comes from the DB. On the other hand, __setattr__ must be called for backwards compatibility reasons. I don't see how to achieve this: you can't assign to the attribute in __init__ - setter will not be called. You can't assign to __dict__ - setattr will not be called. In addition the solution must not cause any severe performance regressions to __init__

We have managed to live without this feature to this day. Maybe it is time to just let this feature go?

[Aymeric Augustin]

Indeed, this could introduce subtle backwards incompatibilities for people who have overridden __setattr__. I don't believe this had been noticed until now.

An earlier comment claims that "the desired behaviour is simple as hell", but that isn't backed by any code and the analysis includes some hand-waving. I see that the latest patch, which is the 14th attempt at doing this correctly, still contains a comment saying it's a hack, and it alters __bases__. I'm not comfortable with such techniques, to say the least.

My recommendation would be to call the field _foo and have a foo property on your model that actually gets/sets _foo. This might seem unclean, but it's absolutely transparent, and the relation with the ORM is totally obvious. It's also immune to the multiple-execution problem.

If you have a better idea — and enough arguments to convince the core team that it doesn't have side effects such as the one found by Anssi — please make your case on django-developers. Thanks!

[anonymous]

Anyone looking for a simple answer to this problem without using an underscore in the database could try:

​http://www.korokithakis.net/posts/how-replace-django-model-field-property/

classMyClass(models.Model):
    _my_date=models.DateField(db_column="my_date")

@property
def my_date(self):
    return self._my_date

@my_date.setter
def my_date(self,value):
    if value>datetime.date.today():
        logger.warning("The date chosen was in the future.")
    self._my_date=value

Augustin's solution was half complete.

[ariel17]

This solution does not work well on templates because they cannot see a field starting with "_". Also queries cannot be done on this field (for example, when writing a custom model manager) because referencing the field "_field" confuses the ORM with field joining.