Skip to content

fix: avoid infinite recursion when accessing _copier_conf.answers_file via Jinja context hook #2114

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sisp
Copy link
Member

@sisp sisp commented Apr 29, 2025

I've fixed a regression introduced by #2023 which causes infinite recursion when accessing _copier_conf.answers_file via Jinja context hook with copier-templates-extensions.

When rendering the answers file path in Worker.answers_relpath, which is accessible in the render context as _copier_conf.answers_file, the render context also includes _copier_conf.answers_file as a lazy dict value of _copier_conf. That's no problem when simply rendering the answers file path exposed as _copier_conf.answers_file because the answers file template never uses this variable itself. But when a context hook accesses _copier_conf.answers_file (e.g. in github.com/scientific-python/cookie:helpers/extensions.py#L17), then one invocation of the context hook intercepts the render context for rendering the template for that same value, which results in an infinite recursion. To avoid it, I've extended the LazyDict class to support setting entries and overwrote the answers_file entry with "". This is a bit of a hack, but I don't see another way to avoid this problem. I'll also submit a PR to copier-templates-extensions with a new test case for this scenario once we've agreed on the path forward here, @pawamoy.

Until v9.4.0, this problem never occurred because the render context of the answers file template consisted only of answers and didn't include additional variables such as _copier_conf. One would think that a context hook that accessed _copier_conf.answers_file would have failed because _copier_conf wasn't included in the render context of the answers file template, but no error occurred. That's because the context hook is invoked only when _copier_conf exists. Thus, while a context hook accessing, e.g., _copier_conf.* was working with Copier until v9.4.0 – in the sense that no error occurred –, the behavior wasn't entirely correct, as not all rendering contexts were updated because _copier_conf wasn't present in all of them until recent Copier versions.

Now, to the best of my knowledge, all rendering contexts include _copier_conf, so the context hook is invoked for all rendering contexts including those prior to answering any questions. I believe that this is correct behavior and previous behavior was unintended. (WDYT, @pawamoy?) This means unconditional access of answers variables or other variables that require answers variables for rendering does not work. For example, github.com/scientific-python/cookie:helpers/extensions.py#L18 raises a KeyError when accessing the url variable, as all rendering contexts prior to answering the url question don't contain it (@henryiii). In this specific example, I think the correct behavior is to inject the __ci variable into the context only when the url variable exists. This is consistent with using a computed value in copier.yaml:

url:
  type: str
  help: The url to your GitHub or GitLab repository
  # [[[end]]]
  default: "https://github.com/{{ org }}/{{ project_name }}"

ci: # Actually `__ci`, but we can't prefix questions (or computed values) with underscores
  type: str
  default: "{% if 'github.com' in url %}github{% else %}gitlab{% endif %}"
  when: false

I'm aware that this may be considered a breaking change. To restore previous behavior where context hooks are invoked only when rendering files/directories/paths, I believe we could add another condition in copier-templates-extensions to check for the value of _copier_phase, such that recent Copier versions that expose this variable don't cause a context hook invocation in other phases. However, I suggest to raise a deprecation warning and recommend to template authors to implement context updates with the different rendering phases in mind.

WDYT, @pawamoy @henryiii?

Fixes #2113.

@sisp sisp force-pushed the fix/context-hook-answers-file-recursion branch from 0a21f53 to 33722d7 Compare April 29, 2025 12:18
Copy link

codecov bot commented Apr 29, 2025

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.98%. Comparing base (be767f6) to head (33722d7).

Files with missing lines Patch % Lines
copier/_types.py 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2114      +/-   ##
==========================================
+ Coverage   97.86%   97.98%   +0.12%     
==========================================
  Files          55       55              
  Lines        5949     5957       +8     
==========================================
+ Hits         5822     5837      +15     
+ Misses        127      120       -7     
Flag Coverage Δ
unittests 97.98% <81.81%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest copier now has recursion error
1 participant