fix(loader): preserve non-env {{...}} placeholders in ConfigLoader (#81) - #132
owen-pengtao wants to merge 1 commit into
Conversation
…ontextforge-org#81) ConfigLoader.load_config renders the whole plugin config.yaml through a single jinja pass with the default (empty) Undefined, so any {{...}} that is not an env reference is silently blanked to "". A plugin that stores a runtime template in its config (e.g. WebhookNotification's default_template) is therefore emptied at load time and later emits an all-empty, invalid body. Render with jinja2.DebugUndefined so undefined names (everything except the env.* values passed to render()) survive verbatim as {{ name }} instead of being blanked, while env.* references are still substituted. Adds regression tests covering env substitution, non-env placeholder preservation, and use_jinja=False. Fixes contextforge-org#81. Signed-off-by: Owen Peng <tao.peng@tibco.com>
|
can anyone review this pr? |
@owen-pengtao I was busy with something else, but marking myself to review it this week. |
|
@araujof do you able to review this pr week? |
araujof
left a comment
There was a problem hiding this comment.
@owen-pengtao Thanks for this PR. DebugUndefined does not preserve arbitrary non-env Jinja.** The entire YAML file is still evaluated as a Jinja template. For example, {{ event.name }} raises UndefinedError, filters can transform placeholders, and conditional blocks can disappear. An unset {{ env.MISSING }} also becomes Jinja diagnostic text rather than remaining {{ env.MISSING }} as described in the PR. Use genuinely env-only interpolation and add regression coverage for nested expressions, filters, conditionals, and missing environment variables.
Below is a suggested alternative that addresses this problem. Would you like to try it and update your PR?
Suggested implementation
Replace only the explicitly supported {{ env.NAME }} syntax instead of rendering the entire configuration as Jinja. For example:
import re
_ENV_REFERENCE = re.compile(r"{{\s*env\.([A-Za-z_][A-Za-z0-9_]*)\s*}}")
def _interpolate_env(template: str) -> str:
def replace(match: re.Match[str]) -> str:
name = match.group(1)
# Preserve an unset reference. This policy could instead raise a clear
# configuration error, but it should be explicit and tested.
return os.environ.get(name, match.group(0))
return _ENV_REFERENCE.sub(replace, template)Then use _interpolate_env(template) in the use_jinja branch. Given:
endpoint: "{{ env.WEBHOOK_URL }}"
default_template: "{{ event.name | upper }}"only WEBHOOK_URL would be substituted; the runtime template would remain byte-for-byte unchanged. If | default(...) must remain supported, add it as an explicitly parsed part of this restricted syntax rather than evaluating the whole YAML document with Jinja.
Add tests confirming that nested expressions, filters, and {% ... %} blocks remain unchanged, plus a test defining the behavior of an unset environment variable.
Summary
Fixes #81 —
ConfigLoader.load_configblanks non-env{{...}}placeholders in the plugin config.load_configrenders the entireconfig.yamlthrough one jinja pass with the default (empty)Undefined:Because the render only defines
env, any{{...}}that is not anenvreference is silently rendered to""at load time — including placeholders a plugin legitimately stores in its config to render itself later, at runtime. The reported symptom:WebhookNotification'sdefault_templateis blanked to
{ "event": "", "timestamp": "", "violation": , ... }at load, so the plugin posts an all-empty, invalid-JSON body.Fix
Render with
jinja2.DebugUndefined(issue #81's suggested option 1) so undefined names — everything except theenv.*values passed torender()— survive verbatim as{{ name }}instead of being blanked, whileenv.*references are still substituted.Behavior note
A bare
{{ env.MISSING }}for an unset env var now renders literally as{{ env.MISSING }}instead of"". Configs that want an empty fallback should use thedefaultfilter ({{ env.X | default('') }}), which continues to work. This is the documented trade-off of the DebugUndefined approach.Tests
Adds
tests/unit/cpex/framework/loader/test_config_loader.py:env.*reference is still substituted;env{{...}}runtime template placeholder is preserved (the [BUG]: ConfigLoader.load_config Jinja pass blanks non-env {{...}} placeholders in plugin config #81 regression);use_jinja=Falseleaves everything literal.Verified against the real
0.1.xConfigLoader: current behavior blanks the placeholders; with this change the three tests pass and the WebhookNotification-styledefault_templatesurvives load.Signed-off-by: Owen Peng tao.peng@tibco.com