EmbeddedModelField pre_save, get_db_prep_value rework

[gpitfield]

[did this on the master branch b/c there's an error that prevents testing on the develop branch for me, let me know if I should merge the changes from my local master back to develop and do another pull request]

Removes pre_save() function entirely, moving its functionality
into get_db_prep_value. Among other things, this means that
queryset update() calls on EmbeddedModelFields work

[jonashaag]

Holy cow. If I only knew it was that simple... thanks for the patch!

Yes, this should be done against develop but no reason to start another pull request. We can simply choose to merge this into develop when pulling.

What's left is some tests for the new feature that fail without and pass with this patch.

[wrwrwr]

Maybe lets also call the method get_db_prep_save and call the same on fields -- as this actually prepares values only for storage. I'm working on some other changes to the same code and will have to merge them together in a couple of days, so let me know if the following test is enough to be sure I've not caused a regression (feel free to copy or add anything you consider worthwhile to be checked):

    def test_update(self):
        """
        Test that update can be used on an a subset of objects
        containing collections of embedded instances; see issue #13.
        Also ensure that updated values are coerced according to
        collection field.
        """
        class Parent(models.Model):
            pass
        class Child(models.Model):
            id = models.IntegerField(primary_key=True)
            integer_list = ListField(IntegerField)
            integer_dict = DictField(IntegerField)
            embedded_list = ListField(EmbeddedModelField(Parent))
            embedded_dict = DictField(EmbeddedModelField(Parent))
        parent1 = Parent.objects.create()
        parent2 = Parent.objects.create()
        child = Child.objects.create(pk=1,
            integer_list=[1], integer_dict={'a': 2},
            embedded_list=[parent1], embedded_dict={'a': parent2})
        Child.objects.filter(pk=1).update(
            integer_list=['3'], integer_dict={'b': '3'},
            embedded_list=[parent2], embedded_dict={'b': parent1})
        child = Child.objects.get()
        self.assertEqual(child.integer_list, [3])
        self.assertEqual(child.integer_dict, {'b': 3})
        self.assertEqual(child.embedded_list, [parent2])
        self.assertEqual(child.embedded_dict, {'b': parent1})

[jonashaag]

Looks great but you could also re-use existing models instead of defining new ones for every test. Also I don't think you can use integer PKs on MongoDB.

It would also be nice to have a test that ensures that values are coerced correctly when using .update, e.g. if you have an ListField(IntegerField) and do .update(foo=['1']) that the 1 isn't stored as a string.

[wrwrwr]

I find those existing models a bit hard to use for debugging type conversions -- they are rather big and even creating a single object often causes a ton of value_to_db_* or convert_value_for/from_db calls to happen.

Non-ObjectId keys -- yes you can, only AutoFields are limited to ObjectIds (and MongoDB docs say that you may use anything as far as you can guarantee uniqueness and that it's not an "array" ;-) There are some flaws in the logic that decides what should be converted and what should not, so you may get a surprising "values representing ObjectId" exception here and there, but that's a part of what I'm working on.

[wrwrwr]

Aw, sorry, you're right I didn't define that integer primary key :-)

[jonashaag]

I find those existing models a bit hard to use for debugging type conversions -- they are rather big and even creating a single object often causes a ton of value_to_db_* or convert_value_for/from_db calls to happen.

That's true. Maybe we could split up the models into smaller ones.

[gpitfield]

Re: moving it to get_db_prep_save, will just need to confirm that function is called during update(). If so, then it seems to me that's a fine place to put it. In terms of the tests, do you want me to do that or did wrwrwr already commit the test above?

[wrwrwr]

Updated the test a bit (I have it marked as an expectedFailure at the moment, so the test needs testing :-) If you have a spare moment than please move models to the top of the file or maybe use some existing (generally this is better because the models are shared between test cases anyway; splitting models into smaller parts is also a good idea).

[jonashaag]

@gpitfield these tests look great so I think you're done (assuming the tests pass :-)

Wojtek, it's up to you to commit this patch, I don't want to mess with your other changes/branches.

[gpitfield]

Cool. One thing, on the tests I think it would make more sense if you switched Parent/Child.

And here endeth my first ever contribution to open source. That was fun! Hopefully more to come...

[jonashaag]

That was fun! Hopefully more to come...

We have tons of open tasks/issues so if you feel like doing some coding/documenting/supporting/whatever, just grab whatever task sounds most fun to you :-)

[gpitfield]

Where would I find the list?

[jonashaag]

If you like fixing stuff:

https://github.com/django-nonrel/djangotoolbox/issues
https://github.com/django-nonrel/djangoappengine/issues
https://github.com/django-nonrel/django-dbindexer/issues
https://github.com/django-nonrel/django-nonrel/issues
https://github.com/django-nonrel/mongodb-engine/issues

If you like writing docs: Get in touch with @markusgattol on IRC/mailing list

[wrwrwr]

Changing get_db_prep_value to get_db_prep_save results in one test failing due to an int / long mismatch:

    def test_foreignkey_in_embedded_object(self):
        class A(models.Model):
            pass
        class FK(models.Model):
            fk = models.ForeignKey(A)
        class E(models.Model):
            efk = EmbeddedModelField(FK)
        e = E.objects.create(efk=FK(fk=A.objects.create()))
        efk = E.objects.get().efk
        self.assertNotIn('fk', efk.__dict__)
        self.assertIsInstance(efk.__dict__['fk_id'], type(e.id))
        # ^ failure: efk.__dict__['fk_id'] is an int, while e.id is a long
        self.assertIsInstance(efk.fk, A)

I think it's OK to loosen the test a little bit -- the mismatch is due to ForeignKey having a no-op get_db_prep_value (inherited from Field), but defining get_db_prep_save (that calls the method on the related field, which in case of an AutoField results in passing the value through value_to_db_auto in the end). My feeling is that it would actually be good, if the value would go through the same conversion as a value of the pointed to field when it's being saved; what do you think?

[jonashaag]

This whole system is screwed. We need to invent a new, sane set of methods for type conversion and try to get that into Django vanilla.

[wrwrwr]

Committed the original patch together with the test, thanks again.

Parent / child: changed; amusing that if I write the same thing using foreign keys it feels natural the other way.

Type conversion methods / test failure: the test turned out to work again after the "deconvert-new-keys-as-other-values" fix, so no changes were necessary (and this only proves that these tests have good coverage :-) I have some preliminary thoughts on a fairly small set of changes to Django that would make the database-value-casting framework lend itself better to nonrel (very roughly -- by giving the back-end, rather than the field, the last word and moving SQL-specific code from Field), but this requires a wider discussion.