Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve __slots__ metadata on Undefined types #2026

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Unreleased
- Use modern packaging metadata with ``pyproject.toml`` instead of ``setup.cfg``.
:pr:`1793`
- Use ``flit_core`` instead of ``setuptools`` as build backend.
- Fix copy/deepcopy/pickle support for ``Undefined`` objects.
:pr:`2026`


Version 3.1.5
Expand Down
30 changes: 17 additions & 13 deletions src/jinja2/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,11 @@ def _fail_with_undefined_error(

@internalcode
def __getattr__(self, name: str) -> t.Any:
if name[:2] == "__":
# Raise AttributeError on requests for names that appear to be unimplemented
# dunder methods to keep Python's internal protocol probing behaviors working
# properly in cases where another exception type could cause unexpected or
# difficult-to-diagnose failures.
if name[:2] == "__" and name[-2:] == "__":
raise AttributeError(name)

return self._fail_with_undefined_error()
Expand Down Expand Up @@ -982,10 +986,20 @@ class ChainableUndefined(Undefined):
def __html__(self) -> str:
return str(self)

def __getattr__(self, _: str) -> "ChainableUndefined":
def __getattr__(self, name: str) -> "ChainableUndefined":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change. Can you make the comment clearer?

Copy link
Author

@nitzmahone nitzmahone Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same (incomplete, BTW) heuristic exclusion already used by Undefined.__getattr__. ChainableUndefined needs it for the same reason, but there weren't previously any tests that exhibited the bugs caused by the current unconditional behavior.

There are a number of Python interaction protocols that sniff for the presence of a particular dunder method to support builtin behavior. This is one of the places where Jinja's getattr/getitem equivalency causes problems. In this case, copy uses the presence of a __setstate__ method on the type to decide how it will create and populate the copy. Since hasattr(ChainableUndefined, '__setstate__') is true with the current impl, Python assumes it can call that to populate the empty storage for the copy, but blows up when it actually tries to invoke the Undefined object it receives instead of a bound method to fill in an empty object instance.

In general, __getattr__ should raise AttibuteError on a request for any dunder method it doesn't know about- there are all sorts of weird things that can happen in various places in Python if an object confuses the runtime about its support for a particular interaction protocol.

Happy to include a more detailed inline explanation along those lines in the code. but should probably either copy/paste to the original usage or actually share that logic between them.

Also happy to correct the heuristic in both places to only exclude __XYZ__ instead of the current __.* - while probably unlikely, the current impl would incorrectly exclude, eg __fooattr.

# Raise AttributeError on requests for names that appear to be unimplemented
# dunder methods to avoid confusing Python with truthy non-method objects that
# do not implement the protocol being probed for. e.g., copy.copy(Undefined())
# fails spectacularly if getattr(Undefined(), '__setstate__') returns an
# Undefined object instead of raising AttributeError to signal that it does not
# support that style of object initialization.
if name[:2] == "__" and name[-2:] == "__":
raise AttributeError(name)

return self

__getitem__ = __getattr__ # type: ignore
def __getitem__(self, _name: str) -> "ChainableUndefined": # type: ignore[override]
return self


class DebugUndefined(Undefined):
Expand Down Expand Up @@ -1044,13 +1058,3 @@ class StrictUndefined(Undefined):
__iter__ = __str__ = __len__ = Undefined._fail_with_undefined_error
__eq__ = __ne__ = __bool__ = __hash__ = Undefined._fail_with_undefined_error
__contains__ = Undefined._fail_with_undefined_error


# Remove slots attributes, after the metaclass is applied they are
# unneeded and contain wrong data for subclasses.
del (
Undefined.__slots__,
ChainableUndefined.__slots__,
DebugUndefined.__slots__,
StrictUndefined.__slots__,
)
8 changes: 0 additions & 8 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,6 @@ def test_default_undefined(self):
assert und1 == und2
assert und1 != 42
assert hash(und1) == hash(und2) == hash(Undefined())
with pytest.raises(AttributeError):
getattr(Undefined, "__slots__") # noqa: B009

def test_chainable_undefined(self):
env = Environment(undefined=ChainableUndefined)
Expand All @@ -335,8 +333,6 @@ def test_chainable_undefined(self):
assert env.from_string("{{ foo.missing }}").render(foo=42) == ""
assert env.from_string("{{ not missing }}").render() == "True"
pytest.raises(UndefinedError, env.from_string("{{ missing - 1}}").render)
with pytest.raises(AttributeError):
getattr(ChainableUndefined, "__slots__") # noqa: B009

# The following tests ensure subclass functionality works as expected
assert env.from_string('{{ missing.bar["baz"] }}').render() == ""
Expand Down Expand Up @@ -368,8 +364,6 @@ def test_debug_undefined(self):
str(DebugUndefined(hint=undefined_hint))
== f"{{{{ undefined value printed: {undefined_hint} }}}}"
)
with pytest.raises(AttributeError):
getattr(DebugUndefined, "__slots__") # noqa: B009

def test_strict_undefined(self):
env = Environment(undefined=StrictUndefined)
Expand All @@ -386,8 +380,6 @@ def test_strict_undefined(self):
env.from_string('{{ missing|default("default", true) }}').render()
== "default"
)
with pytest.raises(AttributeError):
getattr(StrictUndefined, "__slots__") # noqa: B009
assert env.from_string('{{ "foo" if false }}').render() == ""

def test_indexing_gives_undefined(self):
Expand Down
50 changes: 50 additions & 0 deletions tests/test_runtime.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
import copy
import itertools
import pickle

import pytest

from jinja2 import ChainableUndefined
from jinja2 import DebugUndefined
from jinja2 import StrictUndefined
from jinja2 import Template
from jinja2 import TemplateRuntimeError
from jinja2 import Undefined
from jinja2.runtime import LoopContext

TEST_IDX_TEMPLATE_STR_1 = (
Expand Down Expand Up @@ -73,3 +82,44 @@ def __call__(self, *args, **kwargs):
out = t.render(calc=Calc())
# Would be "1" if context argument was passed.
assert out == "0"


_undefined_types = (Undefined, ChainableUndefined, DebugUndefined, StrictUndefined)


@pytest.mark.parametrize("undefined_type", _undefined_types)
def test_undefined_copy(undefined_type):
undef = undefined_type("a hint", ["foo"], "a name", TemplateRuntimeError)
copied = copy.copy(undef)

assert copied is not undef
assert copied._undefined_hint is undef._undefined_hint
assert copied._undefined_obj is undef._undefined_obj
assert copied._undefined_name is undef._undefined_name
assert copied._undefined_exception is undef._undefined_exception


@pytest.mark.parametrize("undefined_type", _undefined_types)
def test_undefined_deepcopy(undefined_type):
undef = undefined_type("a hint", ["foo"], "a name", TemplateRuntimeError)
copied = copy.deepcopy(undef)

assert copied._undefined_hint is undef._undefined_hint
assert copied._undefined_obj is not undef._undefined_obj
assert copied._undefined_obj == undef._undefined_obj
assert copied._undefined_name is undef._undefined_name
assert copied._undefined_exception is undef._undefined_exception


@pytest.mark.parametrize("undefined_type", _undefined_types)
def test_undefined_pickle(undefined_type):
undef = undefined_type("a hint", ["foo"], "a name", TemplateRuntimeError)
copied = pickle.loads(pickle.dumps(undef))

assert copied._undefined_hint is not undef._undefined_hint
assert copied._undefined_hint == undef._undefined_hint
assert copied._undefined_obj is not undef._undefined_obj
assert copied._undefined_obj == undef._undefined_obj
assert copied._undefined_name is not undef._undefined_name
assert copied._undefined_name == undef._undefined_name
assert copied._undefined_exception is undef._undefined_exception