Merge lp:~free.ekanayaka/storm/infoheritance into lp:storm

[Free Ekanayaka]

This branch adds polymorphism support to References. See the NEWS file and the Reference docstring for an overview.

In a nutshell: if poly_type is passed to Reference.__init__, it will be treated as a column on the local object that holds an integer value identifying the type of the remote object. A Reference instance built with a non-None poly_type keeps a (lazily built) map of those integer values to the specific Relation objects to use for them.

[Gustavo Niemeyer (niemeyer)]

FWIW, we had something like this in the initial versions of Storm, before the
project was made public.

We actually used this pattern in Landscape itself for some time, and it created
code that is more involved and thus harder to understand, and we were able to
map the same model to a simpler and much cleaner version that was more explicit
without using that kind of trick, and decided to kill the feature because it
clearly encouraged unnecessary complexity.

I'd resist adding something like that, and would instead attempt to simplify
the object model.

I'll abstain from voting it, though, and will relay to others, but I have a
strong feeling you'll regret having invented this after you use it for some
time in real code.

[Free Ekanayaka (free.ekanayaka)]

Just to clarify, what do you mean by simplifying the object model? Not using the infoheritance pattern at all?

[Gustavo Niemeyer (niemeyer)]

I mean making things straightforward, non-fancy, no magic.

[Free Ekanayaka (free.ekanayaka)]

I still don't understand your view. Specifically, I don't get whether you are suggesting to not use the infoheritance pattern at all by arranging your model objects differently, OR to use it but implement it with the non-fancy no-magic way described in the Storm website in the infohertiance page.

What I think is that if we are fine with the infoheritance pattern, or we don't have really sound alternatives (as it's the case with Landscape atm, since we are still using that pattern), then I believe there's nothing too bad in implementing support for it in Storm, which at the end of the day just move the very same non-fancy no-magic user code/logic into Storm, for convenience and sake of code re-use.

[Gustavo Niemeyer (niemeyer)]

Yes, I mean making your model match the database content, which is what the "infoheritance" page
demonstrates. The model documented there is the outcome of precisely getting _away_ from internal
support in Storm for what you're adding here, since it was confusing and unnecessarily complex.

You're going backwards in history. Check out old revisions of Landscape and Storm and you'll
notice we've been there, and regretted.

[Gustavo Niemeyer (niemeyer)]

review: Disapprove

[Free Ekanayaka (free.ekanayaka)]

Okay, thanks for explaining, I'm heading off for the day but will check the old revisions tomorrow. Not sure if it makes any difference, and maybe it goes without saying, but note that the branch implements exactly the infoheritance approach, where you have a separate "child" class referenced by the parent and holding extra information, and does *not* introduce polymorphism at the class-hierarchy level (the OOP-style), which is for instance the way this implemented in SQLAlchemy. So in practice I'd say it doesn't introduce further abstractions with respect to the infohertiance approach.

[Thomas Herve (therve)]

I think there may be some value to extract the infoheritance pattern into some piece of code, for reusability/testability. But, I don't think modifying Reference is the way to go, as it's already a fairly complicated class (and poorly understood, at least by me). Maybe using composition would make more sense.

[Gustavo Niemeyer (niemeyer)]

Free, the beauty of the suggested pattern and the reason we have a page explaining it is precisely
that there's nothing special about it in terms of Storm itself. Storm offers simple building
blocks and it's up to people to put the pattern in place in a way that suits them, possibly with
variations that were not foreseen by us. Hacking that into Storm itself indeed feels like a bad idea.

[Free Ekanayaka (free.ekanayaka)]

Thanks Gustavo and Thomas for the feedback. I understand your points they make sense to me. Maybe we could for composition like suggested by Thomas, something that doesn't modify the core of Storm at all, but simply plugs to it. It could be even an external package.

For now, I'm withdrawing the MP.

Unmerged revisions

420. By Free Ekanayaka on 2011-10-25

Add polymorphic support to Reference

=== modified file 'NEWS'
--- NEWS	2011-10-17 18:30:27 +0000
+++ NEWS	2011-10-25 10:34:20 +0000
@@ -4,6 +4,51 @@
 Improvements
 ------------
 
+- The Reference class now supports polymorphism, by linking to remote objects of
+  different types and from  different tables. Its new "poly_type" constructor
+  parameter is used to indicate a column on the local object whose value
+  identifies the type of remote object. This feature can be used to implement
+  the infoheritance pattern, for example:
+
+    class Person(object):
+
+        __storm_table__ = "person"
+        id = Int(allow_none=False, primary=True)
+        name = Unicode(allow_none=False)
+        info_type = Int(allow_none=False)
+        info = Reference(id, "person_id", on_remote=True, poly_type=info_type)
+
+        def __init__(self, name, info_class, **kwargs):
+            self.name = name
+
+    class PersonInfo(object):
+
+        person_id = Int(allow_none=False, primary=True)
+        person = Reference(person_id, Person.id)
+
+        def __init__(self, person):
+            self.person = person
+
+    @poly_type(1, on=Person.info)
+    class SecretAgent(PersonInfo):
+
+        __storm_table__ = "secret_agent"
+        passcode = Unicode(allow_none=False)
+
+        def __init__(self, person, passcode=None):
+            super(SecretAgent, self).__init__(person)
+            self.passcode = passcode
+
+    @poly_type(2, on=Person.info)
+    class Teacher(PersonInfo):
+
+        __storm_table__ = "teacher"
+        school = Unicode(allow_none=False)
+
+        def __init__(self, person, school=None):
+            super(Teacher, self).__init__(person)
+            self.school = school
+
 Bug fixes
 ---------
 

=== modified file 'storm/references.py'
--- storm/references.py	2010-06-01 08:33:33 +0000
+++ storm/references.py	2011-10-25 10:34:20 +0000
@@ -111,7 +111,7 @@
     # warnings!).
     _relation = LazyAttribute("_relation", "_build_relation")
 
-    def __init__(self, local_key, remote_key, on_remote=False):
+    def __init__(self, local_key, remote_key, on_remote=False, poly_type=None):
         """
         Create a Reference property.
 
@@ -124,10 +124,15 @@
         @param on_remote: If specified, then the reference is
             backwards: It is the C{remote_key} which is a foreign key
             onto C{local_key}.
+        @param poly_type: If specified, the sibling column that is used
+            to determine the type of the referred-to object. In this case,
+            the C{remote_key} parameter must be a C{str} which will resolve
+            to the matching column of the referred-to type.
         """
         self._local_key = local_key
         self._remote_key = remote_key
         self._on_remote = on_remote
+        self._poly_type = poly_type
         self._cls = None
 
     def __get__(self, local, cls=None):
@@ -141,27 +146,34 @@
         if local is None:
             return self
 
-        remote = self._relation.get_remote(local)
+        if isinstance(self._relation, Relation):
+            relation = self._relation
+        else:
+            # This means the reference is polymorfic and we have to select
+            # the relation according to the poly type
+            relation = self._select_relation(local)
+
+        remote = relation.get_remote(local)
         if remote is not None:
             return remote
 
-        if self._relation.local_variables_are_none(local):
+        if relation.local_variables_are_none(local):
             return None
 
         store = Store.of(local)
         if store is None:
             return None
 
-        if self._relation.remote_key_is_primary:
-            remote = store.get(self._relation.remote_cls,
-                               self._relation.get_local_variables(local))
+        if relation.remote_key_is_primary:
+            remote = store.get(relation.remote_cls,
+                               relation.get_local_variables(local))
         else:
-            where = self._relation.get_where_for_remote(local)
-            result = store.find(self._relation.remote_cls, where)
+            where = relation.get_where_for_remote(local)
+            result = store.find(relation.remote_cls, where)
             remote = result.one()
 
         if remote is not None:
-            self._relation.link(local, remote)
+            relation.link(local, remote)
 
         return remote
 
@@ -172,33 +184,97 @@
         if self._cls is None:
             self._cls = _find_descriptor_class(local.__class__, self)
 
+        if isinstance(self._relation, Relation):
+            relation = self._relation
+        else:
+            # This means the reference is polymorfic and we have to select
+            # the relation according to the poly type
+            relation = self._select_relation(local, remote)
+
         if remote is None:
             if self._on_remote:
                 remote = self.__get__(local)
                 if remote is None:
                     return
             else:
-                remote = self._relation.get_remote(local)
+                remote = relation.get_remote(local)
             if remote is None:
                 remote_info = None
             else:
                 remote_info = get_obj_info(remote)
-            self._relation.unlink(get_obj_info(local), remote_info, True)
+                if self._poly_type:
+                    # We also need to reset the poly type
+                    local_info = get_obj_info(local)
+                    local_info.variables[self._poly_column].set(None)
+            relation.unlink(get_obj_info(local), remote_info, True)
         else:
             # Don't use remote here, as it might be
             # security proxied or something.
             try:
                 remote = get_obj_info(remote).get_obj()
             except ClassInfoError:
-                pass # It might fail when remote is a tuple or a raw value.
-            self._relation.link(local, remote, True)
+                pass  # It might fail when remote is a tuple or a raw value.
+            relation.link(local, remote, True)
 
     def _build_relation(self):
         resolver = PropertyResolver(self, self._cls)
         self._local_key = resolver.resolve(self._local_key)
-        self._remote_key = resolver.resolve(self._remote_key)
-        self._relation = Relation(self._local_key, self._remote_key,
-                                  False, self._on_remote)
+        if self._poly_type is None:
+            self._remote_key = resolver.resolve(self._remote_key)
+            self._relation = Relation(self._local_key, self._remote_key, False,
+                                      self._on_remote)
+        else:
+            # The reference is polymorfic, we will build relations lazily,
+            # saving them in a poly_type -> poly_class mapping
+            self._relation = {}
+            if type(self._remote_key) is not tuple:
+                self._remote_key = tuple((self._remote_key,))
+            self._poly_column = resolver.resolve_one(self._poly_type)
+
+    def _select_relation(self, local, remote=None):
+        """
+        Select the correct L{Relation} object for the polymorphic type at hand,
+        building if it's the first time we hit this type.
+        """
+        local_info = get_obj_info(local)
+        local_poly_type = local_info.variables[self._poly_column].get()
+        if remote is None:
+            # We're being called from __get__, as poly type we use the value of
+            # the given poly_type column on the local object
+            poly_type = local_poly_type
+        else:
+            # We're being called from __set__, as the poly type we use the
+            # value stored in the remote obect's class by register_poly_type
+            remote_poly_type = getattr(remote.__class__,
+                                       self._poly_column.name)
+            # Set the poly_type column of the local object to the poly
+            # type value found in the remote object's class
+            local_info.variables[self._poly_column].set(remote_poly_type)
+            poly_type = remote_poly_type
+
+        relation = self._relation.get(poly_type)
+
+        if relation is None:
+            # The relation for the given poly type is not in the map, build
+            # it now
+            if remote is None:
+                remote_class = self._poly_lookup[poly_type]
+            else:
+                remote_class = remote.__class__
+
+            if hasattr(remote_class, "__storm_table__"):
+                # The remote object is a Storm object, so build a regular
+                # relation
+                resolver = PropertyResolver(self, self._cls)
+                remote_key = resolver.resolve(
+                    tuple(getattr(remote_class, k) for k in self._remote_key))
+                relation = Relation(self._local_key, remote_key, False,
+                                    self._on_remote)
+            else:
+                relation = RemoteFactory(remote_class, self._poly_column)
+
+            self._relation[poly_type] = relation
+        return relation
 
     def __eq__(self, other):
         return self._relation.get_where_for_local(other)
@@ -892,6 +968,26 @@
             return self._r_to_l.setdefault(local_cls, map).get(remote_column)
 
 
+class RemoteFactory(object):
+    """Factory for in-memory remote object references."""
+
+    def __init__(self, remote_class, poly_column):
+        self._remote_class = remote_class
+        self._local_attr = "__storm_poly_remote_%s_" % poly_column.name
+
+    def get_remote(self, local):
+        remote = getattr(local, self._local_attr, None)
+        if remote is None:
+            remote = self._remote_class.__new__(self._remote_class)
+            self.link(local, remote, False)
+        return remote
+
+    def link(self, local, remote, setting):
+        setattr(local, self._local_attr, remote)
+        if not setting and getattr(remote, "link", None) is not None:
+            remote.link(local)
+
+
 class PropertyResolver(object):
     """Transform strings and pure properties (non-columns) into columns."""
 
@@ -943,3 +1039,50 @@
             if _descr is descr:
                 return getattr(cls, attr)
     raise RuntimeError("Reference used in an unknown class")
+
+
+def register_poly_type(cls, type, on):
+    """Register a new type for a polymorfic reference.
+
+    @param cls: The class of the new type to register.
+    @param type: A unique identifier for the class, usually an integer value.
+    @param on: The polymorfic L{Reference} to associated the type with.
+    """
+    if getattr(on, "_poly_lookup", None) is None:
+        on._poly_lookup = {}
+    if type in on._poly_lookup:
+        raise RuntimeError("poly type already registered")
+    on._poly_lookup[type] = cls
+    for attr, value in on._cls.__dict__.iteritems():
+        if value is on._poly_type:
+            setattr(cls, attr, type)
+            break
+    else:
+        raise RuntimeError("parent attr not found")
+
+
+def unregister_poly_type(cls, on):
+    """Unregister a type of a polymorphic reference."""
+    for attr, value in on._cls.__dict__.iteritems():
+        if value is on._poly_type:
+            del on._poly_lookup[getattr(cls, attr)]
+            break
+    else:
+        raise RuntimeError("parent attr not found")
+
+
+def unregister_poly_types(on):
+    """Unregister all types on the given polymorfic reference."""
+    on._poly_lookup.clear()
+
+
+def poly_type(type, on):
+    """
+    Decorator used to register a class as a type for a polymorfic reference.
+    """
+
+    def register(cls):
+        register_poly_type(cls, type, on)
+        return cls
+
+    return register

=== modified file 'tests/store/base.py'
--- tests/store/base.py	2011-08-01 12:54:36 +0000
+++ tests/store/base.py	2011-10-25 10:34:20 +0000
@@ -27,7 +27,9 @@
 from uuid import uuid4
 import weakref
 
-from storm.references import Reference, ReferenceSet, Proxy
+from storm.references import (
+    Reference, ReferenceSet, Proxy, poly_type, register_poly_type,
+    unregister_poly_type)
 from storm.database import Result, STATE_DISCONNECTED
 from storm.properties import (
     Int, Float, RawStr, Unicode, Property, Pickle, UUID)
@@ -125,6 +127,32 @@
     foo = Reference(foo_id, Foo.id)
     foo_title = Proxy(foo, Foo.title)
 
+
+class Egg(object):
+    __storm_table__ = "egg"
+    id = Int(primary=True, default=AutoReload)
+    info_type = Int()
+    info = Reference(id, "egg_id", on_remote=True, poly_type=info_type)
+
+    def __init__(self, info_class=None):
+        if info_class is not None:
+            self.info = info_class()
+
+@poly_type(1, on=Egg.info)
+class StoredEggInfo(object):
+    __storm_table__ = "stored_egg_info"
+    egg_id = Int(primary=True)
+    egg = Reference(egg_id, Egg.id)
+
+@poly_type(2, on=Egg.info)
+class MemoryEggInfo(object):
+    def __init__(self):
+        self.created = True
+
+    def link(self, egg):
+        self.created = False
+        self.egg = egg
+
 class Money(object):
     __storm_table__ = "money"
     id = Int(primary=True)
@@ -267,7 +295,7 @@
 
     def drop_tables(self):
         for table in ["foo", "bar", "bin", "link", "money", "selfref",
-                      "foovalue", "unique_id"]:
+                      "foovalue", "unique_id", "egg", "stored_egg_info"]:
             try:
                 self.connection.execute("DROP TABLE %s" % table)
                 self.connection.commit()
@@ -5756,6 +5784,163 @@
             self.assertEquals(bar.foo_id, 20)
             self.assertEquals(link.foo_id, 20)
 
+    def test_poly_register(self):
+        """
+        It's possible to register new polymorphic types using the C{poly_type}
+        decorator.
+        """
+        self.assertEqual(1, StoredEggInfo.info_type)
+        self.assertEqual(2, MemoryEggInfo.info_type)
+
+    def test_poly_register_with_duplicate_type(self):
+        """
+        An error is raised when trying to register the same info type twice
+        for the same Reference.
+        """
+
+        class OtherEggInfo(object):
+            pass
+
+        self.assertRaises(RuntimeError, register_poly_type, OtherEggInfo,
+                          MemoryEggInfo.info_type, Egg.info)
+
+    def test_poly_unregister(self):
+        """
+        It's possible to unregister a poly type.
+        """
+
+        class OtherEggInfo(object):
+            pass
+
+        register_poly_type(OtherEggInfo, 3, Egg.info)
+        self.assertEqual(3, OtherEggInfo.info_type)
+        self.assertIs(OtherEggInfo, Egg.info._poly_lookup[3])
+        unregister_poly_type(OtherEggInfo, Egg.info)
+        self.assertIs(None, Egg.info._poly_lookup.get(3))
+
+
+    def test_poly_in_memory_init(self):
+        """
+        In-memory remote objects are cached while the local object is alive,
+        but when they're recreated, their C{__init__} should not be called
+        again.
+        """
+        egg = Egg()
+        egg.info = MemoryEggInfo()
+        self.store.add(egg)
+        self.store.flush()
+        egg_id = egg.id
+        self.assertTrue(egg.info.created)
+        self.store.invalidate(egg)
+        del egg
+        gc.collect()
+        egg = self.store.get(Egg, egg_id)
+        self.assertFalse(egg.info.created)
+        self.assertIs(egg, egg.info.egg)
+
+    def test_poly_wb_relations_are_built_lazily(self):
+        """
+        The relations for the registered poly types are built lazily.
+        """
+        Egg.info._relation.clear()
+        egg = Egg()
+        egg.info = StoredEggInfo()
+        self.assertEqual([1], Egg.info._relation.keys())
+        egg.info = MemoryEggInfo()
+        self.assertEqual(sorted([1, 2]), sorted(Egg.info._relation.keys()))
+
+    def test_poly_with_type_override(self):
+        """
+        When the local and remote poly type values don't match, the local one
+        is overriden when setting the reference.
+        """
+        egg = Egg()
+        egg.info_type = 9999
+        egg.info = StoredEggInfo()
+        self.assertEqual(StoredEggInfo.info_type, egg.info_type)
+
+    def test_poly_create_in_memory_object(self):
+        """
+        When setting the local reference to a new in-memory remote object, the
+        C{link} method of the remote object is not invoked.
+        """
+        egg = Egg(MemoryEggInfo)
+        self.store.add(egg)
+        self.store.flush()
+        self.assertEqual(MemoryEggInfo.info_type, egg.info_type)
+        self.assertTrue(egg.info.created)
+
+    def test_poly_link_in_memory_remote(self):
+        """
+        If the remote object is an in memory object, its C{link} method is
+        invoked when it's linked to a Storm object loaded from the store.
+        """
+        egg = Egg()
+        egg.info_type = MemoryEggInfo.info_type
+        self.store.add(egg)
+        self.store.flush()
+        self.assertFalse(egg.info.created)
+        self.assertIs(egg, egg.info.egg)
+
+    def test_poly_create_stored_remote(self):
+        """
+        When a setting the local reference to a remote object, the remote
+        key is set to the local key.
+        """
+        egg = Egg()
+        egg_info = StoredEggInfo()
+        egg.info = egg_info
+        self.store.add(egg)
+        self.store.flush()
+        self.assertEqual(StoredEggInfo.info_type, egg.info_type)
+        self.assertEqual(egg.id, egg_info.egg_id)
+        self.assertIs(egg, egg_info.egg)
+        self.assertEqual(egg.id, egg_info.egg.id)
+
+    def test_poly_link_stored_remote(self):
+        """
+        When the remote object is a Storm object, the local key value is used
+        to find the matching remote key.
+        """
+        egg = Egg()
+        egg.id = 1234
+        egg.info_type = StoredEggInfo.info_type
+        egg_info1 = StoredEggInfo()
+        egg_info1.egg_id = egg.id
+        egg_info2 = StoredEggInfo()
+        egg_info2.egg_id = 9999
+        self.store.add(egg)
+        self.store.add(egg_info1)
+        self.store.add(egg_info2)
+        self.store.flush()
+        self.assertIs(egg_info1, egg.info)
+
+    def test_poly_link_with_change_type(self):
+        """
+        When changing the type of the linked remote object, the poly type
+        column on the local object gets changed accordingly.
+        """
+        egg = Egg(StoredEggInfo)
+        self.assertEqual(StoredEggInfo.info_type, egg.info_type)
+
+        egg.info = MemoryEggInfo()
+        self.assertEqual(MemoryEggInfo.info_type, egg.info_type)
+
+    def test_poly_with_unlink_stored_remote(self):
+        """
+        If the reference is linked to a remote object, when setting it to
+        C{None} the poly type on the local object gets reset too.
+        """
+        egg = Egg(StoredEggInfo)
+        egg.id = 1234
+        egg_info = egg.info
+        self.assertEqual(StoredEggInfo.info_type, egg.info_type)
+        self.assertEqual(1234, egg_info.egg_id)
+
+        egg.info = None
+        self.assertEqual(None, egg.info_type)
+        self.assertEqual(None, egg_info.egg_id)
+
     def test_get_decimal_property(self):
         money = self.store.get(Money, 10)
         self.assertEquals(money.value, decimal.Decimal("12.3455"))

=== modified file 'tests/store/mysql.py'
--- tests/store/mysql.py	2011-02-14 12:17:54 +0000
+++ tests/store/mysql.py	2011-10-25 10:34:20 +0000
@@ -79,6 +79,10 @@
         connection.execute("CREATE TABLE unique_id "
                            "(id VARCHAR(36) PRIMARY KEY) "
                            "ENGINE=InnoDB")
+        connection.execute("CREATE TABLE egg "
+                           "(id INT PRIMARY KEY AUTO_INCREMENT,"
+                           " info_type INT)")
+        connection.execute("CREATE TABLE stored_egg_info (egg_id INT)")
         connection.commit()
 
 

=== modified file 'tests/store/postgres.py'
--- tests/store/postgres.py	2011-02-14 12:17:54 +0000
+++ tests/store/postgres.py	2011-10-25 10:34:20 +0000
@@ -93,6 +93,9 @@
                            " value1 INTEGER, value2 INTEGER)")
         connection.execute("CREATE TABLE unique_id "
                            "(id UUID PRIMARY KEY)")
+        connection.execute("CREATE TABLE egg "
+                           "(id SERIAL PRIMARY KEY, info_type INTEGER)")
+        connection.execute("CREATE TABLE stored_egg_info (egg_id INTEGER)")
         connection.commit()
 
     def drop_tables(self):

=== modified file 'tests/store/sqlite.py'
--- tests/store/sqlite.py	2011-02-14 12:17:54 +0000
+++ tests/store/sqlite.py	2011-10-25 10:34:20 +0000
@@ -65,6 +65,10 @@
                            " value1 INTEGER, value2 INTEGER)")
         connection.execute("CREATE TABLE unique_id "
                            "(id VARCHAR PRIMARY KEY)")
+        connection.execute("CREATE TABLE egg "
+                           "(id INTEGER PRIMARY KEY, info_type INTEGER)")
+        connection.execute("CREATE TABLE stored_egg_info "
+                           "(egg_id INTEGER)")
         connection.commit()
 
     def drop_tables(self):