pallets / jinja

A very fast and expressive template engine.
https://jinja.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
10.35k stars 1.62k forks source link

Cannot pickle `jinja2.utils.missing` #2027

Open mattclay opened 1 month ago

mattclay commented 1 month ago

The jinja2.utils.missing singleton instance cannot be pickled since pickle cannot find MissingType due to the jinja2.utils.MissingType attribute not existing:

import pickle
import jinja2.utils

pickle.dumps(jinja2.utils.missing)

Running the above code results in the following error:

Traceback (most recent call last):
  File "/tmp/bug.py", line 4, in <module>
    pickle.dumps(jinja2.utils.missing)
_pickle.PicklingError: Can't pickle <class 'jinja2.utils.MissingType'>: attribute lookup MissingType on jinja2.utils failed

Attempting to pickle (and then restore) the jinja2.utils.missing singleton instance should succeed. Supporting this would allow pickling of jinja2.runtime.Undefined instances which do not set obj, once https://github.com/pallets/jinja/issues/2025 is also resolved.

Environment:

davidism commented 1 month ago

Looks like a few folks have started looking at pickling all of a sudden. Note that it is not a goal of Jinja to have it's internal structure be pickleable. While I'm willing to consider it, it's not a priority. It might help if I understood why you were trying to do this.

nitzmahone commented 1 month ago

Yeah, it probably would've made more sense if the repro on this one was in the context of #2025.

In our case, it's not about pickling per se (agreed, generally silly for Jinja to support), but that pickle underlies the runtime-provided default impls for copy and deepcopy (which we do need). Various Ansible plugins and infrastructure running beneath Jinja templating operations need to be able to copy/deepcopy collections that may contain Undefined instances, which currently blows up if obj wasn't set (because it's an instance of the missing sentinel). My kingdom for PEP661, but until then... :laughing:

nitzmahone commented 1 month ago

(we also have a local monkeypatch workaround and a PR on the way for this one)

davidism commented 1 month ago

Right, but is there a reason to pass undefined around? It's probably coming from your use of native environment, but what's the purpose of passing it on? Maybe it would make more sense to discard undefined items (or items with undefined members) or replace them with none. Then you're storing plain Python data rather than internal Jinja state.

nitzmahone commented 1 month ago

Maybe it would make more sense to discard undefined items

We eventually do, but only at the very end of the overall templating pipeline before we stash the result. Ansible has always had a gnarly layer around Jinja that adds recursive/chained/indirect templating, and we've been working in a private branch to improve performance and tighten up our interactions with Jinja. Part of that increases our reliance on several custom Undefined subclasses to capture and defer various kinds of errors as the results flow through the pipeline. User plugin code (custom filters/tests, as well as Ansible-specific Jinja plugins) can interact with these objects mid-stream, so it's not uncommon for folks to e.g., copy/deepcopy a dictionary that came into a filter and do stuff with it. There may be Undefined objects embedded in those dicts/lists from previous template/var operations (I'm oversimplifying- you probably don't want to know the whole story, but I can show you if you really want to know). Ultimately, we can't let the operation fail unless the user code directly interacts with an Undefined (just copying a collection that contains one doesn't count) or if the final result still contains any Undefined objects as it's leaving the templating subsystem (the one time we actually recursively traverse the entire structure).