From 7b8cf49628fbc6116e6428faca3b9b3b49209523 Mon Sep 17 00:00:00 2001 From: Kiran Jonnalagadda Date: Thu, 31 Aug 2017 03:31:58 +0530 Subject: [PATCH 1/2] 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 | 3 ++ coaster/roles.py | 112 ++++++++++++++++++++++++++++++++---------- coaster/sqlalchemy.py | 16 +++--- tests/test_roles.py | 43 ++++++++++------ 4 files changed, 124 insertions(+), 50 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 6885c9a2..8273659b 100644 --- a/CHANGES.rst +++ b/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 ----- diff --git a/coaster/roles.py b/coaster/roles.py index e8599d6d..caf452e5 100644 --- a/coaster/roles.py +++ b/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 ''.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] diff --git a/coaster/sqlalchemy.py b/coaster/sqlalchemy.py index ba0fc7a8..ff15af3a 100644 --- a/coaster/sqlalchemy.py +++ b/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): diff --git a/tests/test_roles.py b/tests/test_roles.py index 091ecce5..80ded34d 100644 --- a/tests/test_roles.py +++ b/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 From c489f7cca0f8583999957a24867f2dda89c903cc Mon Sep 17 00:00:00 2001 From: Kiran Jonnalagadda Date: Thu, 31 Aug 2017 15:54:10 +0530 Subject: [PATCH 2/2] Remove set_roles from documentation --- coaster/roles.py | 27 ++++++++++++++------------- coaster/sqlalchemy.py | 6 +++--- tests/test_roles.py | 8 ++++---- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/coaster/roles.py b/coaster/roles.py index caf452e5..2f329dc9 100644 --- a/coaster/roles.py +++ b/coaster/roles.py @@ -25,7 +25,7 @@ from flask import Flask from flask_sqlalchemy import SQLAlchemy - from coaster.sqlalchemy import BaseMixin, with_roles, set_roles + from coaster.sqlalchemy import BaseMixin, with_roles app = Flask(__name__) db = SQLAlchemy(app) @@ -52,8 +52,10 @@ def mixed_in3(cls): class RoleModel(DeclaredAttrMixin, RoleMixin, db.Model): __tablename__ = 'role_model' - # Approach one, declare roles in advance. - # 'all' is a special role that is always granted from the base class + # The low level approach is to declare roles in advance. + # 'all' is a special role that is always granted from the base class. + # Avoid this approach because you may accidentally lose roles defined + # in base classes. __roles__ = { 'all': { @@ -61,17 +63,16 @@ class RoleModel(DeclaredAttrMixin, RoleMixin, db.Model): } } - # Approach two, annotate roles on the attributes. - # These annotations always add to anything specified in __roles__ + # Recommended: annotate roles on the attributes using ``with_roles``. + # These annotations always add to anything specified in ``__roles__``. id = db.Column(db.Integer, primary_key=True) 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 + # ``with_roles`` can also be called later. This is typically required + # for properties, where roles must be assigned after the property is + # fully described. _title = db.Column('title', db.Unicode(250)) @@ -83,10 +84,10 @@ def title(self): def title(self, value): self._title = value - set_roles(title, write={'owner', 'editor'}) # This grants 'owner' and 'editor' write but not read access + title = with_roles(title, write={'owner', 'editor'}) # This grants 'owner' and 'editor' write but not read access - # with_roles can be used as a decorator on functions. - # '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): @@ -111,7 +112,7 @@ def roles_for(self, user=None, token=None): from sqlalchemy.orm import mapper from sqlalchemy.orm.attributes import QueryableAttribute -__all__ = ['RoleAccessProxy', 'RoleMixin', 'with_roles', 'set_roles', 'declared_attr_roles'] +__all__ = ['RoleAccessProxy', 'RoleMixin', 'with_roles', 'declared_attr_roles'] # Global dictionary for temporary storage of roles until the mapper_configured events __cache__ = {} diff --git a/coaster/sqlalchemy.py b/coaster/sqlalchemy.py index ff15af3a..472c031c 100644 --- a/coaster/sqlalchemy.py +++ b/coaster/sqlalchemy.py @@ -303,7 +303,7 @@ def url_id(cls): else: return SqlHexUuidComparator(cls.uuid) - set_roles(url_id, read={'all'}) + url_id = with_roles(url_id, read={'all'}) @hybrid_property def buid(self): @@ -318,7 +318,7 @@ def buid(self, value): def buid(cls): return SqlBuidComparator(cls.uuid) - set_roles(buid, read={'all'}) + buid = with_roles(buid, read={'all'}) @hybrid_property def suuid(self): @@ -333,7 +333,7 @@ def suuid(self, value): def suuid(cls): return SqlSuuidComparator(cls.uuid) - set_roles(suuid, read={'all'}) + suuid = with_roles(suuid, read={'all'}) # Setup listeners for UUID-based subclasses diff --git a/tests/test_roles.py b/tests/test_roles.py index 80ded34d..a011163a 100644 --- a/tests/test_roles.py +++ b/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, with_roles, set_roles, declared_attr_roles, BaseMixin, UuidMixin +from coaster.sqlalchemy import RoleMixin, with_roles, declared_attr_roles, BaseMixin, UuidMixin from coaster.db import db app = Flask(__name__) @@ -34,7 +34,7 @@ def mixed_in3(cls): # A regular column from the mixin mixed_in4 = db.Column(db.Unicode(250)) - with_roles(mixed_in4, rw={'owner'}) + mixed_in4 = with_roles(mixed_in4, rw={'owner'}) class RoleModel(DeclaredAttrMixin, RoleMixin, db.Model): @@ -53,8 +53,8 @@ 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 title = with_roles(db.Column(db.Unicode(250)), write={'owner', 'editor'}) # Grant 'owner' and 'editor' write but not read access