refactor(workflow): add Jinja2 renderer abstraction for template transform#29
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>
WalkthroughIntroduces a pluggable template rendering abstraction for template transform nodes. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TemplateTransformNode
participant Jinja2TemplateRenderer
participant CodeExecutor
Client->>TemplateTransformNode: Execute node with template & variables
TemplateTransformNode->>Jinja2TemplateRenderer: render_template(template, variables)
Jinja2TemplateRenderer->>CodeExecutor: execute_workflow_code_template(JINJA2, template, variables)
CodeExecutor-->>Jinja2TemplateRenderer: {result: rendered_string}
Jinja2TemplateRenderer-->>TemplateTransformNode: rendered_string
TemplateTransformNode->>TemplateTransformNode: Validate output length
TemplateTransformNode-->>Client: {output: rendered_string}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 1
🤖 Fix all issues with AI agents
In `@api/core/workflow/nodes/template_transform/template_renderer.py`:
- Around line 29-40: The render_template method (render_template) currently
returns result.get("result") which can be None, contradicting the declared
return type str; change the implementation to treat a missing/None result as an
error: after calling self._code_executor.execute_workflow_code_template
(CodeLanguage.JINJA2), validate that result.get("result") is a non-None string
and if it is None (or not a str) raise TemplateRenderError with a clear message
instead of returning None so callers expecting a str won't receive None; keep
the signature as -> str and ensure TemplateRenderError is raised for
missing/None results.
🧹 Nitpick comments (1)
api/core/workflow/nodes/node_factory.py (1)
63-63: Consider usingself._code_executorfor consistency.Line 63 passes the raw
code_executorparameter toCodeExecutorJinja2TemplateRenderer, while line 49 already resolvedcode_executor or CodeExecutortoself._code_executor. Using the resolved value would be more consistent.♻️ Suggested change
- self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer(code_executor) + self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer(self._code_executor)
📜 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_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/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-92)api/core/workflow/enums.py (1)
NodeType(36-85)api/core/workflow/nodes/base/node.py (1)
graph_init_params(244-245)
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/template_transform/template_renderer.py (1)
api/core/helper/code_executor/code_executor.py (4)
CodeExecutionError(29-30)CodeExecutor(56-156)CodeLanguage(43-46)execute_workflow_code_template(142-156)
🔇 Additional comments (7)
api/core/workflow/nodes/template_transform/template_renderer.py (1)
9-18: LGTM on the abstraction design.The
TemplateRenderErrorexception andJinja2TemplateRendererprotocol provide a clean abstraction for template rendering with proper separation of concerns. This enables easy testing and alternative implementations.api/core/workflow/nodes/template_transform/template_transform_node.py (2)
26-41: Clean dependency injection pattern.The constructor properly accepts an optional
template_rendererwith a sensible default. Using a keyword-only parameter (*,) enforces explicit naming at call sites, improving readability.
68-79: Depends onrender_templatereturning non-None.The code assumes
renderedis a string (callslen(rendered)on line 69 and uses it in outputs). This will work correctly once the return type issue intemplate_renderer.pyis addressed to guarantee a non-None return.api/core/workflow/nodes/node_factory.py (1)
117-124: LGTM on TEMPLATE_TRANSFORM node wiring.The factory correctly handles the new node type with proper dependency injection, matching the pattern established for
CodeNode.api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py (3)
130-165: Correctly updated patch target and return values.The tests properly patch
CodeExecutorJinja2TemplateRenderer.render_templateand return plain strings instead of dictionaries, matching the new abstraction's interface.
197-215: Error handling test correctly usesTemplateRenderError.The test properly validates that
TemplateRenderErrorexceptions are caught and result in aFAILEDstatus with the error message propagated.
433-1151: Comprehensive test coverage for template features.Excellent coverage of edge cases including:
- Boolean values and conditionals
- Nested dictionaries
- Various Jinja2 filters (upper, lower, length, default, escape)
- Multiline templates and whitespace control
- Unicode and special characters
- Numeric edge cases (zero, negative, large numbers, float precision)
- Empty collections
✏️ 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.
Return type mismatch: method can return None but signature declares str.
The method signature declares -> str, but result.get("result") on line 37 can return None when the key is absent. Line 40 then returns this potentially None value.
This causes downstream issues in template_transform_node.py where len(rendered) (line 69) would raise TypeError if rendered is None.
Either:
- Update the return type to
str | Noneand handleNonein callers, or - Raise
TemplateRenderErrorwhen the result is missing/None
🐛 Proposed fix (option 2 - enforce non-None return)
rendered = result.get("result")
if rendered is not None and not isinstance(rendered, str):
raise TemplateRenderError("Template render result must be a string.")
+ if rendered is None:
+ raise TemplateRenderError("Template render returned no result.")
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 rendered is not None and not isinstance(rendered, str): | |
| raise TemplateRenderError("Template render result must be a string.") | |
| if rendered is None: | |
| raise TemplateRenderError("Template render returned no result.") | |
| return rendered |
🤖 Prompt for AI Agents
In `@api/core/workflow/nodes/template_transform/template_renderer.py` around lines
29 - 40, The render_template method (render_template) currently returns
result.get("result") which can be None, contradicting the declared return type
str; change the implementation to treat a missing/None result as an error: after
calling self._code_executor.execute_workflow_code_template
(CodeLanguage.JINJA2), validate that result.get("result") is a non-None string
and if it is None (or not a str) raise TemplateRenderError with a clear message
instead of returning None so callers expecting a str won't receive None; keep
the signature as -> str and ensure TemplateRenderError is raised for
missing/None results.
2 participants
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.