Why is from_db_value not called on ModelClass.objects.create()?

[Gordon]

I originally asked this on IRC.  It was suggested to raise the question in the mailing list. I've copied the log below with irrelevant lines removed (dpastes provided after the quoted irc)

<gp> Why is from_db_value not called on Cls.objects.create()?  For example, in the CashModel test, why isn't it tested that cash is an instance of Cash after create?

<gp> https://github.com/django/django/blob/master/tests/from_db_value/tests.py

<gp> Something like http://dpaste.com/1R69S5F  I seem to be getting the wrong value for a custom primary key in the admin because of this

<gp> *only on the create redirect

<gp> Or is this expected to be handled by the field?

<timograham> gp: this seems similar to #24028

<gp> timograhm: would that test 'test_create()' in the dpaste fit in with the from_db_value?  Or should the create test be somewhere else?

<timograham> seems okay there, does it pass or are you proposing a behavior change?

<gp> When I was trying to figure out why my values were incorrect I tried that test and it failed.  But I would need to verify in a clean project before sending a pr

<gp> but I think the actual fix is outside of my knowledge

<gp> This ugly hack seems to fix it on my field if that means anything to you http://dpaste.com/3J0C5DD

<timograham> gp: it seems like it could be a useful behavior. I guess it has probably been discussed before but I'm not aware of the discussion. Maybe it would be worth raising on the mailing list and at least documenting the reasons for the current behavior if it can't be changed due to backwards compatibility.

 The first dpaste (1R69S5F) is a copy of the from_db_value/tests.py with the following modifications:

class FromDBValueTest(TestCase):
    def setUp(self):
        self.obj = CashModel.objects.create(cash='12.50')

    def test_create(self):
        self.assertIsInstance(self.obj.cash, Cash)

The second dpaste (3J0C5DD) is the following:

    def contribute_to_class(self, cls, name):
        super(IntegerIdentifierBase, self).contribute_to_class(cls, name)
        cls_save = cls.save
        def save_wrapper(obj, *args, **kwargs):
            cls_save(obj, *args, **kwargs)
            value = getattr(obj, self.attname)
            value = self.to_python(value)
            setattr(obj, self.attname, value)
        cls.save = save_wrapper

[charettes]

Hi Gordon,

That's what I would expect the cash attribute to be since it was not retrieved
from the database but explicit set during initialization. To me the `from_db_value`
method name makes it clear it is only called in the former case. If you want
the object you created to have a certain value pass them as desired:

CashModel.objects.create(cash=Cash('12.50'))

or call clean() on your model instance to make sure CashField.to_python() is called.

Note that this behavior can also be exhibited by assigning a value to an already
existing instance.

instance = CashModel.objects.get()
instance.cash = '12.50'
assert instance.cash == '12.50'

I think it adheres to the principle of least surprise since it makes Django
models behave just like normal Python classes in this regard.

e.g.

class Foo:
    def __init__(self, bar):
        self.bar = bar

assert Foo('value').bar == 'value'

If you really want to make sure assignment for a specific field
sets an altered value on an instance the you'll have attach a
descriptor on the model class and make sure to call
CashField.to_python on __set__(). Django does this for

Simon

[Marten Kenbeek]

I've had to explain this behaviour a few times - most recently this SO question - so 

I think it's worth it to explicitly document this behaviour. I agree with Simon, though - I don't 

think the behaviour itself should change. 

You can always call refresh_from_db() to reload the values from the database. This method

was added in 1.8. Just a quick idea: adding a flag to Model.save() to call refresh_from_db() 

after the model is saved might make it more accessible, and it makes it clearer that the values 

on the model aren't changed if you don't explicitly reload the model. 

Marten

[Gordon]

Thanks for the replies and suggestions Simon and Marten.

I am definitely calling form.clean() because I am using the default admin for views in my tests.  The field in the case that was tripping me up is being used as an auto-populating primary key... so the admin form was excluding it from clean() and then redirecting to the wrong url.

Since the behavior is written and working as intended I can tackle this particular problem from a different direction.  But in my mind, it still seems like ModelClass.objects.create() should return an instance as if it were pulled from the database... especially with convenience methods like `get_or_create` because the field value would be different in this scenario depending on if the method uses get or create to return the instance (I haven't looked at the implementation but the name would imply it relies on the create method).  But I guess there are cases where you don't need to use the returned instance and would waste a few cpu cycles.  This issue makes `get_or_create` a little dangerous I think.

I think it adheres to the principle of least surprise since it makes Django
models behave just like normal Python classes in this regard.

I am not sure about this since ModelClass.objects.create() is a manager method that returns a new model instance and not the manager "self".  But after rereading the documentation about the `create` method it does provide an alternate equivalent syntax that I would expect this behavior from.