Merge pull request #288 from fantix/t287_model_lookup

Fixed #287, added `lookup()` in `Model`.

Author: fantix

gino/crud.py

@@ -28,7 +28,7 @@ def __get__(self, instance, owner):
         owner._check_abstract()
         q = sa.select([owner.__table__])
         if instance is not None:
-            q = instance.append_where_primary_key(q)
+            q = q.where(instance.lookup())
         return q.execution_options(model=weakref.ref(owner))
 
 
@@ -37,7 +37,7 @@ def __get__(self, instance, owner):
         def select(*args):
             q = sa.select([getattr(owner, x) for x in args])
             if instance is not None:
-                q = instance.append_where_primary_key(q)
+                q = q.where(instance.lookup())
             return q.execution_options(model=weakref.ref(owner),
                                        return_model=False)
         return select
@@ -83,10 +83,13 @@ def __init__(self, instance):
         self._values = {}
         self._props = {}
         self._literal = True
-        if instance.__table__ is None:
-            self._pk_values = None
-        else:
-            self._pk_values = _PrimaryKeyValues(instance)
+        self._locator = None
+        if instance.__table__ is not None:
+            try:
+                self._locator = instance.lookup()
+            except LookupError:
+                # apply() will fail anyway, but still allow updates()
+                pass
 
     def _set(self, key, value):
         self._values[key] = value
@@ -110,9 +113,9 @@ async def apply(self, bind=None, timeout=DEFAULT):
         :return: ``self`` for chaining calls.
 
         """
-        if self._pk_values is None:
+        if self._locator is None:
             raise TypeError(
-                'GINO model {} is abstract, no table is defined'.format(
+                'Model {} has no table, primary key or custom lookup()'.format(
                     self._instance.__class__.__name__))
         cls = type(self._instance)
         values = self._values.copy()
@@ -146,8 +149,8 @@ async def apply(self, bind=None, timeout=DEFAULT):
         opts = dict(return_model=False)
         if timeout is not DEFAULT:
             opts['timeout'] = timeout
-        clause = self._pk_values.append_where_primary_key(
-            type(self._instance).update
+        clause = type(self._instance).update.where(
+            self._locator,
         ).values(
             **values,
         ).returning(
@@ -252,18 +255,6 @@ def _inspect_alias(target):
     return sa.inspection.inspect(target.alias)
 
 
-class _PrimaryKeyValues:
-    def __init__(self, instance):
-        self._values = {}
-        for c in instance.__table__.primary_key.columns:
-            self._values[c] = getattr(instance, c.name)
-
-    def append_where_primary_key(self, q):
-        for c, v in self._values.items():
-            q = q.where(c == v)
-        return q
-
-
 class CRUDModel(Model):
     """
     The base class for models with CRUD support.
@@ -422,8 +413,7 @@ class CRUDModel(Model):
     def __init__(self, **values):
         super().__init__()
         self.__profile__ = None
-        # noinspection PyCallingNonCallable
-        self.update(**values)
+        self._update_request_cls(self).update(**values)
 
     @classmethod
     def _init_table(cls, sub_cls):
@@ -541,8 +531,37 @@ def append_where_primary_key(self, q):
 
             await user.query.gino.first()
 
+        .. deprecated:: 0.7.6
+            Use :meth:`.lookup` instead.
+
         """
-        return _PrimaryKeyValues(self).append_where_primary_key(q)
+        return q.where(self.lookup())  # pragma: no cover
+
+    def lookup(self):
+        """
+        Generate where-clause expression to locate this model instance.
+
+        By default this method uses current values of all primary keys, and you
+        can override it to behave differently. Most instance-level CRUD
+        operations depend on this method internally. Particularly while
+        :meth:`.lookup` is called in :meth:`.update`, the where condition is
+        used in :meth:`.UpdateRequest.apply`, so that queries like ``UPDATE ...
+        SET id = NEW WHERE id = OLD`` could work correctly.
+
+        :return:
+
+        .. versionadded:: 0.7.6
+
+        """
+        exps = []
+        for c in self.__table__.primary_key.columns:
+            exps.append(c == getattr(self, c.name))
+        if exps:
+            return sa.and_(*exps)
+        else:
+            raise LookupError('Instance-level CRUD operations not allowed on '
+                              'models without primary keys or lookup(), please'
+                              ' use model-level CRUD operations instead.')
 
     def _update(self, **values):
         return self._update_request_cls(self).update(**values)
@@ -551,7 +570,7 @@ async def _delete(self, bind=None, timeout=DEFAULT):
         cls = type(self)
         # noinspection PyUnresolvedReferences,PyProtectedMember
         cls._check_abstract()
-        clause = self.append_where_primary_key(cls.delete)
+        clause = cls.delete.where(self.lookup())
         if timeout is not DEFAULT:
             clause = clause.execution_options(timeout=timeout)
         if bind is None:

tests/test_crud.py

@@ -237,3 +237,48 @@ async def test_string_primary_key(engine):
         await Relation.create(bind=engine, timeout=10, name=r)
     r1 = await Relation.get(relations[0], bind=engine, timeout=10)
     assert r1.name == relations[0]
+
+
+async def test_lookup_287(bind):
+    from gino.exceptions import NoSuchRowError
+
+    class Game(db.Model):
+        __tablename__ = 'games'
+        game_id = db.Column(db.String(32), unique=True)
+        channel_id = db.Column(db.String(1), default='A')
+
+    await Game.gino.create()
+    try:
+        game_1 = await Game.create(game_id='1', channel_id='X')
+        game_2 = await Game.create(game_id='2', channel_id='Y')
+
+        # ordinary update should be fine
+        uq = game_1.update(game_id='3')
+
+        with pytest.raises(TypeError,
+                           match='Model Game has no table, primary key'):
+            # but applying the updates to DB should fail
+            await uq.apply()
+
+        with pytest.raises(LookupError,
+                           match='Instance-level CRUD operations not allowed'):
+            await game_2.delete()
+        with pytest.raises(LookupError,
+                           match='Instance-level CRUD operations not allowed'):
+            await game_2.query.gino.all()
+        with pytest.raises(LookupError,
+                           match='Instance-level CRUD operations not allowed'):
+            await game_2.select('game_id')
+
+        # previous ordinary update still in effect
+        assert game_1.game_id == '3'
+
+        assert await Game.select('game_id').gino.all() == [('1',), ('2',)]
+
+        Game.lookup = lambda self: Game.game_id == self.game_id
+        with pytest.raises(NoSuchRowError):
+            await game_1.update(channel_id='Z').apply()
+        await game_2.update(channel_id='Z').apply()
+        assert await Game.select('channel_id').gino.all() == [('X',), ('Z',)]
+    finally:
+        await Game.gino.drop()

tests/test_declarative.py

@@ -207,7 +207,7 @@ class AbstractModel(db.Model):
 
     req = am.update()
 
-    with pytest.raises(TypeError, match='AbstractModel is abstract'):
+    with pytest.raises(TypeError, match='AbstractModel has no table'):
         await req.apply()
 
     with pytest.raises(TypeError, match='AbstractModel is abstract'):

tests/test_json.py

@@ -93,6 +93,7 @@ async def test_non_jsonb(bind):
     class News(db.Model):
         __tablename__ = 'news'
 
+        id = db.Column(db.Integer(), primary_key=True)
         profile = db.Column(JSON(), nullable=False, server_default='{}')
         visits = db.IntegerProperty(default=0)