SectorLabs/django-localized-fields

Bogus type checking condition in LocalizedValueDescriptor.__get__, LocalizedValue._interpret_value may need recosideration

atleta opened this issue · 0 comments

The type check on line 56 in descriptor.py is bogus and it causes a few related issues. The main one being is that you can't set a value for a language not specified in settings.LANGUAGES, because it will be overwritten on the next attribute access. So e.g. if you have a model like this:

class MyModel(models.Model):
    content = LocalizedTextField()

and you don't have e.g. the 'nl' (Dutch) language enabled, then doing this:

instance = MyModel()
instance.content.set('nl', 'Yolo')

won't set (won't retain) the setting for nl. The reason is the following line in descriptor.py:

        if isinstance(value, dict):

Because LocalizedValue inherits from dict, this will keep reinitializing the field on every attribute access:

            attr = self.field.attr_class(value)
            instance.__dict__[self.field.name] = attr

This is problematic because it can e.g. break data migrations if you change the language settings (this is how I found out about this). Also, it's an unnecessary copy operation with an impact on performance.

the solution is to change the line with the condition to:

if not isinstance(value, LocalizedValue) and isinstance(value, dict)

(most of the time it will be a LocalizedValue, as on the first access we change it to be that.)

On a side not, LocalizedValue._interpret_value may not be doing the right thing either (that's the other side of the problem): changing the available languages in the settings is basically equivalent to changing the db schema. (Again, it could break migrations too.)