[repository] forbid usage of set_attributes() in before_add_entity stable
authorSylvain Thénault <sylvain.thenault@logilab.fr>
Wed, 31 Mar 2010 14:46:04 +0200
branchstable
changeset 5115 2e43ef618d14
parent 5114 55b8b7a5bc4a
child 5116 a2ce436e00ad
[repository] forbid usage of set_attributes() in before_add_entity set_attributes() will generate a RQL query that will silently fail. An explicit error is better. You can still use the ``entity[attr] = value`` notation that won't generate a SQL query but still update the inner state of the entity and its edited_attributes attribute.
entity.py
server/repository.py
server/test/unittest_repository.py
--- a/entity.py	Wed Mar 31 14:32:19 2010 +0200
+++ b/entity.py	Wed Mar 31 14:46:04 2010 +0200
@@ -298,6 +298,12 @@
             self.edited_attributes.remove(attr)
         return value
 
+    def update(self, values):
+        """override update to update self.edited_attributes. See `__setitem__`
+        """
+        for attr, value in values.items():
+            self[attr] = value # use self.__setitem__ implementation
+
     def rql_set_value(self, attr, value):
         """call by rql execution plan when some attribute is modified
 
@@ -874,17 +880,21 @@
     # raw edition utilities ###################################################
 
     def set_attributes(self, **kwargs):
+        _check_cw_unsafe(kwargs)
         assert kwargs
-        _check_cw_unsafe(kwargs)
+        assert self._is_saved
         relations = []
         for key in kwargs:
             relations.append('X %s %%(%s)s' % (key, key))
-        # update current local object
-        self.update(kwargs)
         # and now update the database
         kwargs['x'] = self.eid
         self._cw.execute('SET %s WHERE X eid %%(x)s' % ','.join(relations),
                          kwargs, 'x')
+        kwargs.pop('x')
+        # update current local object _after_ the rql query to avoid
+        # interferences between the query execution itself and the
+        # edited_attributes / skip_security_attributes machinery
+        self.update(kwargs)
 
     def set_relations(self, **kwargs):
         """add relations to the given object. To set a relation where this entity
--- a/server/repository.py	Wed Mar 31 14:32:19 2010 +0200
+++ b/server/repository.py	Wed Mar 31 14:46:04 2010 +0200
@@ -24,7 +24,6 @@
 from os.path import join
 from datetime import datetime
 from time import time, localtime, strftime
-#from pickle import dumps
 
 from logilab.common.decorators import cached
 from logilab.common.compat import any
@@ -38,7 +37,7 @@
                       UnknownEid, AuthenticationError, ExecutionError,
                       ETypeNotSupportedBySources, MultiSourcesError,
                       BadConnectionId, Unauthorized, ValidationError,
-                      typed_eid, onevent)
+                      RepositoryError, typed_eid, onevent)
 from cubicweb import cwvreg, schema, server
 from cubicweb.server import utils, hook, pool, querier, sources
 from cubicweb.server.session import Session, InternalSession, InternalManager, \
@@ -1067,62 +1066,68 @@
         if server.DEBUG & server.DBG_REPO:
             print 'UPDATE entity', entity.__regid__, entity.eid, \
                   dict(entity), edited_attributes
-        entity.edited_attributes = edited_attributes
-        if session.is_hook_category_activated('integrity'):
-            entity.check()
+        hm = self.hm
         eschema = entity.e_schema
         session.set_entity_cache(entity)
-        only_inline_rels, need_fti_update = True, False
-        relations = []
-        for attr in edited_attributes:
-            if attr == 'eid':
-                continue
-            rschema = eschema.subjrels[attr]
-            if rschema.final:
-                if getattr(eschema.rdef(attr), 'fulltextindexed', False):
-                    need_fti_update = True
-                only_inline_rels = False
-            else:
-                # inlined relation
-                previous_value = entity.related(attr) or None
-                if previous_value is not None:
-                    previous_value = previous_value[0][0] # got a result set
-                    if previous_value == entity[attr]:
-                        previous_value = None
+        orig_edited_attributes = getattr(entity, 'edited_attributes', None)
+        entity.edited_attributes = edited_attributes
+        try:
+            if session.is_hook_category_activated('integrity'):
+                entity.check()
+            only_inline_rels, need_fti_update = True, False
+            relations = []
+            for attr in list(edited_attributes):
+                if attr == 'eid':
+                    continue
+                rschema = eschema.subjrels[attr]
+                if rschema.final:
+                    if getattr(eschema.rdef(attr), 'fulltextindexed', False):
+                        need_fti_update = True
+                    only_inline_rels = False
+                else:
+                    # inlined relation
+                    previous_value = entity.related(attr) or None
+                    if previous_value is not None:
+                        previous_value = previous_value[0][0] # got a result set
+                        if previous_value == entity[attr]:
+                            previous_value = None
+                        else:
+                            hm.call_hooks('before_delete_relation', session,
+                                          eidfrom=entity.eid, rtype=attr,
+                                          eidto=previous_value)
+                    relations.append((attr, entity[attr], previous_value))
+                source = self.source_from_eid(entity.eid, session)
+                if source.should_call_hooks:
+                    # call hooks for inlined relations
+                    for attr, value, _ in relations:
+                        hm.call_hooks('before_add_relation', session,
+                                      eidfrom=entity.eid, rtype=attr, eidto=value)
+                    if not only_inline_rels:
+                        hm.call_hooks('before_update_entity', session, entity=entity)
+                source.update_entity(session, entity)
+            self.system_source.update_info(session, entity, need_fti_update)
+            if source.should_call_hooks:
+                if not only_inline_rels:
+                    hm.call_hooks('after_update_entity', session, entity=entity)
+                for attr, value, prevvalue in relations:
+                    # if the relation is already cached, update existant cache
+                    relcache = entity.relation_cached(attr, 'subject')
+                    if prevvalue is not None:
+                        hm.call_hooks('after_delete_relation', session,
+                                      eidfrom=entity.eid, rtype=attr, eidto=prevvalue)
+                        if relcache is not None:
+                            session.update_rel_cache_del(entity.eid, attr, prevvalue)
+                    del_existing_rel_if_needed(session, entity.eid, attr, value)
+                    if relcache is not None:
+                        session.update_rel_cache_add(entity.eid, attr, value)
                     else:
-                        self.hm.call_hooks('before_delete_relation', session,
-                                           eidfrom=entity.eid, rtype=attr,
-                                           eidto=previous_value)
-                relations.append((attr, entity[attr], previous_value))
-        source = self.source_from_eid(entity.eid, session)
-        if source.should_call_hooks:
-            # call hooks for inlined relations
-            for attr, value, _ in relations:
-                self.hm.call_hooks('before_add_relation', session,
-                                    eidfrom=entity.eid, rtype=attr, eidto=value)
-            if not only_inline_rels:
-                self.hm.call_hooks('before_update_entity', session, entity=entity)
-        source.update_entity(session, entity)
-        self.system_source.update_info(session, entity, need_fti_update)
-        if source.should_call_hooks:
-            if not only_inline_rels:
-                self.hm.call_hooks('after_update_entity', session, entity=entity)
-            for attr, value, prevvalue in relations:
-                # if the relation is already cached, update existant cache
-                relcache = entity.relation_cached(attr, 'subject')
-                if prevvalue is not None:
-                    self.hm.call_hooks('after_delete_relation', session,
-                                       eidfrom=entity.eid, rtype=attr, eidto=prevvalue)
-                    if relcache is not None:
-                        session.update_rel_cache_del(entity.eid, attr, prevvalue)
-                del_existing_rel_if_needed(session, entity.eid, attr, value)
-                if relcache is not None:
-                    session.update_rel_cache_add(entity.eid, attr, value)
-                else:
-                    entity.set_related_cache(attr, 'subject',
-                                             session.eid_rset(value))
-                self.hm.call_hooks('after_add_relation', session,
-                                    eidfrom=entity.eid, rtype=attr, eidto=value)
+                        entity.set_related_cache(attr, 'subject',
+                                                 session.eid_rset(value))
+                    hm.call_hooks('after_add_relation', session,
+                                  eidfrom=entity.eid, rtype=attr, eidto=value)
+        finally:
+            if orig_edited_attributes is not None:
+                entity.edited_attributes = orig_edited_attributes
 
     def glob_delete_entity(self, session, eid):
         """delete an entity and all related entities from the repository"""
--- a/server/test/unittest_repository.py	Wed Mar 31 14:32:19 2010 +0200
+++ b/server/test/unittest_repository.py	Wed Mar 31 14:46:04 2010 +0200
@@ -8,6 +8,8 @@
 """
 from __future__ import with_statement
 
+from __future__ import with_statement
+
 import os
 import sys
 import threading
@@ -21,12 +23,14 @@
 
 from cubicweb import (BadConnectionId, RepositoryError, ValidationError,
                       UnknownEid, AuthenticationError)
+from cubicweb.selectors import implements
 from cubicweb.schema import CubicWebSchema, RQLConstraint
 from cubicweb.dbapi import connect, multiple_connections_unfix
 from cubicweb.devtools.testlib import CubicWebTC
 from cubicweb.devtools.repotest import tuplify
 from cubicweb.server import repository, hook
 from cubicweb.server.sqlutils import SQL_PREFIX
+from cubicweb.server.hook import Hook
 from cubicweb.server.sources import native
 
 # start name server anyway, process will fail if already running
@@ -273,8 +277,10 @@
             from logilab.common import pyro_ext
             pyro_ext._DAEMONS.clear()
 
+
     def _pyro_client(self, done):
-        cnx = connect(self.repo.config.appid, u'admin', password='gingkow')
+        cnx = connect(self.repo.config.appid, u'admin', password='gingkow',
+                      initlog=False) # don't reset logging configuration
         try:
             # check we can get the schema
             schema = cnx.get_schema()
@@ -284,7 +290,7 @@
             cnx.close()
             done.append(True)
         finally:
-            # connect monkey path some method by default, remove them
+            # connect monkey patch some method by default, remove them
             multiple_connections_unfix()
 
     def test_internal_api(self):
@@ -358,6 +364,42 @@
         self.assertEquals(rset.rows[0][0], p2.eid)
 
 
+    def test_set_attributes_in_before_update(self):
+        # local hook
+        class DummyBeforeHook(Hook):
+            __regid__ = 'dummy-before-hook'
+            __select__ = Hook.__select__ & implements('EmailAddress')
+            events = ('before_update_entity',)
+            def __call__(self):
+                # safety belt: avoid potential infinite recursion if the test
+                #              fails (i.e. RuntimeError not raised)
+                pendings = self._cw.transaction_data.setdefault('pending', set())
+                if self.entity.eid not in pendings:
+                    pendings.add(self.entity.eid)
+                    self.entity.set_attributes(alias=u'foo')
+        with self.temporary_appobjects(DummyBeforeHook):
+            req = self.request()
+            addr = req.create_entity('EmailAddress', address=u'a@b.fr')
+            addr.set_attributes(address=u'a@b.com')
+            rset = self.execute('Any A,AA WHERE X eid %(x)s, X address A, X alias AA',
+                                {'x': addr.eid})
+            self.assertEquals(rset.rows, [[u'a@b.com', u'foo']])
+
+    def test_set_attributes_in_before_add(self):
+        # local hook
+        class DummyBeforeHook(Hook):
+            __regid__ = 'dummy-before-hook'
+            __select__ = Hook.__select__ & implements('EmailAddress')
+            events = ('before_add_entity',)
+            def __call__(self):
+                # set_attributes is forbidden within before_add_entity()
+                self.entity.set_attributes(alias=u'foo')
+        with self.temporary_appobjects(DummyBeforeHook):
+            req = self.request()
+            self.assertRaises(RepositoryError, req.create_entity,
+                              'EmailAddress', address=u'a@b.fr')
+
+
 class DataHelpersTC(CubicWebTC):
 
     def test_create_eid(self):