From: Mike Bayer Date: Tue, 7 Apr 2020 21:37:14 +0000 (-0400) Subject: Use dot-separated name resolution for relationship target X-Git-Tag: rel_1_3_16~1^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=53591fc18dbf879c5fbda04f675b6341056f9781;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Use dot-separated name resolution for relationship target The string argument accepted as the first positional argument by the :func:`.relationship` function when using the Declarative API is no longer interpreted using the Python ``eval()`` function; instead, the name is dot separated and the names are looked up directly in the name resolution dictionary without treating the value as a Python expression. However, passing a string argument to the other :func:`.relationship` parameters that necessarily must accept Python expressions will still use ``eval()``; the documentation has been clarified to ensure that there is no ambiguity that this is in use. Fixes: #5238 Change-Id: Id802f403190adfab0ca034afe2214ba10fd9cfbb (cherry picked from commit 17e31604ae13ebd58b148a4319cfed09e5448ee2) --- diff --git a/doc/build/changelog/unreleased_13/5238.rst b/doc/build/changelog/unreleased_13/5238.rst new file mode 100644 index 0000000000..1fb54ebe74 --- /dev/null +++ b/doc/build/changelog/unreleased_13/5238.rst @@ -0,0 +1,17 @@ +.. change:: + :tags: bug, orm, declarative + :tickets: 5238 + + The string argument accepted as the first positional argument by the + :func:`.relationship` function when using the Declarative API is no longer + interpreted using the Python ``eval()`` function; instead, the name is dot + separated and the names are looked up directly in the name resolution + dictionary without treating the value as a Python expression. However, + passing a string argument to the other :func:`.relationship` parameters + that necessarily must accept Python expressions will still use ``eval()``; + the documentation has been clarified to ensure that there is no ambiguity + that this is in use. + + .. seealso:: + + :ref:`declarative_relationship_eval` - details on string evaluation \ No newline at end of file diff --git a/doc/build/orm/basic_relationships.rst b/doc/build/orm/basic_relationships.rst index ace6f8d7e3..0f098d4cd6 100644 --- a/doc/build/orm/basic_relationships.rst +++ b/doc/build/orm/basic_relationships.rst @@ -240,6 +240,13 @@ is accepted as well, matching the name of the table as stored in ``Base.metadata secondary="association", backref="parents") +.. warning:: When passed as a Python-evaluable string, the + :paramref:`.relationship.secondary` argument is interpreted using Python's + ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. See + :ref:`declarative_relationship_eval` for details on declarative + evaluation of :func:`.relationship` arguments. + + .. _relationships_many_to_many_deletion: Deleting Rows from the Many to Many Table diff --git a/doc/build/orm/extensions/declarative/relationships.rst b/doc/build/orm/extensions/declarative/relationships.rst index 1763344c76..07b6ed5bfd 100644 --- a/doc/build/orm/extensions/declarative/relationships.rst +++ b/doc/build/orm/extensions/declarative/relationships.rst @@ -44,13 +44,21 @@ class using them:: user_id = Column(Integer, ForeignKey('users.id')) user = relationship(User, primaryjoin=user_id == User.id) +.. _declarative_relationship_eval: + +Evaluation of relationship arguments +===================================== + In addition to the main argument for :func:`~sqlalchemy.orm.relationship`, other arguments which depend upon the columns present on an as-yet -undefined class may also be specified as strings. These strings are -evaluated as Python expressions. The full namespace available within -this evaluation includes all classes mapped for this declarative base, -as well as the contents of the ``sqlalchemy`` package, including -expression functions like :func:`~sqlalchemy.sql.expression.desc` and +undefined class may also be specified as strings. For most of these +arguments except that of the main argument, these strings are +**evaluated as Python expressions using Python's built-in eval() function.** + +The full namespace available within this evaluation includes all classes mapped +for this declarative base, as well as the contents of the ``sqlalchemy`` +package, including expression functions like +:func:`~sqlalchemy.sql.expression.desc` and :attr:`~sqlalchemy.sql.expression.func`:: class User(Base): @@ -59,6 +67,37 @@ expression functions like :func:`~sqlalchemy.sql.expression.desc` and order_by="desc(Address.email)", primaryjoin="Address.user_id==User.id") +.. warning:: + + The strings accepted by the following parameters: + + :paramref:`.relationship.order_by` + + :paramref:`.relationship.primaryjoin` + + :paramref:`.relationship.secondaryjoin` + + :paramref:`.relationship.secondary` + + :paramref:`.relationship.remote_side` + + :paramref:`.relationship.foreign_keys` + + :paramref:`.relationship._user_defined_foreign_keys` + + Are **evaluated as Python code expressions using eval(). DO NOT PASS + UNTRUSTED INPUT TO THESE ARGUMENTS.** + + In addition, prior to version 1.3.16 of SQLAlchemy, the main + "argument" to :func:`.relationship` is also evaluated as Python + code. **DO NOT PASS UNTRUSTED INPUT TO THIS ARGUMENT.** + +.. versionchanged:: 1.3.16 + + The string evaluation of the main "argument" no longer accepts an open + ended Python expression, instead only accepting a string class name + or dotted package-qualified name. + For the case where more than one module contains a class of the same name, string class names can also be specified as module-qualified paths within any of these string expressions:: diff --git a/doc/build/orm/join_conditions.rst b/doc/build/orm/join_conditions.rst index ef023feb47..a317c6eccb 100644 --- a/doc/build/orm/join_conditions.rst +++ b/doc/build/orm/join_conditions.rst @@ -96,6 +96,13 @@ one :class:`.Column` we need:: billing_address = relationship("Address", foreign_keys="Customer.billing_address_id") +.. warning:: When passed as a Python-evaluable string, the + :paramref:`.relationship.foreign_keys` argument is interpreted using Python's + ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. See + :ref:`declarative_relationship_eval` for details on declarative + evaluation of :func:`.relationship` arguments. + + .. _relationship_primaryjoin: Specifying Alternate Join Conditions @@ -138,12 +145,21 @@ load those ``Address`` objects which specify a city of "Boston":: state = Column(String) zip = Column(String) -Within this string SQL expression, we made use of the :func:`.and_` conjunction construct to establish -two distinct predicates for the join condition - joining both the ``User.id`` and -``Address.user_id`` columns to each other, as well as limiting rows in ``Address`` -to just ``city='Boston'``. When using Declarative, rudimentary SQL functions like -:func:`.and_` are automatically available in the evaluated namespace of a string -:func:`.relationship` argument. +Within this string SQL expression, we made use of the :func:`.and_` conjunction +construct to establish two distinct predicates for the join condition - joining +both the ``User.id`` and ``Address.user_id`` columns to each other, as well as +limiting rows in ``Address`` to just ``city='Boston'``. When using +Declarative, rudimentary SQL functions like :func:`.and_` are automatically +available in the evaluated namespace of a string :func:`.relationship` +argument. + +.. warning:: When passed as a Python-evaluable string, the + :paramref:`.relationship.primaryjoin` argument is interpreted using + Python's + ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. See + :ref:`declarative_relationship_eval` for details on declarative + evaluation of :func:`.relationship` arguments. + The custom criteria we use in a :paramref:`~.relationship.primaryjoin` is generally only significant when SQLAlchemy is rendering SQL in @@ -557,6 +573,14 @@ use the string name of the table as it is present in the :class:`.MetaData`:: backref="left_nodes" ) +.. warning:: When passed as a Python-evaluable string, the + :paramref:`.relationship.primaryjoin` and + :paramref:`.relationship.secondaryjoin` arguments are interpreted using + Python's ``eval()`` function. **DO NOT PASS UNTRUSTED INPUT TO THESE + STRINGS**. See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + + A classical mapping situation here is similar, where ``node_to_node`` can be joined to ``node.c.id``:: diff --git a/lib/sqlalchemy/ext/declarative/clsregistry.py b/lib/sqlalchemy/ext/declarative/clsregistry.py index 57a6d1a05d..7d91d1801e 100644 --- a/lib/sqlalchemy/ext/declarative/clsregistry.py +++ b/lib/sqlalchemy/ext/declarative/clsregistry.py @@ -289,6 +289,38 @@ class _class_resolver(object): return self.fallback[key] + def _raise_for_name(self, name, err): + util.raise_( + exc.InvalidRequestError( + "When initializing mapper %s, expression %r failed to " + "locate a name (%r). If this is a class name, consider " + "adding this relationship() to the %r class after " + "both dependent classes have been defined." + % (self.prop.parent, self.arg, name, self.cls) + ), + from_=err, + ) + + def _resolve_name(self): + name = self.arg + d = self._dict + rval = None + try: + for token in name.split("."): + if rval is None: + rval = d[token] + else: + rval = getattr(rval, token) + except KeyError as err: + self._raise_for_name(name, err) + except NameError as n: + self._raise_for_name(n.args[0], n) + else: + if isinstance(rval, _GetColumns): + return rval.cls + else: + return rval + def __call__(self): try: x = eval(self.arg, globals(), self._dict) @@ -298,16 +330,7 @@ class _class_resolver(object): else: return x except NameError as n: - util.raise_( - exc.InvalidRequestError( - "When initializing mapper %s, expression %r failed to " - "locate a name (%r). If this is a class name, consider " - "adding this relationship() to the %r class after " - "both dependent classes have been defined." - % (self.prop.parent, self.arg, n.args[0], self.cls) - ), - from_=n, - ) + self._raise_for_name(n.args[0], n) def _resolver(cls, prop): @@ -320,16 +343,18 @@ def _resolver(cls, prop): def resolve_arg(arg): return _class_resolver(cls, prop, fallback, arg) - return resolve_arg + def resolve_name(arg): + return _class_resolver(cls, prop, fallback, arg)._resolve_name + + return resolve_name, resolve_arg def _deferred_relationship(cls, prop): if isinstance(prop, RelationshipProperty): - resolve_arg = _resolver(cls, prop) + resolve_name, resolve_arg = _resolver(cls, prop) for attr in ( - "argument", "order_by", "primaryjoin", "secondaryjoin", @@ -341,6 +366,11 @@ def _deferred_relationship(cls, prop): if isinstance(v, util.string_types): setattr(prop, attr, resolve_arg(v)) + for attr in ("argument",): + v = getattr(prop, attr) + if isinstance(v, util.string_types): + setattr(prop, attr, resolve_name(v)) + if prop.backref and isinstance(prop.backref, tuple): key, kwargs = prop.backref for attr in ( diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index c4a6ac26e1..de372eb414 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -221,7 +221,19 @@ class RelationshipProperty(StrategizedProperty): :paramref:`~.relationship.argument` may also be passed as a callable function which is evaluated at mapper initialization time, and may - be passed as a Python-evaluable string when using Declarative. + be passed as a string name when using Declarative. + + .. warning:: Prior to SQLAlchemy 1.3.16, this value is interpreted + using Python's ``eval()`` function. + **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. + See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + + .. versionchanged 1.3.16:: + + The string evaluation of the main "argument" no longer accepts an + open ended Python expression, instead only accepting a string + class name or dotted package-qualified name. .. seealso:: @@ -241,6 +253,12 @@ class RelationshipProperty(StrategizedProperty): present in the :class:`.MetaData` collection associated with the parent-mapped :class:`.Table`. + .. warning:: When passed as a Python-evaluable string, the + argument is interpreted using Python's ``eval()`` function. + **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. + See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + The :paramref:`~.relationship.secondary` keyword argument is typically applied in the case where the intermediary :class:`.Table` is not otherwise expressed in any direct class mapping. If the @@ -479,6 +497,12 @@ class RelationshipProperty(StrategizedProperty): and may be passed as a Python-evaluable string when using Declarative. + .. warning:: When passed as a Python-evaluable string, the + argument is interpreted using Python's ``eval()`` function. + **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. + See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + .. seealso:: :ref:`relationship_foreign_keys` @@ -638,6 +662,12 @@ class RelationshipProperty(StrategizedProperty): function which is evaluated at mapper initialization time, and may be passed as a Python-evaluable string when using Declarative. + .. warning:: When passed as a Python-evaluable string, the + argument is interpreted using Python's ``eval()`` function. + **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. + See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + :param passive_deletes=False: Indicates loading behavior during delete operations. @@ -730,6 +760,12 @@ class RelationshipProperty(StrategizedProperty): and may be passed as a Python-evaluable string when using Declarative. + .. warning:: When passed as a Python-evaluable string, the + argument is interpreted using Python's ``eval()`` function. + **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. + See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + .. seealso:: :ref:`relationship_primaryjoin` @@ -743,6 +779,12 @@ class RelationshipProperty(StrategizedProperty): and may be passed as a Python-evaluable string when using Declarative. + .. warning:: When passed as a Python-evaluable string, the + argument is interpreted using Python's ``eval()`` function. + **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. + See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + .. seealso:: :ref:`self_referential` - in-depth explanation of how @@ -777,6 +819,12 @@ class RelationshipProperty(StrategizedProperty): and may be passed as a Python-evaluable string when using Declarative. + .. warning:: When passed as a Python-evaluable string, the + argument is interpreted using Python's ``eval()`` function. + **DO NOT PASS UNTRUSTED INPUT TO THIS STRING**. + See :ref:`declarative_relationship_eval` for details on + declarative evaluation of :func:`.relationship` arguments. + .. seealso:: :ref:`relationship_primaryjoin` diff --git a/test/ext/declarative/test_clsregistry.py b/test/ext/declarative/test_clsregistry.py index d61cc81c83..fbde544e4f 100644 --- a/test/ext/declarative/test_clsregistry.py +++ b/test/ext/declarative/test_clsregistry.py @@ -49,13 +49,16 @@ class ClsRegistryTest(fixtures.TestBase): f2 = MockClass(base, "foo.alt.Foo") clsregistry.add_class("Foo", f1) clsregistry.add_class("Foo", f2) - resolver = clsregistry._resolver(f1, MockProp()) + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) gc_collect() is_(resolver("foo.bar.Foo")(), f1) is_(resolver("foo.alt.Foo")(), f2) + is_(name_resolver("foo.bar.Foo")(), f1) + is_(name_resolver("foo.alt.Foo")(), f2) + def test_fragment_resolve(self): base = weakref.WeakValueDictionary() f1 = MockClass(base, "foo.bar.Foo") @@ -64,13 +67,16 @@ class ClsRegistryTest(fixtures.TestBase): clsregistry.add_class("Foo", f1) clsregistry.add_class("Foo", f2) clsregistry.add_class("HoHo", f3) - resolver = clsregistry._resolver(f1, MockProp()) + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) gc_collect() is_(resolver("bar.Foo")(), f1) is_(resolver("alt.Foo")(), f2) + is_(name_resolver("bar.Foo")(), f1) + is_(name_resolver("alt.Foo")(), f2) + def test_fragment_ambiguous(self): base = weakref.WeakValueDictionary() f1 = MockClass(base, "foo.bar.Foo") @@ -79,7 +85,7 @@ class ClsRegistryTest(fixtures.TestBase): clsregistry.add_class("Foo", f1) clsregistry.add_class("Foo", f2) clsregistry.add_class("Foo", f3) - resolver = clsregistry._resolver(f1, MockProp()) + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) gc_collect() @@ -91,6 +97,39 @@ class ClsRegistryTest(fixtures.TestBase): resolver("alt.Foo"), ) + assert_raises_message( + exc.InvalidRequestError, + 'Multiple classes found for path "alt.Foo" in the registry ' + "of this declarative base. Please use a fully " + "module-qualified path.", + name_resolver("alt.Foo"), + ) + + def test_no_fns_in_name_resolve(self): + base = weakref.WeakValueDictionary() + f1 = MockClass(base, "foo.bar.Foo") + f2 = MockClass(base, "foo.alt.Foo") + clsregistry.add_class("Foo", f1) + clsregistry.add_class("Foo", f2) + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) + + gc_collect() + + import sqlalchemy + + is_( + resolver("__import__('sqlalchemy.util').util.EMPTY_SET")(), + sqlalchemy.util.EMPTY_SET, + ) + + assert_raises_message( + exc.InvalidRequestError, + r"When initializing mapper some_parent, expression " + r"\"__import__\('sqlalchemy.util'\).util.EMPTY_SET\" " + "failed to locate a name", + name_resolver("__import__('sqlalchemy.util').util.EMPTY_SET"), + ) + def test_resolve_dupe_by_name(self): base = weakref.WeakValueDictionary() f1 = MockClass(base, "foo.bar.Foo") @@ -100,7 +139,7 @@ class ClsRegistryTest(fixtures.TestBase): gc_collect() - resolver = clsregistry._resolver(f1, MockProp()) + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) resolver = resolver("Foo") assert_raises_message( exc.InvalidRequestError, @@ -110,6 +149,15 @@ class ClsRegistryTest(fixtures.TestBase): resolver, ) + resolver = name_resolver("Foo") + assert_raises_message( + exc.InvalidRequestError, + 'Multiple classes found for path "Foo" in the ' + "registry of this declarative base. Please use a " + "fully module-qualified path.", + resolver, + ) + def test_dupe_classes_back_to_one(self): base = weakref.WeakValueDictionary() f1 = MockClass(base, "foo.bar.Foo") @@ -121,9 +169,12 @@ class ClsRegistryTest(fixtures.TestBase): gc_collect() # registry restores itself to just the one class - resolver = clsregistry._resolver(f1, MockProp()) - resolver = resolver("Foo") - is_(resolver(), f1) + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) + f_resolver = resolver("Foo") + is_(f_resolver(), f1) + + f_resolver = name_resolver("Foo") + is_(f_resolver(), f1) def test_dupe_classes_cleanout(self): # force this to maintain isolation between tests @@ -156,13 +207,21 @@ class ClsRegistryTest(fixtures.TestBase): dupe_reg = base["Foo"] dupe_reg.contents = [lambda: None] - resolver = clsregistry._resolver(f1, MockProp()) - resolver = resolver("Foo") + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) + f_resolver = resolver("Foo") assert_raises_message( exc.InvalidRequestError, r"When initializing mapper some_parent, expression " r"'Foo' failed to locate a name \('Foo'\).", - resolver, + f_resolver, + ) + + f_resolver = name_resolver("Foo") + assert_raises_message( + exc.InvalidRequestError, + r"When initializing mapper some_parent, expression " + r"'Foo' failed to locate a name \('Foo'\).", + f_resolver, ) def test_module_reg_cleanout_race(self): @@ -175,14 +234,22 @@ class ClsRegistryTest(fixtures.TestBase): reg = base["_sa_module_registry"] mod_entry = reg["foo"]["bar"] - resolver = clsregistry._resolver(f1, MockProp()) - resolver = resolver("foo") + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) + f_resolver = resolver("foo") del mod_entry.contents["Foo"] assert_raises_message( AttributeError, "Module 'bar' has no mapped classes registered " "under the name 'Foo'", - lambda: resolver().bar.Foo, + lambda: f_resolver().bar.Foo, + ) + + f_resolver = name_resolver("foo") + assert_raises_message( + AttributeError, + "Module 'bar' has no mapped classes registered " + "under the name 'Foo'", + lambda: f_resolver().bar.Foo, ) def test_module_reg_no_class(self): @@ -191,13 +258,21 @@ class ClsRegistryTest(fixtures.TestBase): clsregistry.add_class("Foo", f1) reg = base["_sa_module_registry"] mod_entry = reg["foo"]["bar"] # noqa - resolver = clsregistry._resolver(f1, MockProp()) - resolver = resolver("foo") + name_resolver, resolver = clsregistry._resolver(f1, MockProp()) + f_resolver = resolver("foo") + assert_raises_message( + AttributeError, + "Module 'bar' has no mapped classes registered " + "under the name 'Bat'", + lambda: f_resolver().bar.Bat, + ) + + f_resolver = name_resolver("foo") assert_raises_message( AttributeError, "Module 'bar' has no mapped classes registered " "under the name 'Bat'", - lambda: resolver().bar.Bat, + lambda: f_resolver().bar.Bat, ) def test_module_reg_cleanout_two_sub(self):