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

Proposed by Gustavo Niemeyer on 2010-10-14
Status: Merged
Merged at revision: 377
Proposed branch: lp:~niemeyer/storm/bug-620615
Merge into: lp:storm
Diff against target: 124 lines (+39/-16)
4 files modified
NEWS (+2/-0)
storm/store.py (+18/-14)
tests/store/base.py (+18/-0)
tests/zope/testing.py (+1/-2)
To merge this branch: bzr merge lp:~niemeyer/storm/bug-620615
Reviewer Review Type Date Requested Status
Thomas Herve (community) 2010-10-14 Needs Information on 2010-10-17
Jamu Kakar (community) Approve on 2010-10-17
Review via email: mp+38459@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Jamu Kakar (jkakar) wrote :

[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) wrote :

[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) wrote :

@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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-08-20 09:23:10 +0000
3+++ NEWS 2010-10-14 21:03:42 +0000
4@@ -10,6 +10,8 @@
5 Bug fixes
6 ---------
7 - Make storm compatible with psycopg2 2.2 (bug #585704).
8+ - Fix bug #620615, which caused lazy expressions to cause following
9+ loading of objects to explode if unflushed.
10
11
12 0.17 (2010-08-05)
13
14=== modified file 'storm/store.py'
15--- storm/store.py 2010-09-01 15:00:42 +0000
16+++ storm/store.py 2010-10-14 21:03:42 +0000
17@@ -314,7 +314,8 @@
18 default_tables=cls_info.table, limit=1)
19 result = self._connection.execute(select)
20 values = result.get_one()
21- self._set_values(obj_info, cls_info.columns, result, values)
22+ self._set_values(obj_info, cls_info.columns, result, values,
23+ replace_unknown_lazy=True)
24 self._set_clean(obj_info)
25
26 def autoreload(self, obj=None):
27@@ -711,7 +712,8 @@
28 obj_info = get_obj_info(obj)
29 obj_info["store"] = self
30
31- self._set_values(obj_info, cls_info.columns, result, values)
32+ self._set_values(obj_info, cls_info.columns, result, values,
33+ replace_unknown_lazy=True)
34
35 self._add_to_alive(obj_info)
36 self._enable_change_notification(obj_info)
37@@ -744,26 +746,28 @@
38 func()
39
40 def _set_values(self, obj_info, columns, result, values,
41- keep_defined=False):
42+ keep_defined=False, replace_unknown_lazy=False):
43 if values is None:
44 raise LostObjectError("Can't obtain values from the database "
45 "(object got removed?)")
46 obj_info.pop("invalidated", None)
47 for column, value in zip(columns, values):
48 variable = obj_info.variables[column]
49+ lazy_value = variable.get_lazy()
50+ is_unknown_lazy = not (lazy_value is None or
51+ lazy_value is AutoReload)
52 if keep_defined:
53- if variable.is_defined():
54+ if variable.is_defined() or is_unknown_lazy:
55 continue
56- lazy_value = variable.get_lazy()
57- if not (lazy_value is None or lazy_value is AutoReload):
58- # This should *never* happen, because whenever we get
59- # to this point it should be after a flush() which
60- # updated the database with lazy values and then replaced
61- # them by AutoReload. Letting this go through means
62- # we're blindly discarding an unknown lazy value and
63- # replacing it by the value from the database.
64- raise RuntimeError("Unexpected situation. "
65- "Please contact the developers.")
66+ elif is_unknown_lazy and not replace_unknown_lazy:
67+ # This should *never* happen, because whenever we get
68+ # to this point it should be after a flush() which
69+ # updated the database with lazy values and then replaced
70+ # them by AutoReload. Letting this go through means
71+ # we're blindly discarding an unknown lazy value and
72+ # replacing it by the value from the database.
73+ raise RuntimeError("Unexpected situation. "
74+ "Please contact the developers.")
75 if value is None:
76 variable.set(value, from_db=True)
77 else:
78
79=== modified file 'tests/store/base.py'
80--- tests/store/base.py 2010-09-01 09:02:41 +0000
81+++ tests/store/base.py 2010-10-14 21:03:42 +0000
82@@ -5150,6 +5150,24 @@
83 (30, "Title 10"),
84 ])
85
86+ def test_bug_620615(self):
87+ # Retrieve an object, fully loaded.
88+ foo = self.store.get(Foo, 20)
89+
90+ # Build and retrieve a result set ahead of time, so that
91+ # flushes won't happen when actually loading the object.
92+ result = self.store.find(Foo, Foo.id == 20)
93+
94+ # Now, set an unflushed lazy value on an attribute.
95+ foo.title = SQL("'New title'")
96+
97+ # Finally, get the existing object.
98+ foo = result.one()
99+
100+ # We don't really have to test anything here, since the
101+ # explosion happened above, but here it is anyway.
102+ self.assertEquals(foo.title, "New title")
103+
104 def test_expr_values_with_columns(self):
105 bar = self.store.get(Bar, 200)
106 bar.foo_id = Bar.id+1
107
108=== modified file 'tests/zope/testing.py'
109--- tests/zope/testing.py 2010-08-31 08:33:09 +0000
110+++ tests/zope/testing.py 2010-10-14 21:03:42 +0000
111@@ -21,12 +21,11 @@
112 import os
113 import sys
114
115-from pysqlite2.dbapi2 import IntegrityError
116-
117 from tests.helper import TestHelper
118 from tests.zope import has_zope, has_testresources
119
120 from storm.locals import create_database, Store, Unicode, Int
121+from storm.exceptions import IntegrityError
122
123 if has_zope and has_testresources:
124 from storm.zope.schema import ZSchema

Subscribers

People subscribed via source and target branches

to status/vote changes: