Merge lp:~niemeyer/storm/bug-620615 into lp:storm

[Gustavo Niemeyer]

That's a test and fix for bug #620615.

[Jamu Kakar (jkakar)]

[1]

+ def test_bug_620615(self):

Please rename this test and add a docstring:

    def test_lazy_value_preserved_with_subsequent_object_initialization(self):
        """
        If a lazy value has been modified on an object that is subsequently
        initialized from the database the lazy value is correctly preserved
        and the object is initialized properly. This tests the fix for the
        problem reported in bug #620615.
        """

[2]

+ - Fix bug #620615, which caused lazy expressions to cause following
+ loading of objects to explode if unflushed.

It took me a moment to understand this sentence. I think
s/following/subsequent/ would make it a touch better.

Nice work, +1! Thanks for pushing the fix quickly to unblock the 0.18
release process.

review: Approve

[Thomas Herve (therve)]

[1] The value of replace_unknown_lazy doesn't seem to make any tests fail. I understand that it's probably tricky to raise this exception, but can you explain me why the value is set differently in some cases?

[2] Previously the check was only done when keep_defined was True, now it's only done if keep_defines is False. Shouldn't it be done in both cases?

Thanks!

review: Needs Information

[Gustavo Niemeyer (niemeyer)]

@Jamu,

[1] [2]

Both done, thanks!

@Thomas,

> [1] The value of replace_unknown_lazy doesn't seem to make any tests fail. I
> understand that it's probably tricky to raise this exception, but can you
> explain me why the value is set differently in some cases?

Good point. I've added an additional test doing the counter logic by using
reload (the lazy value should be discarded).

> [2] Previously the check was only done when keep_defined was True, now it's
> only done if keep_defines is False. Shouldn't it be done in both cases?

It is actually done in both cases. Look at how "is_unknown_lazy" is tracked in both branches. The distinction is in how that fact is handled, which is precisely the bug fix.

=== modified file 'NEWS'
--- NEWS	2010-08-20 09:23:10 +0000
+++ NEWS	2010-10-14 21:03:42 +0000
@@ -10,6 +10,8 @@
 Bug fixes
 ---------
  - Make storm compatible with psycopg2 2.2 (bug #585704).
+ - Fix bug #620615, which caused lazy expressions to cause following
+   loading of objects to explode if unflushed.
 
 
 0.17 (2010-08-05)

=== modified file 'storm/store.py'
--- storm/store.py	2010-09-01 15:00:42 +0000
+++ storm/store.py	2010-10-14 21:03:42 +0000
@@ -314,7 +314,8 @@
                         default_tables=cls_info.table, limit=1)
         result = self._connection.execute(select)
         values = result.get_one()
-        self._set_values(obj_info, cls_info.columns, result, values)
+        self._set_values(obj_info, cls_info.columns, result, values,
+                         replace_unknown_lazy=True)
         self._set_clean(obj_info)
 
     def autoreload(self, obj=None):
@@ -711,7 +712,8 @@
             obj_info = get_obj_info(obj)
             obj_info["store"] = self
 
-            self._set_values(obj_info, cls_info.columns, result, values)
+            self._set_values(obj_info, cls_info.columns, result, values,
+                             replace_unknown_lazy=True)
 
             self._add_to_alive(obj_info)
             self._enable_change_notification(obj_info)
@@ -744,26 +746,28 @@
             func()
 
     def _set_values(self, obj_info, columns, result, values,
-                    keep_defined=False):
+                    keep_defined=False, replace_unknown_lazy=False):
         if values is None:
             raise LostObjectError("Can't obtain values from the database "
                                   "(object got removed?)")
         obj_info.pop("invalidated", None)
         for column, value in zip(columns, values):
             variable = obj_info.variables[column]
+            lazy_value = variable.get_lazy()
+            is_unknown_lazy = not (lazy_value is None or
+                                   lazy_value is AutoReload)
             if keep_defined:
-                if variable.is_defined():
+                if variable.is_defined() or is_unknown_lazy:
                     continue
-                lazy_value = variable.get_lazy()
-                if not (lazy_value is None or lazy_value is AutoReload):
-                    # This should *never* happen, because whenever we get
-                    # to this point it should be after a flush() which
-                    # updated the database with lazy values and then replaced
-                    # them by AutoReload.  Letting this go through means
-                    # we're blindly discarding an unknown lazy value and
-                    # replacing it by the value from the database.
-                    raise RuntimeError("Unexpected situation. "
-                                       "Please contact the developers.")
+            elif is_unknown_lazy and not replace_unknown_lazy:
+                # This should *never* happen, because whenever we get
+                # to this point it should be after a flush() which
+                # updated the database with lazy values and then replaced
+                # them by AutoReload.  Letting this go through means
+                # we're blindly discarding an unknown lazy value and
+                # replacing it by the value from the database.
+                raise RuntimeError("Unexpected situation. "
+                                   "Please contact the developers.")
             if value is None:
                 variable.set(value, from_db=True)
             else:

=== modified file 'tests/store/base.py'
--- tests/store/base.py	2010-09-01 09:02:41 +0000
+++ tests/store/base.py	2010-10-14 21:03:42 +0000
@@ -5150,6 +5150,24 @@
                           (30, "Title 10"),
                          ])
 
+    def test_bug_620615(self):
+        # Retrieve an object, fully loaded.
+        foo = self.store.get(Foo, 20)
+
+        # Build and retrieve a result set ahead of time, so that
+        # flushes won't happen when actually loading the object.
+        result = self.store.find(Foo, Foo.id == 20)
+
+        # Now, set an unflushed lazy value on an attribute.
+        foo.title = SQL("'New title'")
+
+        # Finally, get the existing object.
+        foo = result.one()
+
+        # We don't really have to test anything here, since the
+        # explosion happened above, but here it is anyway.
+        self.assertEquals(foo.title, "New title")
+
     def test_expr_values_with_columns(self):
         bar = self.store.get(Bar, 200)
         bar.foo_id = Bar.id+1

=== modified file 'tests/zope/testing.py'
--- tests/zope/testing.py	2010-08-31 08:33:09 +0000
+++ tests/zope/testing.py	2010-10-14 21:03:42 +0000
@@ -21,12 +21,11 @@
 import os
 import sys
 
-from pysqlite2.dbapi2 import IntegrityError
-
 from tests.helper import TestHelper
 from tests.zope import has_zope, has_testresources
 
 from storm.locals import create_database, Store, Unicode, Int
+from storm.exceptions import IntegrityError
 
 if has_zope and has_testresources:
     from storm.zope.schema import ZSchema