Merge a3e799a into d4c1a62

CHANGES.txt

@@ -1,9 +1,18 @@
 Change History
 ==============
 
-1.1.6 - unreleased
+1.2.0 - unreleased
 ------------------
 
+- **Greatly** reduce the number of queries that are sent to the DB:
+  - Add caching for the root node.
+  - Use eager / joined loading for local_groups.
+  - Don't query principals for roles
+- Add "missing" foreign key indices (with corresponding migration step).
+- Add a ``kotti.modification_date_excludes`` configuration option.
+  It takes a list of attributes in dotted name notation that should not trigger
+  an update of ``modification_date`` on change.  Defaults to
+  ``kotti.resources.Node.position``.
 - Don't try to set a caching header from the NewRequest handler when Pyramid's
   tweens didn't follow the usual chain of calls. This fixes compatibility with
   ``bowerstatic``.

kotti/alembic/versions/4a3de0d0804a_add_foreignkey_indices.py

@@ -0,0 +1,32 @@
+"""Add ForeignKey indices
+
+Revision ID: 4a3de0d0804a
+Revises: 37a05f6246af
+Create Date: 2015-08-31 12:37:26.493958
+
+"""
+
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = '4a3de0d0804a'
+down_revision = '37a05f6246af'
+
+
+def upgrade():
+    op.create_index(
+        'ix_nodes_parent_id', 'nodes', ['parent_id', ])
+    op.create_index(
+        'ix_local_groups_node_id', 'local_groups', ['node_id', ])
+    op.create_index(
+        'ix_local_groups_principal_name', 'local_groups', ['principal_name', ])
+    op.create_index(
+        'ix_tags_to_contents_tag_id', 'tags_to_contents', ['tag_id', ])
+    op.create_index(
+        'ix_tags_to_contents_content_id', 'tags_to_contents', ['content_id', ])
+
+
+def downgrade():
+    op.drop_index('ix_nodes_parent_id', 'nodes')
+    op.drop_index('ix_local_groups_node_id', 'local_groups')
+    op.drop_index('ix_local_groups_principal_name', 'local_groups')

kotti/resources.py

@@ -18,6 +18,7 @@
 from depot.fields.sqlalchemy import _SQLAMutationTracker
 from depot.fields.sqlalchemy import UploadedFileField
 from kotti import _resolve_dotted
+from pyramid.decorator import reify
 from pyramid.traversal import resource_path
 from sqlalchemy import Boolean
 from sqlalchemy import Column
@@ -179,10 +180,10 @@ class LocalGroup(Base):
     id = Column(Integer(), primary_key=True)
     #: ID of the node for this assignment
     #: (:class:`sqlalchemy.types.Integer`)
-    node_id = Column(ForeignKey('nodes.id'))
+    node_id = Column(ForeignKey('nodes.id'), index=True)
     #: Name of the principal (user or group)
     #: (:class:`sqlalchemy.types.Unicode`)
-    principal_name = Column(Unicode(100))
+    principal_name = Column(Unicode(100), index=True)
     #: Name of the assigned group or role
     #: (:class:`sqlalchemy.types.Unicode`)
     group_name = Column(Unicode(100))
@@ -223,7 +224,7 @@ class Node(Base, ContainerMixin, PersistentACLMixin):
     type = Column(String(30), nullable=False)
     #: ID of the node's parent
     #: (:class:`sqlalchemy.types.Integer`)
-    parent_id = Column(ForeignKey('nodes.id'))
+    parent_id = Column(ForeignKey('nodes.id'), index=True)
     #: Position of the node within its container / parent
     #: (:class:`sqlalchemy.types.Integer`)
     position = Column(Integer())
@@ -246,7 +247,7 @@ class Node(Base, ContainerMixin, PersistentACLMixin):
         remote_side=[id],
         backref=backref(
             '_children',
-            collection_class=ordering_list('position'),
+            collection_class=ordering_list('position', reorder_on_append=True),
             order_by=[position],
             cascade='all',
         )
@@ -256,6 +257,7 @@ class Node(Base, ContainerMixin, PersistentACLMixin):
         LocalGroup,
         backref=backref('node'),
         cascade='all',
+        lazy='joined',
         )
 
     def __init__(self, name=None, parent=None, title=u"", annotations=None,
@@ -462,10 +464,12 @@ class TagsToContents(Base):
 
     #: Foreign key referencing :attr:`Tag.id`
     #: (:class:`sqlalchemy.types.Integer`)
-    tag_id = Column(Integer, ForeignKey('tags.id'), primary_key=True)
+    tag_id = Column(Integer, ForeignKey('tags.id'), primary_key=True,
+                    index=True)
     #: Foreign key referencing :attr:`Content.id`
     #: (:class:`sqlalchemy.types.Integer`)
-    content_id = Column(Integer, ForeignKey('contents.id'), primary_key=True)
+    content_id = Column(Integer, ForeignKey('contents.id'), primary_key=True,
+                        index=True)
     #: Relation that adds a ``content_tags`` :func:`sqlalchemy.orm.backref`
     #: to :class:`~kotti.resources.Tag` instances to allow easy access to all
     #: content tagged with that tag.
@@ -789,19 +793,41 @@ def get_root(request=None):
     return get_settings()['kotti.root_factory'][0](request)
 
 
-def default_get_root(request=None):
-    """Default implementation for :func:`~kotti.resources.get_root`.
+class DefaultRootCache(object):
+    """ Default implementation for :func:`~kotti.resources.get_root` """
 
-    :param request: Current request (optional)
-    :type request: :class:`kotti.request.Request`
+    @reify
+    def root_id(self):
+        """ Query for the one node without a parent and return its id.
+        :result: The root node's id.
+        :rtype: int
+        """
 
-    :result: Node in the object tree that has no parent.
-    :rtype: :class:`~kotti.resources.Node` or descendant; in a fresh Kotti site
-            with Kotti's :func:`default populator <kotti.populate.populate>`
-            this will be an instance of :class:`~kotti.resources.Document`.
-    """
+        return Node.query.filter(Node.parent_id == None).one().id  # noqa
+
+    def get_root(self):
+        """ Query for the root node by its id.  This enables SQLAlchemy's
+        session cache (query is executed only once per session).
+        :result: The root node.
+        :rtype: :class:`Node`.
+        """
+
+        return Node.query.get(self.root_id)
+
+    def __call__(self, request=None):
+        """ Default implementation for :func:`~kotti.resources.get_root`
+        :param request: Current request (optional)
+        :type request: :class:`kotti.request.Request`
+        :result: Node in the object tree that has no parent.
+        :rtype: :class:`~kotti.resources.Node` or descendant;
+                in a fresh Kotti site with Kotti's
+                :func:`default populator <kotti.populate.populate>` this will
+                be an instance of :class:`~kotti.resources.Document`.
+        """
+
+        return self.get_root()
 
-    return DBSession.query(Node).filter(Node.parent_id == None).one()  # noqa
+default_get_root = DefaultRootCache()
 
 
 def _adjust_for_engine(engine):

kotti/security.py

@@ -270,14 +270,14 @@ def list_groups_raw(name, context):
     Only groups defined in context will be considered, therefore no
     global or inherited groups are returned.
     """
-    from kotti.resources import LocalGroup
+
     from kotti.resources import Node
 
     if isinstance(context, Node):
         return set(
-            r[0] for r in DBSession.query(LocalGroup.group_name).filter(
-                LocalGroup.node_id == context.id).filter(
-                LocalGroup.principal_name == name).all())
+            r.group_name for r in context.local_groups
+            if r.principal_name == name
+        )
     return set()
 
 
@@ -295,7 +295,7 @@ def _cachekey_list_groups_ext(name, context=None, _seen=None, _inherited=None):
         raise DontCache
     else:
         context_id = getattr(context, 'id', id(context))
-        return (name, context_id)
+        return (unicode(name), context_id)
 
 
 @request_cache(_cachekey_list_groups_ext)
@@ -340,14 +340,19 @@ def set_groups(name, context, groups_to_set=()):
     """Set the list of groups for principal with given ``name`` and in
     given ``context``.
     """
-    name = unicode(name)
+
     from kotti.resources import LocalGroup
-    DBSession.query(LocalGroup).filter(
-        LocalGroup.node_id == context.id).filter(
-        LocalGroup.principal_name == name).delete()
 
-    for group_name in groups_to_set:
-        DBSession.add(LocalGroup(context, name, unicode(group_name)))
+    name = unicode(name)
+    context.local_groups = [
+        # keep groups for "other" principals
+        lg for lg in context.local_groups
+        if lg.principal_name != name
+    ] + [
+        # reset groups for given principal
+        LocalGroup(context, name, unicode(group_name))
+        for group_name in groups_to_set
+    ]
 
 
 def list_groups_callback(name, request):
@@ -397,19 +402,19 @@ def principals_with_local_roles(context, inherit=True):
     """Return a list of principal names that have local roles in the
     context.
     """
-    from resources import LocalGroup
+
     principals = set()
     items = [context]
+
     if inherit:
         items = lineage(context)
+
     for item in items:
         principals.update(
-            r[0] for r in
-            DBSession.query(LocalGroup.principal_name).filter(
-                LocalGroup.node_id == item.id).group_by(
-                LocalGroup.principal_name).all()
-            if not r[0].startswith('role:')
-            )
+            r.principal_name for r in item.local_groups
+            if not r.principal_name.startswith('role:')
+        )
+
     return list(principals)
 
 
@@ -443,9 +448,13 @@ class Principals(DictMixin):
     """
     factory = Principal
 
-    @request_cache(lambda self, name: name)
+    @request_cache(lambda self, name: unicode(name))
     def __getitem__(self, name):
         name = unicode(name)
+        # avoid calls to the DB for roles
+        # (they're not stored in the ``principals`` table)
+        if name.startswith('role:'):
+            raise KeyError(name)
         try:
             return DBSession.query(
                 self.factory).filter(self.factory.name == name).one()

kotti/tests/browser.txt

@@ -544,7 +544,7 @@ Contents view change state actions
   >>> browser.getLink("Contents").click()
   >>> 'http://localhost:6543/second-child-1/third-child/@@workflow-change?new_state=public' in browser.contents
   True
-  >>> browser.getLink("Make Public", index=2).click()
+  >>> browser.getLink("Make Public", index=3).click()
   >>> 'http://localhost:6543/second-child-1/third-child/@@workflow-change?new_state=private' in browser.contents
   True