queryset update() and ListField/EmbeddedModelField

[gpitfield]

I think there's an issue when calling update() on a queryset if the values in update() include a ListField with an EmbeddedModelField in it. Made-up example follows. The problem seems to be that the EmbeddedModelField's pre_save() function is not called during update(), only get_db_prep_value() is called, and the latter expects the field's embedded model instances to have already been parsed into a format suitable for conversion to BSON.

Example:

class EmbeddedModel(models.Model):
    a_field = models.IntegerField()

class Parent(models.Model):
    some_list_field = ListField(EmbeddedModelField(EmbeddedModel))

Now, let's say we have a new value of some_list_field for an instance of Parent, but don't want to save() the Parent (because we don't want to trigger auto-updated fields, it's a really big object, etc.). So you'd like to be able to do something like this:

Parent.objects.filter(id=some_id).update(some_list_field=new_list_field_value)

This breaks in djangotoolbox fields.py line 270 (get_db_prep_value), with the error

'EmbeddedModel' object is not iterable

My fix, admittedly a raging hack, is to check the type of the values passed to get_db_prep_value, and if they haven't been parsed, to perform the same parsing that pre_save() does.

def get_db_prep_value(self, value, **kwargs):
    # (embedded_instance, value_list) replaced by value above
    #### hack begins ####
    if isinstance(value, tuple):
        embedded_instance, value_list = value
    else:
        embedded_instance = value
        value_list = []
        for field in embedded_instance._meta.fields:
            add = not embedded_instance._entity_exists
            value = field.pre_save(embedded_instance, add)
            if field.primary_key and value is None:
                # exclude unset pks ({"id" : None})
                continue
            value_list.append((field, value))
     ### hack ends ###
    if value_list is None:
        return None
    values = dict((field.column, field.get_db_prep_value(value, **kwargs))
                  for field, value in value_list)
    if self.embedded_model is None:
        values.update({'_module' : embedded_instance.__class__.__module__,
                       '_model'  : embedded_instance.__class__.__name__})
    # This instance will exist in the db very soon.
    embedded_instance._entity_exists = True
    return values

What I can say in favor of this is that it works. However, what I'm not at all sure of is whether it would just be better to move that parsing step out of pre_save entirely, but I'm out of my league here and in particular don't know if there are other functions expecting the parsed values returned by pre_save().

So I don't know if this is a bug exactly, or if this fix has terrible side effects I've misseed, etc, but there you go. Thoughts?

[jonashaag]

Thanks for the report! I tried to work around this issue several times but didn't succeed. Honestly, I don't know what the right approach is. The whole tuple-passing thing is a hack already.

I don't know if it's worth introducing another hack working around limitations of the existing hack. It would be best to find a cleaner solution...

[gpitfield]

My issue is that this is way deeper into the guts of Django than I've previously had to deal with, and my knowledge of the internals at this level are limited. In particular, I don't fully grok what belongs in pre_save() vs get_db_prep_value(), and what happens in between those. I take it that it doesn't work if you move the tuple-making machine into get_db_prep_value? If it does work there, that would seem cleaner to me. If it doesn't, I think I'll just live with my meta-hack and move on, for now.

...and of course, if I think of a better answer, I'll let you know. But I'm not optimistic about the chances of that haha.

[emperorcezar]

In my opinion, the way the ORM is put together is a hack. In fact, I know Alex Gaynor, the guy who wrote most of it, and he agrees it's a hack and needs to be refactored/rewritten.

Anywho,

get_db_prep_value:

Some data types (for example, dates) need to be in a specific format before they can be used by a database backend. get_db_prep_value() is the method where those conversions should be made.

pre_save:

You only need to override this method if you want to preprocess the value somehow, just before saving. For example, Django's DateTimeField uses this method to set the attribute correctly in the case of auto_now or auto_now_add.

So it seems that the correct place for the value munging is in get_db_prep_value.

[gpitfield]

Interesting. I'd be happy to try moving it there, but are there tests that will confirm it doesn't break something? This is my first visit to the "contributing code" side of the open source equation, please let me know if/how I can help.

[jonashaag]

Yes there are tests, you should immediately notice if you broke something :-)

Edit: This issue belongs to djangotoolbox (never mind, it's okay to be filed here) so the tests there are probably worth the most.

[gpitfield]

Where does the source for djangotoolbox reside now? Is it still https://bitbucket.org/wkornewald/djangotoolbox/src? I thought that Waldemar et al had handed it off or something.

[emperorcezar]

https://github.com/django-nonrel/djangotoolbox

[gpitfield]

Cool. I'm going to re-log this on that repo just so it's where it belongs, and close this one (if I can).