From: lizraeli <leo2002b@yahoo.com>
Date: Mon, 28 Oct 2019 19:33:41 +0000 (-0400)
Subject: Correctly interpret None passed to query.get(); warn for empty PK values
X-Git-Tag: rel_1_4_0b1~645^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=997f4b5f2b3b4725de0960824e95fcb1150ff215;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Correctly interpret None passed to query.get(); warn for empty PK values

A warning is emitted if a primary key value is passed to :meth:`.Query.get`
that consists of None for all primary key column positions.   Previously,
passing a single None outside of a tuple would raise a ``TypeError`` and
passing a composite None (tuple of None values) would silently pass
through.   The fix now coerces the single None into a tuple where it is
handled consistently with the other None conditions.  Thanks to Lev
Izraelit for the help with this.

Fixes #4915
Closes: #4917
Pull-request: https://github.com/sqlalchemy/sqlalchemy/pull/4917
Pull-request-sha: b388343c7cabeecf8c779689b78e638c23f9af40

Change-Id: Ibc6c27ccf50dfd4adbf15b6dbd393115c30d44fb
---

diff --git a/doc/build/changelog/unreleased_13/4915.rst b/doc/build/changelog/unreleased_13/4915.rst
new file mode 100644
index 0000000000..1048d06697
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4915.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4915
+
+    A warning is emitted if a primary key value is passed to :meth:`.Query.get`
+    that consists of None for all primary key column positions.   Previously,
+    passing a single None outside of a tuple would raise a ``TypeError`` and
+    passing a composite None (tuple of None values) would silently pass
+    through.   The fix now coerces the single None into a tuple where it is
+    handled consistently with the other None conditions.  Thanks to Lev
+    Izraelit for the help with this.
+
diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py
index 25ba8a3986..189da78bc5 100644
--- a/lib/sqlalchemy/orm/loading.py
+++ b/lib/sqlalchemy/orm/loading.py
@@ -243,6 +243,12 @@ def load_on_pk_identity(
             )
             _get_clause = sql_util.adapt_criterion_to_null(_get_clause, nones)
 
+            if len(nones) == len(primary_key_identity):
+                util.warn(
+                    "fully NULL primary key identity cannot load any "
+                    "object.  This condition may raise an error in a future "
+                    "release."
+                )
         _get_clause = q._adapt_clause(_get_clause, True, False)
         q._criterion = _get_clause
 
diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py
index d515fa83aa..80d5440687 100644
--- a/lib/sqlalchemy/orm/query.py
+++ b/lib/sqlalchemy/orm/query.py
@@ -977,7 +977,9 @@ class Query(Generative):
 
         is_dict = isinstance(primary_key_identity, dict)
         if not is_dict:
-            primary_key_identity = util.to_list(primary_key_identity)
+            primary_key_identity = util.to_list(
+                primary_key_identity, default=(None,)
+            )
 
         if len(primary_key_identity) != len(mapper.primary_key):
             raise sa_exc.InvalidRequestError(
diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index cdbeba1382..41d06d19ed 100644
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -112,22 +112,14 @@ class OnlyReturnTuplesTest(QueryTest):
 
     def test_multiple_entity_false(self):
         User = self.classes.User
-        query = (
-            create_session()
-            .query(User.id, User)
-            .only_return_tuples(False)
-        )
+        query = create_session().query(User.id, User).only_return_tuples(False)
         is_false(query.is_single_entity)
         row = query.first()
         assert isinstance(row, collections_abc.Sequence)
 
     def test_multiple_entity_true(self):
         User = self.classes.User
-        query = (
-            create_session()
-            .query(User.id, User)
-            .only_return_tuples(True)
-        )
+        query = create_session().query(User.id, User).only_return_tuples(True)
         is_false(query.is_single_entity)
         row = query.first()
         assert isinstance(row, collections_abc.Sequence)
@@ -780,10 +772,8 @@ class GetTest(QueryTest):
         q = s.query(User.id)
         assert_raises(sa_exc.InvalidRequestError, q.get, (5,))
 
-    def test_get_null_pk(self):
-        """test that a mapping which can have None in a
-        PK (i.e. map to an outerjoin) works with get()."""
-
+    @testing.fixture
+    def outerjoin_mapping(self):
         users, addresses = self.tables.users, self.tables.addresses
 
         s = users.outerjoin(addresses)
@@ -799,10 +789,44 @@ class GetTest(QueryTest):
                 "address_id": addresses.c.id,
             },
         )
+        return UserThing
+
+    def test_get_null_pk(self, outerjoin_mapping):
+        """test that a mapping which can have None in a
+        PK (i.e. map to an outerjoin) works with get()."""
+
+        UserThing = outerjoin_mapping
         sess = create_session()
         u10 = sess.query(UserThing).get((10, None))
         eq_(u10, UserThing(id=10))
 
+    def test_get_fully_null_pk(self):
+        User = self.classes.User
+
+        s = Session()
+        q = s.query(User)
+        assert_raises_message(
+            sa_exc.SAWarning,
+            r"fully NULL primary key identity cannot load any object.  "
+            "This condition may raise an error in a future release.",
+            q.get,
+            None,
+        )
+
+    def test_get_fully_null_composite_pk(self, outerjoin_mapping):
+        UserThing = outerjoin_mapping
+
+        s = Session()
+        q = s.query(UserThing)
+
+        assert_raises_message(
+            sa_exc.SAWarning,
+            r"fully NULL primary key identity cannot load any object.  "
+            "This condition may raise an error in a future release.",
+            q.get,
+            (None, None),
+        )
+
     def test_no_criterion(self):
         """test that get()/load() does not use preexisting filter/etc.
         criterion"""