From: Mike Bayer Date: Mon, 13 May 2019 18:56:12 +0000 (-0400) Subject: Continue to assume None for un-accessed attribute on persistent X-Git-Tag: rel_1_3_4~15 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=47e7cb92b1a8df83d926038ce456dbe12f4aba32;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Continue to assume None for un-accessed attribute on persistent object during m2o fetch Fixed regression in new relationship m2o comparison logic first introduced at :ref:`change_4359` when comparing to an attribute that is persisted as NULL and is in an un-fetched state in the mapped instance. Since the attribute has no explicit default, it needs to default to NULL when accessed in a persistent setting. Fixes: #4676 Change-Id: I17160c30131187c735f025a785ff0276a246f6bb (cherry picked from commit b6619f267ba6a327d6bff1aa650dd3ab9b718b29) --- diff --git a/doc/build/changelog/unreleased_13/4676.rst b/doc/build/changelog/unreleased_13/4676.rst new file mode 100644 index 0000000000..aa814be871 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4676.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm + :tickets: 4676 + + Fixed regression in new relationship m2o comparison logic first introduced + at :ref:`change_4359` when comparing to an attribute that is persisted as + NULL and is in an un-fetched state in the mapped instance. Since the + attribute has no explicit default, it needs to default to NULL when + accessed in a persistent setting. + diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index 008cfcd2a8..b40fad332b 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -1574,7 +1574,7 @@ class RelationshipProperty(StrategizedProperty): state, dict_, column, - passive=attributes.PASSIVE_RETURN_NEVER_SET + passive=attributes.PASSIVE_OFF if state.persistent else attributes.PASSIVE_NO_FETCH ^ attributes.INIT_OK, ) diff --git a/test/orm/_fixtures.py b/test/orm/_fixtures.py index bb51777a31..2b4fca1489 100644 --- a/test/orm/_fixtures.py +++ b/test/orm/_fixtures.py @@ -48,6 +48,9 @@ class FixtureTest(fixtures.MappedTest): class Dingaling(Base): pass + class HasDingaling(Base): + pass + class Node(Base): pass @@ -223,6 +226,17 @@ class FixtureTest(fixtures.MappedTest): test_needs_fk=True, ) + Table( + "has_dingaling", + metadata, + Column( + "id", Integer, primary_key=True, test_needs_autoincrement=True + ), + Column("dingaling_id", None, ForeignKey("dingalings.id")), + test_needs_acid=True, + test_needs_fk=True, + ) + Table( "items", metadata, diff --git a/test/orm/test_naturalpks.py b/test/orm/test_naturalpks.py index 52c27f3158..982f342417 100644 --- a/test/orm/test_naturalpks.py +++ b/test/orm/test_naturalpks.py @@ -16,6 +16,7 @@ from sqlalchemy.orm.session import make_transient from sqlalchemy.testing import assert_raises from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import eq_ +from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures from sqlalchemy.testing import ne_ from sqlalchemy.testing.schema import Column @@ -677,6 +678,17 @@ class NaturalPKTest(fixtures.MappedTest): del u2.username + # object is persistent, so since we deleted, we get None + with expect_warnings("Got None for value of column "): + eq_(expr.left.callable(), None) + + s.expunge(u2) + + # however that None isn't in the dict, that's just the default + # attribute value, so after expunge it's gone + assert "username" not in u2.__dict__ + + # detached, we don't have it assert_raises_message( sa.exc.InvalidRequestError, "Can't resolve value for column users.username", diff --git a/test/orm/test_query.py b/test/orm/test_query.py index b31647a725..86b304be9d 100644 --- a/test/orm/test_query.py +++ b/test/orm/test_query.py @@ -4604,8 +4604,18 @@ class WithTransientOnNone(_fixtures.FixtureTest, AssertsCompiledSQL): __dialect__ = "default" def _fixture1(self): - User, Address = self.classes.User, self.classes.Address - users, addresses = self.tables.users, self.tables.addresses + User, Address, Dingaling, HasDingaling = ( + self.classes.User, + self.classes.Address, + self.classes.Dingaling, + self.classes.HasDingaling, + ) + users, addresses, dingalings, has_dingaling = ( + self.tables.users, + self.tables.addresses, + self.tables.dingalings, + self.tables.has_dingaling, + ) mapper(User, users) mapper( @@ -4622,6 +4632,20 @@ class WithTransientOnNone(_fixtures.FixtureTest, AssertsCompiledSQL): ), }, ) + mapper(Dingaling, dingalings) + mapper( + HasDingaling, + has_dingaling, + properties={ + "dingaling": relationship( + Dingaling, + primaryjoin=and_( + dingalings.c.id == has_dingaling.c.dingaling_id, + dingalings.c.data == "hi", + ), + ) + }, + ) def test_filter_with_transient_dont_assume_pk(self): self._fixture1() @@ -4696,6 +4720,86 @@ class WithTransientOnNone(_fixtures.FixtureTest, AssertsCompiledSQL): checkparams={"param_1": None, "param_2": None}, ) + def test_filter_with_persistent_non_pk_col_is_default_null(self): + # test #4676 - comparison to a persistent column that is + # NULL in the database, but is not fetched + self._fixture1() + Dingaling, HasDingaling = ( + self.classes.Dingaling, + self.classes.HasDingaling, + ) + s = Session() + d = Dingaling(id=1) + s.add(d) + s.flush() + assert "data" not in d.__dict__ + + q = s.query(HasDingaling).filter_by(dingaling=d) + + with expect_warnings("Got None for value of column"): + self.assert_compile( + q, + "SELECT has_dingaling.id AS has_dingaling_id, " + "has_dingaling.dingaling_id AS has_dingaling_dingaling_id " + "FROM has_dingaling WHERE :param_1 = " + "has_dingaling.dingaling_id AND :param_2 = :data_1", + checkparams={"param_1": 1, "param_2": None, "data_1": "hi"}, + ) + + def test_filter_with_detached_non_pk_col_is_default_null(self): + self._fixture1() + Dingaling, HasDingaling = ( + self.classes.Dingaling, + self.classes.HasDingaling, + ) + s = Session() + d = Dingaling() + s.add(d) + s.flush() + s.commit() + d.id + s.expire(d, ["data"]) + s.expunge(d) + assert "data" not in d.__dict__ + assert "id" in d.__dict__ + + q = s.query(HasDingaling).filter_by(dingaling=d) + + # this case we still can't handle, object is detached so we assume + # nothing + + assert_raises_message( + sa_exc.StatementError, + r"Can't resolve value for column dingalings.data on " + r"object .*Dingaling.* the object is detached and " + r"the value was expired", + q.all, + ) + + def test_filter_with_detached_non_pk_col_has_value(self): + self._fixture1() + Dingaling, HasDingaling = ( + self.classes.Dingaling, + self.classes.HasDingaling, + ) + s = Session() + d = Dingaling(data="some data") + s.add(d) + s.commit() + s.expire(d) + assert "data" not in d.__dict__ + + q = s.query(HasDingaling).filter_by(dingaling=d) + + self.assert_compile( + q, + "SELECT has_dingaling.id AS has_dingaling_id, " + "has_dingaling.dingaling_id AS has_dingaling_dingaling_id " + "FROM has_dingaling WHERE :param_1 = " + "has_dingaling.dingaling_id AND :param_2 = :data_1", + checkparams={"param_1": 1, "param_2": "some data", "data_1": "hi"}, + ) + def test_with_parent_with_transient_assume_pk(self): self._fixture1() User, Address = self.classes.User, self.classes.Address