refactor(workflow): add Jinja2 renderer abstraction for template transform#3
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>
There was a problem hiding this comment.
Pull request overview
This PR refactors the template transform node to introduce a Jinja2 template renderer abstraction layer, separating rendering logic from the node implementation for better testability and maintainability.
Changes:
- Introduces
Jinja2TemplateRendererProtocol andCodeExecutorJinja2TemplateRendereradapter class for template rendering - Updates
TemplateTransformNodeto use dependency injection for the template renderer - Modifies
DifyNodeFactoryto initialize and inject template renderer into template transform nodes - Updates test suite to mock the new renderer abstraction and adds 23 comprehensive new test cases
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 28 comments.
| File | Description |
|---|---|
| api/core/workflow/nodes/template_transform/template_renderer.py | New file introducing renderer abstraction with Protocol interface and CodeExecutor adapter implementation |
| api/core/workflow/nodes/template_transform/template_transform_node.py | Refactored to accept injected template renderer and use new error type |
| api/core/workflow/nodes/node_factory.py | Updated to instantiate and inject template renderer into TemplateTransformNode |
| api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py | Updated mocks and added 23 new test cases for comprehensive template rendering scenarios |
Comments suppressed due to low confidence (13)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:66
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=basic_node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:81
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=basic_node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:93
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=basic_node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:112
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:158
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=basic_node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:187
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:210
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=basic_node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:234
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=basic_node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:278
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:324
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:364
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:394
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py:425
- Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
node = TemplateTransformNode(
id="test_node",
config=node_data,
graph_init_params=graph_init_params,
graph=mock_graph,
graph_runtime_state=mock_graph_runtime_state,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| max_string_array_length=dify_config.CODE_MAX_STRING_ARRAY_LENGTH, | ||
| max_object_array_length=dify_config.CODE_MAX_OBJECT_ARRAY_LENGTH, | ||
| ) | ||
| self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer(code_executor) |
There was a problem hiding this comment.
The parameter code_executor is used here instead of self._code_executor. While this may work if code_executor is None (passing None through), it's inconsistent with the pattern used for CodeNode initialization on lines 107-115, where self._code_executor is passed.
For consistency and clarity, this should be self._code_executor to ensure the same code executor instance is used across both CodeNode and TemplateTransformNode.
| self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer(code_executor) | |
| self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer(self._code_executor) |
| graph=mock_graph, | ||
| graph_runtime_state=mock_graph_runtime_state, |
There was a problem hiding this comment.
The test is passing an invalid graph parameter to TemplateTransformNode. The Node base class init (api/core/workflow/nodes/base/node.py:203-209) only accepts id, config, graph_init_params, and graph_runtime_state parameters. The graph parameter doesn't exist in the signature and will cause a TypeError at runtime.
This issue appears throughout all the new test methods. The graph=mock_graph, line should be removed from all TemplateTransformNode instantiations.
| class Jinja2TemplateRenderer(Protocol): | ||
| """Render Jinja2 templates for template transform nodes.""" | ||
|
|
||
| def render_template(self, template: str, variables: Mapping[str, Any]) -> str: | ||
| """Render a Jinja2 template with provided variables.""" | ||
| raise NotImplementedError |
There was a problem hiding this comment.
This PR introduces a Protocol class, which is a departure from the codebase convention. Throughout the codebase, abstract interfaces are defined using ABC (Abstract Base Class) from the abc module (e.g., api/core/helper/code_executor/template_transformer.py, api/core/helper/code_executor/code_node_provider.py, and many others).
For consistency with the established patterns, consider using ABC instead of Protocol. If Protocol is intentionally chosen for structural subtyping (duck typing), this should be documented, but the current implementation has CodeExecutorJinja2TemplateRenderer explicitly inheriting from the Protocol, which defeats the purpose of structural typing.
|
|
||
| rendered = result.get("result") | ||
| if rendered is not None and not isinstance(rendered, str): | ||
| raise TemplateRenderError("Template render result must be a string.") |
There was a problem hiding this comment.
The error message "Template render result must be a string." could be more helpful by including the actual type that was received. This would make debugging easier when this error occurs.
Consider changing to: f"Template render result must be a string, got {type(rendered).__name__}"
| raise TemplateRenderError("Template render result must be a string.") | |
| raise TemplateRenderError( | |
| f"Template render result must be a string, got {type(rendered).__name__}" | |
| ) |
| raise TemplateRenderError(str(exc)) from exc | ||
|
|
||
| rendered = result.get("result") | ||
| if rendered is not None and not isinstance(rendered, str): |
There was a problem hiding this comment.
The render_template method can return None when result.get("result") returns None, but the return type annotation indicates it should always return a str. This will cause type checking issues and potential runtime errors when the caller expects a string but receives None.
The condition on line 38 checks if rendered is not None and not a string, but doesn't handle the case where rendered is None. The method should either raise an error when the result is None or handle it explicitly.
| if rendered is not None and not isinstance(rendered, str): | |
| if rendered is None: | |
| raise TemplateRenderError("Template render result is missing.") | |
| if not isinstance(rendered, str): |
| node = TemplateTransformNode( | ||
| id="test_node", | ||
| config=node_data, | ||
| graph_init_params=graph_init_params, | ||
| graph=mock_graph, | ||
| graph_runtime_state=mock_graph_runtime_state, | ||
| ) |
There was a problem hiding this comment.
Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
| node = TemplateTransformNode( | ||
| id="test_node", | ||
| config=node_data, | ||
| graph_init_params=graph_init_params, | ||
| graph=mock_graph, | ||
| graph_runtime_state=mock_graph_runtime_state, | ||
| ) |
There was a problem hiding this comment.
Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
| node = TemplateTransformNode( | ||
| id="test_node", | ||
| config=node_data, | ||
| graph_init_params=graph_init_params, | ||
| graph=mock_graph, | ||
| graph_runtime_state=mock_graph_runtime_state, | ||
| ) |
There was a problem hiding this comment.
Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
| node = TemplateTransformNode( | ||
| id="test_node", | ||
| config=node_data, | ||
| graph_init_params=graph_init_params, | ||
| graph=mock_graph, | ||
| graph_runtime_state=mock_graph_runtime_state, | ||
| ) |
There was a problem hiding this comment.
Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
| node = TemplateTransformNode( | ||
| id="test_node", | ||
| config=node_data, | ||
| graph_init_params=graph_init_params, | ||
| graph=mock_graph, | ||
| graph_runtime_state=mock_graph_runtime_state, | ||
| ) |
There was a problem hiding this comment.
Keyword argument 'graph' is not a supported parameter name of TemplateTransformNode.init.
Benchmark PR from agentic-review-benchmarks#3