refactor(workflow): add Jinja2 renderer abstraction for template transform#9
Conversation
…de and threaded it through DifyNodeFactory so TemplateTransform nodes receive the dependency by default, keeping behavior unchanged unless an override is provided. Changes are in `api/core/workflow/nodes/template_transform/template_transform_node.py` and `api/core/workflow/nodes/node_factory.py`. **Commits** - chore(workflow): identify TemplateTransform dependency on CodeExecutor - feat(workflow): add CodeExecutor constructor injection to TemplateTransformNode (defaulting to current behavior) - feat(workflow): inject CodeExecutor from DifyNodeFactory when creating TemplateTransform nodes **Tests** - Not run (not requested) Next step: run `make lint` and `make type-check` if you want to validate the backend checks.
…Transform to use it, keeping CodeExecutor as the default adapter while preserving current behavior. Updates are in `api/core/workflow/nodes/template_transform/template_renderer.py`, `api/core/workflow/nodes/template_transform/template_transform_node.py`, `api/core/workflow/nodes/node_factory.py`, and `api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py`. Commit-style summary: - feat(template-transform): add Jinja2 template renderer abstraction with CodeExecutor adapter - refactor(template-transform): use renderer in node/factory and update unit test patches Tests not run (not requested).
…ode creation to return TemplateTransformNode directly for template-transform nodes in `api/core/workflow/nodes/node_factory.py`. Commit-style summary: - refactor(template-transform): derive TemplateRenderError from ValueError - refactor(node-factory): instantiate TemplateTransformNode directly with injected renderer Tests not run (not requested).
…ts/core/workflow/nodes/template_transform/template_transform_node_spec.py`) chore(type-check): ran `make type-check` (basedpyright clean, 0 errors) No errors reported.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
WalkthroughThis pull request introduces a new Jinja2 template rendering abstraction layer for workflow nodes. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@api/core/workflow/nodes/template_transform/template_renderer.py`:
- Around line 29-40: render_template currently allows a None result which
callers expect to be a str; update render_template (and the handling of result
from _code_executor.execute_workflow_code_template with
language=CodeLanguage.JINJA2) to fail fast: after obtaining result = ... and
rendered = result.get("result"), raise TemplateRenderError if rendered is None
(in addition to the existing non-str check) so the function always returns a
str; use TemplateRenderError with a clear message like "Template render result
must be a non-empty string" (or "Template render result is None") to enforce the
contract.
In `@api/core/workflow/nodes/template_transform/template_transform_node.py`:
- Around line 26-34: The tests still pass a removed constructor parameter;
remove the obsolete graph argument (e.g., graph=mock_graph) from every
TemplateTransformNode(...) instantiation in the unit test file that instantiates
TemplateTransformNode so calls match the current signature (id, config,
graph_init_params, graph_runtime_state, and optional template_renderer). Ensure
none of the ~10+ calls include the graph parameter and that any mock_graph
variable is no longer passed into TemplateTransformNode constructors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/core/workflow/nodes/node_factory.pyapi/core/workflow/nodes/template_transform/template_renderer.pyapi/core/workflow/nodes/template_transform/template_transform_node.pyapi/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py
🧰 Additional context used
🧬 Code graph analysis (4)
api/core/workflow/nodes/template_transform/template_renderer.py (2)
api/core/helper/code_executor/code_executor.py (3)
CodeExecutionError(29-30)CodeLanguage(43-46)execute_workflow_code_template(142-156)api/models/tools.py (1)
variables(456-457)
api/core/workflow/nodes/template_transform/template_transform_node.py (1)
api/core/workflow/nodes/template_transform/template_renderer.py (5)
CodeExecutorJinja2TemplateRenderer(21-40)Jinja2TemplateRenderer(13-18)TemplateRenderError(9-10)render_template(16-18)render_template(29-40)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py (1)
api/core/workflow/nodes/template_transform/template_renderer.py (1)
TemplateRenderError(9-10)
api/core/workflow/nodes/node_factory.py (4)
api/core/workflow/nodes/template_transform/template_renderer.py (2)
CodeExecutorJinja2TemplateRenderer(21-40)Jinja2TemplateRenderer(13-18)api/core/workflow/nodes/template_transform/template_transform_node.py (1)
TemplateTransformNode(22-93)api/core/workflow/enums.py (1)
NodeType(36-85)api/core/workflow/nodes/base/node.py (1)
graph_init_params(244-245)
🔇 Additional comments (4)
api/core/workflow/nodes/template_transform/template_transform_node.py (1)
68-81: Renderer delegation and output wiring look consistentThe switch to
render_templateand returning the rendered string under"output"aligns cleanly with the new abstraction.api/core/workflow/nodes/node_factory.py (1)
37-64: Template renderer injection is cleanly threadedFactory initialization and node creation paths consistently pass the renderer through.
Also applies to: 117-124
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py (2)
9-10: TemplateRenderError handling matches the new renderer pathTests now raise and assert the renderer-specific error consistently.
Also applies to: 194-215
130-151: Mocked string outputs now match the renderer contractReturning plain strings from
render_templatein tests aligns with the updated render flow.Also applies to: 167-180, 217-227, 241-271, 305-317, 332-357, 371-387, 402-417
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def render_template(self, template: str, variables: Mapping[str, Any]) -> str: | ||
| try: | ||
| result = self._code_executor.execute_workflow_code_template( | ||
| language=CodeLanguage.JINJA2, code=template, inputs=variables | ||
| ) | ||
| except CodeExecutionError as exc: | ||
| raise TemplateRenderError(str(exc)) from exc | ||
|
|
||
| rendered = result.get("result") | ||
| if rendered is not None and not isinstance(rendered, str): | ||
| raise TemplateRenderError("Template render result must be a string.") | ||
| return rendered |
There was a problem hiding this comment.
Enforce non-None render results to avoid downstream TypeError
render_template can return None, but callers assume str and call len(). Fail fast on None to keep the contract strict and avoid crashes.
🛠️ Proposed fix
- rendered = result.get("result")
- if rendered is not None and not isinstance(rendered, str):
- raise TemplateRenderError("Template render result must be a string.")
- return rendered
+ rendered = result.get("result")
+ if not isinstance(rendered, str):
+ raise TemplateRenderError("Template render result must be a string.")
+ return rendered📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def render_template(self, template: str, variables: Mapping[str, Any]) -> str: | |
| try: | |
| result = self._code_executor.execute_workflow_code_template( | |
| language=CodeLanguage.JINJA2, code=template, inputs=variables | |
| ) | |
| except CodeExecutionError as exc: | |
| raise TemplateRenderError(str(exc)) from exc | |
| rendered = result.get("result") | |
| if rendered is not None and not isinstance(rendered, str): | |
| raise TemplateRenderError("Template render result must be a string.") | |
| return rendered | |
| def render_template(self, template: str, variables: Mapping[str, Any]) -> str: | |
| try: | |
| result = self._code_executor.execute_workflow_code_template( | |
| language=CodeLanguage.JINJA2, code=template, inputs=variables | |
| ) | |
| except CodeExecutionError as exc: | |
| raise TemplateRenderError(str(exc)) from exc | |
| rendered = result.get("result") | |
| if not isinstance(rendered, str): | |
| raise TemplateRenderError("Template render result must be a string.") | |
| return rendered |
🤖 Prompt for AI Agents
In `@api/core/workflow/nodes/template_transform/template_renderer.py` around lines
29 - 40, render_template currently allows a None result which callers expect to
be a str; update render_template (and the handling of result from
_code_executor.execute_workflow_code_template with language=CodeLanguage.JINJA2)
to fail fast: after obtaining result = ... and rendered = result.get("result"),
raise TemplateRenderError if rendered is None (in addition to the existing
non-str check) so the function always returns a str; use TemplateRenderError
with a clear message like "Template render result must be a non-empty string"
(or "Template render result is None") to enforce the contract.
| def __init__( | ||
| self, | ||
| id: str, | ||
| config: Mapping[str, Any], | ||
| graph_init_params: "GraphInitParams", | ||
| graph_runtime_state: "GraphRuntimeState", | ||
| *, | ||
| template_renderer: Jinja2TemplateRenderer | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate the file and check its current signature
rg -n "class TemplateTransformNode" --type=pyRepository: qodo-benchmark/dify-combined-coderabbit
Length of output: 321
🏁 Script executed:
#!/bin/bash
# Find all instantiations of TemplateTransformNode
rg -n --type=py "TemplateTransformNode\s*\(" | head -30Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 2955
🏁 Script executed:
#!/bin/bash
# Check the test file mentioned
rg -n --type=py "TemplateTransformNode" api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.pyRepository: qodo-benchmark/dify-combined-coderabbit
Length of output: 1112
🏁 Script executed:
#!/bin/bash
# Check the actual constructor calls in the test file with context
rg -n -A5 "node = TemplateTransformNode" api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py | head -50Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 1956
🏁 Script executed:
#!/bin/bash
# Check node_factory.py instantiation
rg -n -B2 -A10 "return TemplateTransformNode" api/core/workflow/nodes/node_factory.pyRepository: qodo-benchmark/dify-combined-coderabbit
Length of output: 571
🏁 Script executed:
#!/antml/bash
# Check the current __init__ signature
cat api/core/workflow/nodes/template_transform/template_transform_node.py | head -40Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 1443
🏁 Script executed:
#!/bin/bash
# Check if any other callers in test files pass graph parameter
rg -n "TemplateTransformNode\(" api/tests/integration_tests/workflow/nodes/test_template_transform.py -A5Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Check the mock nodes in test files
rg -n "MockTemplateTransformNode\(" api/tests/unit_tests/core/workflow/graph_engine/test_mock_nodes_template_code.py -A8 | head -40Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 1370
🏁 Script executed:
#!/bin/bash
# Let's check if there are any other calls in the codebase passing graph to TemplateTransformNode
rg -n "TemplateTransformNode\(" --type=py -A3 | grep -B3 "graph=" | head -60Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 65
Update template_transform_node_spec.py — constructor no longer accepts graph parameter
The constructor signature has removed the graph parameter, but the test file api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py still passes graph=mock_graph in all 10+ instantiation calls (lines 60, 75, 87, 106, 152, 181, 204, 228, 272, 318, 358, 388, 419). Remove the graph argument from all TemplateTransformNode instantiations in this test file to match the updated constructor signature.
🤖 Prompt for AI Agents
In `@api/core/workflow/nodes/template_transform/template_transform_node.py` around
lines 26 - 34, The tests still pass a removed constructor parameter; remove the
obsolete graph argument (e.g., graph=mock_graph) from every
TemplateTransformNode(...) instantiation in the unit test file that instantiates
TemplateTransformNode so calls match the current signature (id, config,
graph_init_params, graph_runtime_state, and optional template_renderer). Ensure
none of the ~10+ calls include the graph parameter and that any mock_graph
variable is no longer passed into TemplateTransformNode constructors.
2 participants
Benchmark PR from qodo-benchmark#183
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.