a kinder gentler _CompileOnAttr...or just whack it

Issue #1269 resolved
Mike Bayer repo owner created an issue

This patch almost passes tests and allows the __clause_element__() to be accessed in most cases without the mapper compilation step occurring. This would allow column entities to be much easier to use with declarative. The error message tests we have in declarative become easier too.

The next step beyond this patch would be to have MapperProperties set up the InstrumentedAttribute immediately, and do away with _CompileOnAttr. This would probably involve moving the whole _register_attribute step into properties.py. the class_level_loader, which is the only part related to loader strategy at this point, would just be tacked on to the InstrumentedAttribute during compilation. mapper._adapt_inherited_property would also need to trigger an instrument since we need to instrument for subclasses.

Index: lib/sqlalchemy/orm/mapper.py
===================================================================
--- lib/sqlalchemy/orm/mapper.py    (revision 5603)
+++ lib/sqlalchemy/orm/mapper.py    (working copy)
@@ -519,33 +519,26 @@
     class _CompileOnAttr(PropComparator):
         """A placeholder descriptor which triggers compilation on access."""

-        def __init__(self, class_, key):
-            self.class_ = class_
-            self.key = key
-            self.existing_prop = getattr(class_, key, None)
-
+        def __init__(self, mapper, prop):
+            self.mapper = mapper
+            self.prop = prop
+            self.existing_prop = getattr(mapper.class_, prop.key, None)
+            
+        def _get_attr_after_init(self, key):
+            mapper = object.__getattribute__(self, 'mapper')
+            prop = object.__getattribute__(self, 'prop')
+            mapper.compile()
+            return getattr(getattr(mapper.class_, prop.key), key)
+            
         def __getattribute__(self, key):
-            cls = object.__getattribute__(self, 'class_')
-            clskey = object.__getattribute__(self, 'key')
+            mapper = object.__getattribute__(self, 'mapper')
+            prop = object.__getattribute__(self, 'prop')
+            if not isinstance(prop, SynonymProperty) and prop.comparator_factory:
+                return getattr(prop.comparator_factory(prop, mapper), key)
+            else:
+                _get_attr = object.__getattribute__(self, '_get_attr_after_init')
+            return _get_attr(key)

-            # ugly hack
-            if key.startswith('__') and key != '__clause_element__':
-                return object.__getattribute__(self, key)
-
-            class_mapper(cls)
-                
-            if cls.__dict__.get(clskey) is self:
-                # if this warning occurs, it usually means mapper
-                # compilation has failed, but operations upon the mapped
-                # classes have proceeded.
-                util.warn(
-                    ("Attribute '%s' on class '%s' was not replaced during "
-                     "mapper compilation operation") % (clskey, cls.__name__))
-                # clean us up explicitly
-                delattr(cls, clskey)
-
-            return getattr(getattr(cls, clskey), key)
-
     def _compile_property(self, key, prop, init=True, setparent=True):
         self._log("_compile_property(%s, %s)" % (key, prop.__class__.__name__))

@@ -638,7 +631,7 @@

             if not self.non_primary:
                 self.class_manager.install_descriptor(
-                    key, Mapper._CompileOnAttr(self.class_, key))
+                    key, Mapper._CompileOnAttr(self, prop))

         if init:
             prop.init(key, self)

such as this test:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

class User(Base):
    __tablename__ = 'foo'

    id = Column(Integer, primary_key=True)
    addresses = relation("Addresses")


print User.id == 5

Comments (4)

  1. Mike Bayer reporter

    attached a patch which allows all current tests to pass. But its still wrong - the preinstrument __clause_element__() method is setting parententity to the base mapper, which will break for some of those inheritance tests if you do query(Manager.name) before mappers are compiled. I think we really need to reorganize how MapperProperty configures so that instrumentation can occur immediately. It leaves the least room for surprises.

  2. Log in to comment