Rename set_roles to with_roles and add support for declared_attr and …

…column properties

Fixes #139 and addresses the concerns raised in #138.

CHANGES.rst

@@ -1,6 +1,9 @@
 0.6.1
 -----
 
+* Renamed ``coaster.roles.set_roles`` to ``with_roles`` and added support for
+  wrapping ``declared_attr`` and column properties
+
 
 0.6.0
 -----

coaster/roles.py

@@ -25,22 +25,27 @@
 
     from flask import Flask
     from flask_sqlalchemy import SQLAlchemy
-    from coaster.sqlalchemy import BaseMixin, set_roles
+    from coaster.sqlalchemy import BaseMixin, with_roles, set_roles
 
     app = Flask(__name__)
     db = SQLAlchemy(app)
 
     class DeclaredAttrMixin(object):
-        # The ugly way to work with declared_attr
-        @declared_attr
+        # Standard usage
+        @with_roles(rw={'owner'})
         def mixed_in1(cls):
-            return set_roles(db.Column(db.Unicode(250)),
+            return db.Column(db.Unicode(250))
+
+        # Roundabout approach
+        @declared_attr
+        def mixed_in2(cls):
+            return with_roles(db.Column(db.Unicode(250)),
                 rw={'owner'})
 
-        # The clean way to work with declared_attr
+        # Deprecated since 0.6.1
         @declared_attr
         @declared_attr_roles(rw={'owner', 'editor'}, read={'all'})
-        def mixed_in2(cls):
+        def mixed_in3(cls):
             return db.Column(db.Unicode(250))
 
 
@@ -60,13 +65,30 @@ class RoleModel(DeclaredAttrMixin, RoleMixin, db.Model):
         # These annotations always add to anything specified in __roles__
 
         id = db.Column(db.Integer, primary_key=True)
-        name = db.Column(db.Unicode(250))
-        set_roles(name, rw={'owner'})  # Specify read+write access
+        name = with_roles(db.Column(db.Unicode(250)),
+            rw={'owner'})  # Specify read+write access
+
+        # with_roles can also be called later, using the alias set_roles
+        # for clarity. Both are the same. This is typically required for
+        # properties, where roles must be assigned after the property is
+        # fully described
+
+        _title = db.Column('title', db.Unicode(250))
+
+        @property
+        def title(self):
+            return self._title
+
+        @title.setter
+        def title(self, value):
+            self._title = value
 
-        title = db.Column(db.Unicode(250))
-        set_roles(title, write={'owner', 'editor'})  # Grant 'owner' and 'editor' write but not read access
+        set_roles(title, write={'owner', 'editor'})  # This grants 'owner' and 'editor' write but not read access
 
-        @set_roles(call={'all'})  # 'call' is an alias for 'read', to be used for clarity
+        # with_roles can be used as a decorator on functions.
+        # 'call' is an alias for 'read', to be used for clarity
+
+        @with_roles(call={'all'})
         def hello(self):
             return "Hello!"
 
@@ -86,8 +108,13 @@ def roles_for(self, user=None, token=None):
 import collections
 from copy import deepcopy
 from sqlalchemy import event
+from sqlalchemy.orm import mapper
+from sqlalchemy.orm.attributes import QueryableAttribute
+
+__all__ = ['RoleAccessProxy', 'RoleMixin', 'with_roles', 'set_roles', 'declared_attr_roles']
 
-__all__ = ['RoleAccessProxy', 'RoleMixin', 'set_roles', 'declared_attr_roles']
+# Global dictionary for temporary storage of roles until the mapper_configured events
+__cache__ = {}
 
 
 class RoleAccessProxy(collections.Mapping):
@@ -132,7 +159,7 @@ def __init__(self, obj, roles):
         self.__dict__['_write'] = write
 
     def __repr__(self):  # pragma: no-cover
-        return 'RoleAccessProxy(obj={obj}, roles={roles})'.format(
+        return '<RoleAccessProxy(obj={obj}, roles={roles})>'.format(
             obj=repr(self._obj), roles=repr(self._roles))
 
     def __getattr__(self, attr):
@@ -170,7 +197,7 @@ def __iter__(self):
             yield key
 
 
-def set_roles(obj=None, rw=None, call=None, read=None, write=None):
+def with_roles(obj=None, rw=None, call=None, read=None, write=None):
     """
     Convenience function and decorator to define roles on an attribute. Only
     works with :class:`RoleMixin`, which reads the annotations made by this
@@ -179,9 +206,9 @@ def set_roles(obj=None, rw=None, call=None, read=None, write=None):
     Examples::
 
         id = db.Column(Integer, primary_key=True)
-        set_roles(id, read={'all'})
+        with_roles(id, read={'all'})
 
-        @set_roles(read={'all'})
+        @with_roles(read={'all'})
         @hybrid_property
         def url_id(self):
             return str(self.id)
@@ -206,7 +233,14 @@ def url_id(self):
     write.update(rw)
 
     def inner(attr):
-        attr._coaster_roles = {'read': read, 'write': write}
+        __cache__[attr] = {'read': read, 'write': write}
+        try:
+            attr._coaster_roles = {'read': read, 'write': write}
+            # If the attr has a restrictive __slots__, we'll get an attribute error.
+            # Use of _coaster_roles is now legacy, for declared_attr_roles, so we
+            # can safely ignore the error.
+        except AttributeError:
+            pass
         return attr
 
     if isinstance(obj, (list, tuple, set)):
@@ -217,28 +251,35 @@ def inner(attr):
     else:
         return inner
 
+# with_roles was set_roles when originally introduced in 0.6.0
+set_roles = with_roles
+
 
 def declared_attr_roles(rw=None, call=None, read=None, write=None):
     """
-    Equivalent of :func:`set_roles` for use with ``@declared_attr``::
+    Equivalent of :func:`with_roles` for use with ``@declared_attr``::
 
         @declared_attr
         @declared_attr_roles(read={'all'})
         def my_column(cls):
             return Column(Integer)
 
-    While :func:`set_roles` is always the outermost decorator on properties
+    While :func:`with_roles` is always the outermost decorator on properties
     and functions, :func:`declared_attr_roles` must appear below
     ``@declared_attr`` to work correctly.
+
+    .. deprecated:: 0.6.1
+        Use :func:`with_roles` instead. It works for
+        :class:`~sqlalchemy.ext.declarative.declared_attr` since 0.6.1
     """
     def inner(f):
         @wraps(f)
         def attr(cls):
-            # Pass f(cls) as a parameter to set_roles.inner to avoid the test for
-            # iterables within set_roles. We have no idea about the use cases for
+            # Pass f(cls) as a parameter to with_roles.inner to avoid the test for
+            # iterables within with_roles. We have no idea about the use cases for
             # declared_attr in downstream code. There could be a declared_attr
             # that returns a list that should be accessible via the proxy.
-            return set_roles(rw=rw, call=call, read=read, write=write)(f(cls))
+            return with_roles(rw=rw, call=call, read=read, write=write)(f(cls))
         return attr
     return inner
 
@@ -326,7 +367,7 @@ def access_for(self, roles=None, user=None, token=None):
 def __configure_roles(mapper, cls):
     """
     Run through attributes of the class looking for role decorations from
-    :func:`set_roles` and add them to :attr:`cls.__roles__`
+    :func:`with_roles` and add them to :attr:`cls.__roles__`
     """
     # Don't mutate __roles__ in the base class.
     # The subclass must have its own.
@@ -345,9 +386,26 @@ def __configure_roles(mapper, cls):
         for name, attr in base.__dict__.items():
             if name in processed or name.startswith('__'):
                 continue
-            processed.add(name)
-            if hasattr(attr, '_coaster_roles'):
-                for role in attr._coaster_roles.get('read', []):
+
+            if isinstance(attr, collections.Hashable) and attr in __cache__:
+                data = __cache__[attr]
+                del __cache__[attr]
+            elif isinstance(attr, QueryableAttribute) and attr.property in __cache__:
+                data = __cache__[attr.property]
+                del __cache__[attr.property]
+            elif hasattr(attr, '_coaster_roles'):  # XXX: Deprecated
+                data = attr._coaster_roles
+            else:
+                data = None
+            if data is not None:
+                for role in data.get('read', []):
                     cls.__roles__.setdefault(role, {}).setdefault('read', set()).add(name)
-                for role in attr._coaster_roles.get('write', []):
+                for role in data.get('write', []):
                     cls.__roles__.setdefault(role, {}).setdefault('write', set()).add(name)
+                processed.add(name)
+
+
+@event.listens_for(mapper, 'after_configured')
+def __clear_cache():
+    for key in tuple(__cache__):
+        del __cache__[key]

coaster/sqlalchemy.py

@@ -38,7 +38,7 @@ class MyModel(BaseMixin, db.Model):
 from flask import Markup, url_for
 from flask_sqlalchemy import BaseQuery
 from .utils import make_name, uuid2buid, uuid2suuid, buid2uuid, suuid2uuid
-from .roles import RoleMixin, set_roles, declared_attr_roles  # NOQA
+from .roles import RoleMixin, with_roles, set_roles, declared_attr_roles  # NOQA
 from .gfm import markdown
 import six
 
@@ -280,8 +280,8 @@ class MyDocument(UuidMixin, BaseMixin, db.Model):
     | BaseScopedIdNameMixin | No          | Conflicting :attr:`url_id` attribute    |
     +-----------------------+-------------+-----------------------------------------+
     """
+    @with_roles(read={'all'})
     @declared_attr
-    @declared_attr_roles(read={'all'})
     def uuid(cls):
         """UUID column, or synonym to existing :attr:`id` column if that is a UUID"""
         if hasattr(cls, '__uuid_primary_key__') and cls.__uuid_primary_key__:
@@ -704,7 +704,7 @@ def make_name(self):
         if self.title:
             self.name = six.text_type(make_name(self.title, maxlength=self.__name_length__))
 
-    @set_roles(read={'all'})
+    @with_roles(read={'all'})
     @hybrid_property
     def url_id_name(self):
         """
@@ -725,7 +725,7 @@ def url_id_name(cls):
 
     url_name = url_id_name  # Legacy name
 
-    @set_roles(read={'all'})
+    @with_roles(read={'all'})
     @hybrid_property
     def url_name_suuid(self):
         """
@@ -753,8 +753,8 @@ class Issue(BaseScopedIdMixin, db.Model):
             parent = db.synonym('event')
             __table_args__ = (db.UniqueConstraint('event_id', 'url_id'),)
     """
+    @with_roles(read={'all'})
     @declared_attr
-    @declared_attr_roles(read={'all'})
     def url_id(cls):
         """Contains an id number that is unique within the parent container"""
         return Column(Integer, nullable=False)
@@ -856,7 +856,7 @@ def make_name(self):
         if self.title:
             self.name = six.text_type(make_name(self.title, maxlength=self.__name_length__))
 
-    @set_roles(read={'all'})
+    @with_roles(read={'all'})
     @hybrid_property
     def url_id_name(self):
         """Returns a URL name combining :attr:`url_id` and :attr:`name` in id-name syntax"""
@@ -1059,8 +1059,8 @@ def MarkdownColumn(name, deferred=False, group=None, **kwargs):
 
 # --- Helper functions --------------------------------------------------------
 
-__all_functions = ['failsafe_add', 'set_roles', 'declared_attr_roles', 'add_primary_relationship',
-    'auto_init_default']
+__all_functions = ['failsafe_add', 'with_roles', 'set_roles', 'declared_attr_roles',
+    'add_primary_relationship', 'auto_init_default']
 
 
 def failsafe_add(_session, _instance, **filters):

tests/test_roles.py

@@ -3,7 +3,7 @@
 import unittest
 from flask import Flask
 from sqlalchemy.ext.declarative import declared_attr
-from coaster.sqlalchemy import RoleMixin, set_roles, declared_attr_roles, BaseMixin, UuidMixin
+from coaster.sqlalchemy import RoleMixin, with_roles, set_roles, declared_attr_roles, BaseMixin, UuidMixin
 from coaster.db import db
 
 app = Flask(__name__)
@@ -18,7 +18,7 @@ class DeclaredAttrMixin(object):
     # The ugly way to work with declared_attr
     @declared_attr
     def mixed_in1(cls):
-        return set_roles(db.Column(db.Unicode(250)),
+        return with_roles(db.Column(db.Unicode(250)),
             rw={'owner'})
 
     # The clean way to work with declared_attr
@@ -27,6 +27,15 @@ def mixed_in1(cls):
     def mixed_in2(cls):
         return db.Column(db.Unicode(250))
 
+    @with_roles(rw={'owner'})
+    @declared_attr
+    def mixed_in3(cls):
+        return db.Column(db.Unicode(250))
+
+    # A regular column from the mixin
+    mixed_in4 = db.Column(db.Unicode(250))
+    with_roles(mixed_in4, rw={'owner'})
+
 
 class RoleModel(DeclaredAttrMixin, RoleMixin, db.Model):
     __tablename__ = 'role_model'
@@ -47,10 +56,13 @@ class RoleModel(DeclaredAttrMixin, RoleMixin, db.Model):
     name = db.Column(db.Unicode(250))
     set_roles(name, rw={'owner'})  # Specify read+write access
 
-    title = db.Column(db.Unicode(250))
-    set_roles(title, write={'owner', 'editor'})  # Grant 'owner' and 'editor' write but not read access
+    title = with_roles(db.Column(db.Unicode(250)),
+        write={'owner', 'editor'})  # Grant 'owner' and 'editor' write but not read access
+
+    defval = with_roles(db.deferred(db.Column(db.Unicode(250))),
+        rw={'owner'})
 
-    @set_roles(call={'all'})  # 'call' is an alias for 'read', to be used for clarity
+    @with_roles(call={'all'})  # 'call' is an alias for 'read', to be used for clarity
     def hello(self):
         return "Hello!"
 
@@ -68,13 +80,13 @@ def roles_for(self, user=None, token=None):
 class AutoRoleModel(RoleMixin, db.Model):
     __tablename__ = 'auto_role_model'
 
-    # This model doesn't specify __roles__. It only uses set_roles.
+    # This model doesn't specify __roles__. It only uses with_roles.
     # It should still work
     id = db.Column(db.Integer, primary_key=True)
-    set_roles(id, read={'all'})
+    with_roles(id, read={'all'})
 
     name = db.Column(db.Unicode(250))
-    set_roles(name, rw={'owner'}, read={'all'})
+    with_roles(name, rw={'owner'}, read={'all'})
 
 
 class BaseModel(BaseMixin, db.Model):
@@ -118,13 +130,13 @@ def test_role_dict(self):
                 'write': {'title', 'mixed_in2'},
                 },
             'owner': {
-                'read': {'name', 'mixed_in1', 'mixed_in2'},
-                'write': {'name', 'title', 'mixed_in1', 'mixed_in2'},
+                'read': {'name', 'defval', 'mixed_in1', 'mixed_in2', 'mixed_in3', 'mixed_in4'},
+                'write': {'name', 'title', 'defval', 'mixed_in1', 'mixed_in2', 'mixed_in3', 'mixed_in4'},
                 },
             })
 
     def test_autorole_dict(self):
-        """A model without __roles__, using only set_roles, also works as expected"""
+        """A model without __roles__, using only with_roles, also works as expected"""
         self.assertEqual(AutoRoleModel.__roles__, {
             'all': {
                 'read': {'id', 'name'},
@@ -192,8 +204,9 @@ def test_diff_roles(self):
         proxy2 = rm.access_for(roles={'owner'})
         proxy3 = rm.access_for(roles={'all', 'owner'})
         self.assertEqual(set(proxy1), {'id', 'name', 'title', 'mixed_in2', 'hello'})
-        self.assertEqual(set(proxy2), {'name', 'mixed_in1', 'mixed_in2'})
-        self.assertEqual(set(proxy3), {'id', 'name', 'title', 'mixed_in1', 'mixed_in2', 'hello'})
+        self.assertEqual(set(proxy2), {'name', 'defval', 'mixed_in1', 'mixed_in2', 'mixed_in3', 'mixed_in4'})
+        self.assertEqual(set(proxy3),
+            {'id', 'name', 'title', 'defval', 'mixed_in1', 'mixed_in2', 'mixed_in3', 'mixed_in4', 'hello'})
 
     def test_write_without_read(self):
         """A proxy may allow writes without allowing reads"""
@@ -243,9 +256,9 @@ def test_dictionary_comparison(self):
             )
 
     def test_bad_decorator(self):
-        """Prevent set_roles from being used with a positional parameter"""
+        """Prevent with_roles from being used with a positional parameter"""
         with self.assertRaises(TypeError):
-            @set_roles({'all'})
+            @with_roles({'all'})
             def foo():
                 pass