user: fix cache errors on first_name/last_name handling (fixes #31937)

first_name/last_name are not updated anymore inside
Attribute.set_value() but only through the Attributes object.

In User.save() we first compare current values of first/last_name before
resetting the corresponding AttributeValue, i.e. if the value does not
change we never reset the verified status.

A map cache of attributes values is kept in user._a2_attributes_cache to
reduce the number of queries when modifying attributes.

src/authentic2/custom_user/models.py

@@ -21,48 +21,50 @@ from .managers import UserManager, UserQuerySet
 
 
 @RequestCache
-def get_attributes():
-    return Attribute.objects.all()
+def get_attributes_map():
+    mapping = {}
+    for at in Attribute.all_objects.all():
+        mapping[at.id] = at
+        mapping[at.name] = at
+    return mapping
 
 
 class Attributes(object):
     def __init__(self, owner, verified=None):
-        super(Attributes, self).__setattr__('owner', owner)
-        super(Attributes, self).__setattr__('verified', verified)
-        attributes = get_attributes()
-        at_map = {attribute.name: attribute for attribute in attributes}
-        for attribute in attributes:
-            at_map[attribute.id] = attribute
-        super(Attributes, self).__setattr__('attributes', at_map)
-        values = {}
-        super(Attributes, self).__setattr__('values', values)
-        for atv in self.owner.attribute_values.all():
-            if verified and not atv.verified:
-                continue
-            try:
-                attribute = at_map[atv.attribute_id]
-            except KeyError:
-                continue
-            value = atv.to_python()
-            if attribute.multiple:
-                values.setdefault(attribute.name, []).append(value)
-            else:
-                values[attribute.name] = value
+        self.__dict__['owner'] = owner
+        self.__dict__['verified'] = verified
+        if not hasattr(self.owner, '_a2_attributes_cache'):
+            values = {}
+            setattr(self.owner, '_a2_attributes_cache', values)
+            for atv in self.owner.attribute_values.all():
+                attribute = get_attributes_map()[atv.attribute_id]
+                atv.attribute = attribute
+                values[attribute.name] = atv
+        self.__dict__['values'] = owner._a2_attributes_cache
 
     def __setattr__(self, name, value):
-        try:
-            attribute = self.attributes[name]
-        except KeyError:
-            raise AttributeError(name)
-        attribute.set_value(self.owner, value, verified=bool(self.verified))
-        self.values[name] = value
+        atv = self.values.get(name)
+        if atv:
+            atv.attribute.set_value(self.owner, value, verified=bool(self.verified), attribute_value=atv)
+        else:
+            attribute = get_attributes_map().get(name)
+            if not attribute:
+                raise AttributeError(name)
+            self.values[name] = attribute.set_value(self.owner, value, verified=bool(self.verified))
+
+        update_fields = ['modified']
+        if name in ['first_name', 'last_name']:
+            if getattr(self.owner, name) != value:
+                setattr(self.owner, name, value)
+                update_fields.append(name)
+        self.owner.save(update_fields=update_fields)
 
     def __getattr__(self, name):
-        try:
-            attribute = self.attributes[name]
-        except KeyError:
-            raise AttributeError(name)
-        return self.values.get(name, [] if attribute.multiple else None)
+        atv = self.values.get(name)
+        if atv:
+            if not self.verified or atv.verified == self.verified:
+                return atv.to_python()
+        return None
 
 
 class AttributesDescriptor(object):
@@ -70,10 +72,7 @@ class AttributesDescriptor(object):
         self.verified = verified
 
     def __get__(self, obj, objtype):
-        cache_name = '_attributes_verified_cache' if self.verified else '_attributes_cache'
-        if not hasattr(obj, cache_name):
-            setattr(obj, cache_name, Attributes(obj, verified=self.verified))
-        return getattr(obj, cache_name)
+        return Attributes(obj, verified=self.verified)
 
 
 class IsVerified(object):
@@ -261,18 +260,19 @@ class User(AbstractBaseUser, PermissionMixin):
         return d
 
     def save(self, *args, **kwargs):
-        sync = not(kwargs.pop('nosync', False))
+        update_fields = kwargs.get('update_fields')
         rc = super(User, self).save(*args, **kwargs)
-        if sync:
-            for attr_name in ('first_name', 'last_name'):
-                try:
-                    attribute = Attribute.objects.get(name=attr_name)
-                except Attribute.DoesNotExist:
-                    pass
-                else:
-                    if attribute.get_value(self) != getattr(self, attr_name, None):
-                        attribute.set_value(self, getattr(self, attr_name, None))
+        if not update_fields or not set(update_fields).isdisjoint(set(['first_name', 'last_name'])):
+            if self.attributes.first_name != self.first_name:
+                self.attributes.first_name = self.first_name
+            if self.attributes.last_name != self.last_name:
+                self.attributes.last_name = self.last_name
         return rc
 
     def can_change_password(self):
         return True
+
+    def refresh_from_db(self, *args, **kwargs):
+        if hasattr(self, '_a2_attributes_cache'):
+            del self._a2_attributes_cache
+        return super(User, self).refresh_from_db(*args, **kwargs)

src/authentic2/models.py

@@ -203,7 +203,7 @@ class Attribute(models.Model):
             except AttributeValue.DoesNotExist:
                 return kind['default']
 
-    def set_value(self, owner, value, verified=False):
+    def set_value(self, owner, value, verified=False, attribute_value=None):
         serialize = self.get_kind()['serialize']
         # setting to None is to delete
         if value is None:
@@ -213,6 +213,7 @@ class Attribute(models.Model):
         if self.multiple:
             assert isinstance(value, (list, set, tuple))
             values = value
+            avs = []
             for value in values:
                 content = serialize(value)
                 av, created = AttributeValue.objects.get_or_create(
@@ -225,27 +226,24 @@ class Attribute(models.Model):
                 if not created:
                     av.verified = verified
                     av.save()
+                avs.append(av)
+            return avs
         else:
             content = serialize(value)
-            av, created = AttributeValue.objects.get_or_create(
-                    content_type=ContentType.objects.get_for_model(owner),
-                    object_id=owner.pk,
-                    attribute=self,
-                    multiple=False,
-                    defaults={'content': content, 'verified': verified})
-            if not created:
+            if attribute_value:
+                av, created = attribute_value, False
+            else:
+                av, created = AttributeValue.objects.get_or_create(
+                        content_type=ContentType.objects.get_for_model(owner),
+                        object_id=owner.pk,
+                        attribute=self,
+                        multiple=False,
+                        defaults={'content': content, 'verified': verified})
+            if not created and (av.content != content or av.verified != verified):
                 av.content = content
                 av.verified = verified
                 av.save()
-
-        # if owner has a modified field, update it
-        try:
-            modified = owner.__class__._meta.get_field('modified')
-        except FieldDoesNotExist:
-            pass
-        else:
-            if getattr(modified, 'auto_now', False):
-                owner.save(update_fields=['modified'])
+            return av
 
     def natural_key(self):
         return (self.name,)
@@ -286,15 +284,6 @@ class AttributeValue(models.Model):
         return (self.content_type.natural_key(), self.owner.natural_key(),
                 self.attribute.natural_key())
 
-    def save(self, *args, **kwargs):
-        changed = False
-        if self.attribute.name in ('first_name', 'last_name'):
-            setattr(self.owner, self.attribute.name, self.to_python())
-            changed = True
-        if changed:
-            self.owner.save(nosync=True)
-        return super(AttributeValue, self).save(*args, **kwargs)
-
     class Meta:
         verbose_name = _('attribute value')
         verbose_name_plural = _('attribute values')

tests/test_user_model.py

@@ -86,3 +86,81 @@ def test_sync_first_name(db, settings):
 
     user.attributes.first_name = 'John Paul'
     assert user.attributes.first_name == 'John Paul'
+
+
+def test_save_does_not_reset_verified_attributes_first_name(db):
+    user = User.objects.create()
+    user.verified_attributes.first_name = 'John'
+
+    user = User.objects.get()
+    assert user.first_name == 'John'
+    assert user.is_verified.first_name
+    assert user.verified_attributes.first_name == 'John'
+    assert user.attributes.first_name == 'John'
+    user.save()
+
+    user = User.objects.get()
+    assert user.first_name == 'John'
+    assert user.is_verified.first_name
+    assert user.verified_attributes.first_name == 'John'
+    assert user.attributes.first_name == 'John'
+    user.first_name = 'Michel'
+    user.save()
+    assert not user.is_verified.first_name
+    assert user.verified_attributes.first_name is None
+    assert user.attributes.first_name == 'Michel'
+
+    user = User.objects.get()
+    assert user.first_name == 'Michel'
+    assert not user.is_verified.first_name
+    assert user.verified_attributes.first_name is None
+    assert user.attributes.first_name == 'Michel'
+    user.verified_attributes.first_name = 'John'
+    assert user.is_verified.first_name
+    assert user.verified_attributes.first_name == 'John'
+    assert user.attributes.first_name == 'John'
+
+    user = User.objects.get()
+    assert user.first_name == 'John'
+    assert user.is_verified.first_name
+    assert user.verified_attributes.first_name == 'John'
+    assert user.attributes.first_name == 'John'
+
+
+def test_save_does_not_reset_verified_attributes_last_name(db):
+    user = User.objects.create()
+    user.verified_attributes.last_name = 'John'
+
+    user = User.objects.get()
+    assert user.last_name == 'John'
+    assert user.is_verified.last_name
+    assert user.verified_attributes.last_name == 'John'
+    assert user.attributes.last_name == 'John'
+    user.save()
+
+    user = User.objects.get()
+    assert user.last_name == 'John'
+    assert user.is_verified.last_name
+    assert user.verified_attributes.last_name == 'John'
+    assert user.attributes.last_name == 'John'
+    user.last_name = 'Michel'
+    user.save()
+    assert not user.is_verified.last_name
+    assert user.verified_attributes.last_name is None
+    assert user.attributes.last_name == 'Michel'
+
+    user = User.objects.get()
+    assert user.last_name == 'Michel'
+    assert not user.is_verified.last_name
+    assert user.verified_attributes.last_name is None
+    assert user.attributes.last_name == 'Michel'
+    user.verified_attributes.last_name = 'John'
+    assert user.is_verified.last_name
+    assert user.verified_attributes.last_name == 'John'
+    assert user.attributes.last_name == 'John'
+
+    user = User.objects.get()
+    assert user.last_name == 'John'
+    assert user.is_verified.last_name
+    assert user.verified_attributes.last_name == 'John'
+    assert user.attributes.last_name == 'John'