Problem with GenericRelations on TranslationModel

[orf]

Hey,
We are using hvad with DRF and it's amazing, thanks for the library and your time in making it.

We've run into a small issue when updating to the new release. One of our models has a GenericRelation to an AuditLog model: audit_logs = GenericRelation(AuditLog), and with the new release it triggers the following error when saving:

  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/rest_framework/mixins.py", line 74, in perform_update
    serializer.save()
  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/rest_framework/serializers.py", line 186, in save
    self.instance = self.update(self.instance, validated_data)
  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/hvad/contrib/restframework/serializers.py", line 239, in update
    return super(TranslatableModelMixin, self).update(instance, data)
  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/hvad/contrib/restframework/serializers.py", line 124, in update
    self.update_translation(instance, translation_data)
  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/hvad/contrib/restframework/serializers.py", line 144, in update_translation
    instance.save(update_fields=fields)
  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/hvad/models.py", line 278, in save
    translation.save(*args, **tkwargs)
  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/django_fsm/__init__.py", line 482, in save
    super(ConcurrentTransitionMixin, self).save(*args, **kwargs)
  File "/home/tom/.virtualenvs/doge/lib/python3.4/site-packages/django/db/models/base.py", line 680, in save
    % ', '.join(non_model_fields))
ValueError: The following fields do not exist in this model or are m2m fields: audit_logs

As far as I can tell the problem lies within the update_translation method in restframework/serializers.py:

fields = [name for name in self.Meta.model._meta.translations_model._meta.get_all_field_names()
                      if name not in ('id', 'master', 'master_id', 'language_code')]

This returns audit_logs in the list, but I think it should be excluded, as it's not an actual field on the model that can be updated.

[spectras]

Hello,
Thanks for reporting. May I ask which release of which packages you updated? It might help getting a better grasp of why it stopped working.
Could you also post the part of the model that causes this issue? From the trace, it looks like audit_logs appears as a translated field, which should not normally happen. I'm a bit confused and I might have to build a small model that shows the same error.

[orf]

My apologies, the original report was slightly wrong/misleading. We have a requirement to have an audit log for each translated model, so I added the following class (the QAMixin is also needed for compatibility with django-fsm, which does some funky stuff):

class QATranslatedFields(TranslatedFields):
    def _scan_model_bases(self, model):
        bases, fields = super()._scan_model_bases(model)
        bases = [QAMixin, AuditMixin] + bases
        return bases, fields

The abstract AuditMixin has the audit_logs = GenericRelation(AuditLog) field, which gets included on the translated model. This worked fine in the previous release but is broken in the current one.

Obviously this isn't something you supported directly, but it did function perfectly. If you would like I can try and whip up a merge request to support this, but I'm not too sure where to start (other than perhaps filtering out any m2m/generic fields in the update_translation function?)

If this isn't a use case you want to support then that's fine, I can modify our code to not do it this way.

[spectras]

Ok, I now understand how it gets there. So, this means, in your audit log, you have entries that actually point at a specific translation instead of the generic model.

This is indeed something that's not supported, because the main concept of hvad is it does not actually appear in the database schema, but acts as a drop-in replacement for field values. It supports a “display language”, which has no explicity identity. This is the reason creating foreign keys to the translations model is not supported, and the translations model is mostly hidden as an implementation detail.

Now, it is true that when you use hvad in a project and you have a single instance where you want to interact with translations, it can make sense to “misuse” hvad a bit. Usually, when possible, I would recommend doing that by:

• keying to the main model (by putting the AuditMixin on the main model)
• and adding a language_code field to the audit entries.

This does have the drawback of requiring the audit model to be aware of the language. I understand how this can be problematic in some cases.

Anyway, I traced your issue, it comes from the optimizations that were added in the latest version of hvad:

• The update_fields parameter is now supported in TranslatableModel.save. It used to be just passed around for django to handle it, but now hvad can detect when only translated or only untranslated fields are being saved, and avoid updating the two tables when that happens.
• The serializer now builds and passes a list of fields to update_fields when it updates translations.

This dramatically cuts down on the number of SQL requests: before, if you updated 5 translations in a DRF serializer, 10 SQL requests were sent (the untranslatable fields were updated 5 times). Now, only 6 requests are sent (1 for untranslatable fields and 1 per translation).

As translation models cannot normally have m2m fields, the update_translation method does not filter them out. Thus your problem. Should not be too hard to fix, and will improve compatibility with some of the edge case hvad is put to use in. It needs to update all fields, but only pass regular fields to update_fields.

A proper fix will require building a sample testcase with failing test, which might actually be trickier than the fix itself. I cannot do it right now, but I should be able to spend some time on this in the weekend.

[orf]

Thanks for the great reply, I'll re-factor the AuditLog to have a language code and stop abusing hvad like this :)

[spectras]

If you can do so, then definitely do. That will be much less trouble in the long run.
I still built a fix for this specific issue, so it should be gone on master.

I think this distinction of language as in “translating some field values” and language as in “semantically meaningful part of the database schema” should go in the FAQ, as it's commonly overlooked, but has strong implications. I'll add that next time I update it.

Thanks for taking the time to report your issue :)