Fix AttributeProperty access with mutable default. Resolve #3488

CHANGELOG.md

@@ -14,10 +14,11 @@
 - [Fix][pull3485]: Typo in `sethome` message (chiizujin)
 - [Fix][pull3487]: Fix traceback when using `get`,`drop` and `give` with no
   arguments (InspectorCaracal)
-- [Fix][issue3476]: Don't ignore EvEditor commands with wrong capitalization
+- [Fix][issue3476]: Don't ignore EvEditor commands with wrong capitalization (Griatch)
-  (Griatch)
 - [Fix][issue3477]: The `at_server_reload_start()` hook was not firing on
   a reload (regression).
+- [Fix][issue3488]: `AttributeProperty(<default>, autocreate=False)`, where
+  `<default>` was mutable would not update/save properly in-place (Griatch)
 - [Docs] Added new [Server-Lifecycle][doc-server-lifecycle] page to describe
   the hooks called on server start/stop/reload (Griatch)
 - [Docs] Doc typo fixes (Griatch, chiizujin)
@@ -27,6 +28,7 @@
 [pull3487]: https://github.com/evennia/evennia/pull/3487
 [issue3476]: https://github.com/evennia/evennia/issues/3476
 [issue3477]: https://github.com/evennia/evennia/issues/3477
+[issue3488]: https://github.com/evennia/evennia/issues/3488
 [doc-server-lifecycle]: https://www.evennia.com/docs/latest/Concepts/Server-Lifecycle.html
 
 

evennia/objects/tests.py

@@ -3,12 +3,8 @@ from unittest import skip
 from evennia import DefaultCharacter, DefaultExit, DefaultObject, DefaultRoom
 from evennia.objects.models import ObjectDB
 from evennia.typeclasses.attributes import AttributeProperty
-from evennia.typeclasses.tags import (
+from evennia.typeclasses.tags import (AliasProperty, PermissionProperty,
-    AliasProperty,
+                                      TagCategoryProperty, TagProperty)
-    PermissionProperty,
-    TagCategoryProperty,
-    TagProperty,
-)
 from evennia.utils import create, search
 from evennia.utils.test_resources import BaseEvenniaTest, EvenniaTestCase
 
@@ -356,6 +352,10 @@ class TestObjectPropertiesClass(DefaultObject):
     attr2 = AttributeProperty(default="attr2", category="attrcategory")
     attr3 = AttributeProperty(default="attr3", autocreate=False)
     attr4 = SubAttributeProperty(default="attr4")
+    attr5 = AttributeProperty(default=list, autocreate=False)
+    attr6 = AttributeProperty(default=[None], autocreate=False)
+    attr7 = AttributeProperty(default=list)
+    attr8 = AttributeProperty(default=[None])
     cusattr = CustomizedProperty(default=5)
     tag1 = TagProperty()
     tag2 = TagProperty(category="tagcategory")
@@ -541,3 +541,99 @@ class TestProperties(EvenniaTestCase):
 
         obj1.delete()
         obj2.delete()
+
+    def test_not_create_attribute_with_autocreate_false(self):
+        """
+        Test that AttributeProperty with autocreate=False does not create an attribute in the database.
+
+        """
+        obj = create.create_object(TestObjectPropertiesClass, key="obj1")
+
+        self.assertEqual(obj.attr3, "attr3")
+        self.assertEqual(obj.attributes.get("attr3"), None)
+
+        self.assertEqual(obj.attr5, [])
+        self.assertEqual(obj.attributes.get("attr5"), None)
+
+        obj.delete()
+
+    def test_callable_defaults__autocreate_false(self):
+        """
+        Test https://github.com/evennia/evennia/issues/3488, where a callable default value like `list`
+        would produce an infinitely empty result even when appended to.
+
+        """
+        obj1 = create.create_object(TestObjectPropertiesClass, key="obj1")
+        obj2 = create.create_object(TestObjectPropertiesClass, key="obj2")
+
+        self.assertEqual(obj1.attr5, [])
+        obj1.attr5.append(1)
+        self.assertEqual(obj1.attr5, [1])
+
+        # check cross-instance sharing
+        self.assertEqual(obj2.attr5, [], "cross-instance sharing detected")
+
+
+    def test_mutable_defaults__autocreate_false(self):
+        """
+        Test https://github.com/evennia/evennia/issues/3488, where a mutable default value (like a
+        list `[]` or `[None]`) would not be updated in the database when appended to.
+
+        Note that using a mutable default value is not recommended, as the mutable will share the
+        same memory space across all instances of the class. This means that if one instance modifiesA
+        the mutable, all instances will be affected.
+
+        """
+        obj1 = create.create_object(TestObjectPropertiesClass, key="obj1")
+        obj2 = create.create_object(TestObjectPropertiesClass, key="obj2")
+
+        self.assertEqual(obj1.attr6, [None])
+        obj1.attr6.append(1)
+        self.assertEqual(obj1.attr6, [None, 1])
+
+        obj1.attr6[1] = 2
+        self.assertEqual(obj1.attr6, [None, 2])
+
+        # check cross-instance sharing
+        self.assertEqual(obj2.attr6, [None], "cross-instance sharing detected")
+
+        obj1.delete()
+        obj2.delete()
+
+    def test_callable_defaults__autocreate_true(self):
+        """
+        Test callables with autocreate=True.
+
+        """
+        obj1 = create.create_object(TestObjectPropertiesClass, key="obj1")
+        obj2 = create.create_object(TestObjectPropertiesClass, key="obj1")
+
+        self.assertEqual(obj1.attr7, [])
+        obj1.attr7.append(1)
+        self.assertEqual(obj1.attr7, [1])
+
+        # check cross-instance sharing
+        self.assertEqual(obj2.attr7, [])
+
+
+    def test_mutable_defaults__autocreate_true(self):
+        """
+        Test mutable defaults with autocreate=True.
+
+        """
+        obj1 = create.create_object(TestObjectPropertiesClass, key="obj1")
+        obj2 = create.create_object(TestObjectPropertiesClass, key="obj2")
+
+        self.assertEqual(obj1.attr8, [None])
+        obj1.attr8.append(1)
+        self.assertEqual(obj1.attr8, [None, 1])
+
+        obj1.attr8[1] = 2
+        self.assertEqual(obj1.attr8, [None, 2])
+
+        # check cross-instance sharing
+        self.assertEqual(obj2.attr8, [None])
+
+        obj1.delete()
+        obj2.delete()
+

evennia/typeclasses/attributes.py

@@ -12,11 +12,11 @@ which is a non-db version of Attributes.
 import fnmatch
 import re
 from collections import defaultdict
+from copy import copy
 
 from django.conf import settings
 from django.db import models
 from django.utils.encoding import smart_str
-
 from evennia.locks.lockhandler import LockHandler
 from evennia.utils.dbserialize import from_pickle, to_pickle
 from evennia.utils.idmapper.models import SharedMemoryModel
@@ -166,6 +166,7 @@ class AttributeProperty:
     """
 
     attrhandler_name = "attributes"
+    cached_default_name_template = "_property_attribute_default_{key}"
 
     def __init__(self, default=None, category=None, strattr=False, lockstring="", autocreate=True):
         """
@@ -207,21 +208,6 @@ class AttributeProperty:
         self._autocreate = autocreate
         self._key = ""
 
-    @property
-    def _default(self):
-        """
-        Tries returning a new instance of default if callable.
-
-        """
-        if callable(self.__default):
-            return self.__default()
-
-        return self.__default
-
-    @_default.setter
-    def _default(self, value):
-        self.__default = value
-
     def __set_name__(self, cls, name):
         """
         Called when descriptor is first assigned to the class. It is called with
@@ -230,17 +216,35 @@ class AttributeProperty:
         """
         self._key = name
 
+    def _get_and_cache_default(self, instance):
+        """
+        Get and cache the default value for this attribute. We make sure to convert any mutables
+        into _Saver* equivalent classes here and cache the result on the instance's AttributeHandler.
+
+        """
+        attrhandler = getattr(instance, self.attrhandler_name)
+        value = getattr(attrhandler, self.cached_default_name_template.format(key=self._key), None)
+        if not value:
+            if callable(self._default):
+                value = self._default()
+            else:
+                value = copy(self._default)
+            value = from_pickle(value, db_obj=instance)
+            setattr(attrhandler, self.cached_default_name_template.format(key=self._key), value)
+        return value
+
     def __get__(self, instance, owner):
         """
         Called when the attrkey is retrieved from the instance.
 
         """
-        value = self._default
+        value = self._get_and_cache_default(instance)
+
         try:
             value = self.at_get(
                 getattr(instance, self.attrhandler_name).get(
                     key=self._key,
-                    default=self._default,
+                    default=value,
                     category=self._category,
                     strattr=self._strattr,
                     raise_exception=self._autocreate,
@@ -250,7 +254,7 @@ class AttributeProperty:
         except AttributeError:
             if self._autocreate:
                 # attribute didn't exist and autocreate is set
-                self.__set__(instance, self._default)
+                self.__set__(instance, value)
             else:
                 raise
         return value