Django

Code

Ticket #3148 (assigned)

Opened 2 years ago

Last modified 7 hours ago

Add getters and setters to model fields

Reported by: jerf@jerf.org Assigned to: telenieko (accepted)
Milestone: Component: Database wrapper
Version: SVN Keywords:
Cc: Taiping, jerf@jerf.org, gav@thataddress.com Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

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.

Attachments

add.getter.setter.patch (5.0 kB) - added by jerf@jerf.org on 12/15/06 19:01:48.
patch to add getters and setters
revised.add.getter.setter.patch (6.2 kB) - added by jerf@jerf.org on 12/15/06 22:19:26.
add documentation, fix minor whitespace issue, supercede previous patch
revised2.add.getter.setter.patch (7.0 kB) - added by jerf@jerf.org on 12/19/06 16:48:30.
figured out what the numbers in the test cases meant; documentation example added for getter/setter
3148.diff (3.7 kB) - added by telenieko on 11/08/07 09:38:52.
New patch using python's property(), includes docs and tests
3148.2.diff (3.6 kB) - added by telenieko on 11/30/07 03:48:37.
Updated diff according to comments.
3148.3.diff (3.6 kB) - added by telenieko on 11/30/07 03:58:49.
Fixed small typo in the docs patch.
3148.4.diff (3.6 kB) - added by telenieko on 05/30/08 03:19:32.
Updated patch (merge conflict resolved) to latest trunk.

Change History

12/15/06 19:01:48 changed by jerf@jerf.org

  • attachment add.getter.setter.patch added.

patch to add getters and setters

12/15/06 22:19:26 changed by jerf@jerf.org

  • attachment revised.add.getter.setter.patch added.

add documentation, fix minor whitespace issue, supercede previous patch

12/16/06 12:14:50 changed by adrian

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?

12/19/06 16:48:30 changed by jerf@jerf.org

  • attachment revised2.add.getter.setter.patch added.

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

01/17/07 09:07:09 changed by anonymous

  • cc set to jerf@jerf.org.

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

01/30/07 02:59:10 changed by Simon G. <dev@simon.net.nz>

  • stage changed from Unreviewed to Design decision needed.

08/18/07 15:29:20 changed by enrobberors

  • cc changed from jerf@jerf.org to Taiping.
  • keywords set to MESSAGE.

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

08/18/07 15:29:25 changed by enrobberors

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

09/14/07 13:58:55 changed by anonymous

  • cc changed from Taiping to Taiping, jerf@jerf.org.

11/04/07 15:31:33 changed by 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.

11/08/07 09:38:52 changed by telenieko

  • attachment 3148.diff added.

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

11/08/07 09:42:18 changed by telenieko

  • owner changed from nobody to telenieko.
  • status changed from new to assigned.
  • version set to SVN.
  • summary changed from [patch] Add getters and setters to model fields to Add getters and setters to model fields.

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.

11/28/07 03:33:29 changed by telenieko

  • stage changed from Design decision needed to Accepted.

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! ;))

(follow-up: ↓ 11 ) 11/28/07 19:25:23 changed by SmileyChris

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)) 

(in reply to: ↑ 10 ; follow-up: ↓ 12 ) 11/29/07 11:05:56 changed by telenieko

  • keywords deleted.

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: {{{ #!python 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.

(in reply to: ↑ 11 ) 11/29/07 13:01:51 changed by SmileyChris

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 :)

11/29/07 13:16:51 changed by mtredinnick

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.

11/30/07 03:48:37 changed by telenieko

  • attachment 3148.2.diff added.

Updated diff according to comments.

11/30/07 03:50:22 changed by telenieko

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 :)

11/30/07 03:58:49 changed by telenieko

  • attachment 3148.3.diff added.

Fixed small typo in the docs patch.

(in reply to: ↑ description ; follow-up: ↓ 16 ) 05/29/08 22:01:12 changed by Adam

Any update on this?

05/30/08 03:19:32 changed by telenieko

  • attachment 3148.4.diff added.

Updated patch (merge conflict resolved) to latest trunk.

(in reply to: ↑ 15 ) 05/30/08 03:21:49 changed by telenieko

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.

06/01/08 15:11:11 changed by anonymous

  • stage changed from Accepted to Ready for checkin.

07/06/08 07:33:01 changed by anonymous

  • cc changed from Taiping, jerf@jerf.org to Taiping, jerf@jerf.org, gav@thataddress.com.

07/08/08 15:24:16 changed by Gulopine

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 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.


Add/Change #3148 (Add getters and setters to model fields)




Change Properties
Action