refactor(workflow): add Jinja2 renderer abstraction for template transform - #52
refactor(workflow): add Jinja2 renderer abstraction for template transform#52tomerqodo wants to merge 6 commits into
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 workflow node to introduce a Jinja2 renderer abstraction layer. The refactor extracts template rendering logic into a dedicated module with a Protocol-based interface, allowing for better testability and flexibility in rendering implementations.
Changes:
- Introduces
Jinja2TemplateRendererProtocol andCodeExecutorJinja2TemplateRendererimplementation for template rendering abstraction - Updates
TemplateTransformNodeto accept an injectable renderer dependency instead of directly callingCodeExecutor - Modifies
DifyNodeFactoryto instantiate and inject the template renderer intoTemplateTransformNodeinstances - Adds 719 lines of comprehensive test coverage for various Jinja2 template scenarios (filters, loops, conditionals, edge 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 module defining the Jinja2TemplateRenderer Protocol, TemplateRenderError exception, and CodeExecutorJinja2TemplateRenderer adapter implementation |
| api/core/workflow/nodes/template_transform/template_transform_node.py | Adds custom __init__ for dependency injection, refactors _run method to use injected renderer, updates exception handling from CodeExecutionError to TemplateRenderError |
| api/core/workflow/nodes/node_factory.py | Adds template_renderer parameter to factory constructor, creates renderer instance with code executor, injects renderer when creating TEMPLATE_TRANSFORM nodes |
| api/tests/unit_tests/core/workflow/nodes/template_transform/template_transform_node_spec.py | Updates mocks to patch new renderer class, changes mock return values from dict to string, adds 32 new test methods covering boolean values, nested dicts, filters, loops, special characters, and edge cases |
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,
)
|
|
||
| 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.
The Protocol class should not raise NotImplementedError in its method. In Python, Protocol classes are used for structural subtyping (duck typing), and their methods should use ... (Ellipsis) as the body or have no implementation at all. The raise statement here is incorrect for a Protocol definition.
| raise NotImplementedError | |
| ... |
| 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 should handle the case where result.get("result") returns None. Currently, if rendered is None, it will be returned directly, but this contradicts the return type annotation which specifies 'str', not 'str | None'. Either add a proper None check and raise an error, or update the return type annotation to include None.
| 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): |
| class CodeExecutorJinja2TemplateRenderer(Jinja2TemplateRenderer): | ||
| """Adapter that renders Jinja2 templates via CodeExecutor.""" | ||
|
|
||
| _code_executor: type[CodeExecutor] | ||
|
|
||
| def __init__(self, code_executor: type[CodeExecutor] | None = None) -> None: | ||
| self._code_executor = code_executor or CodeExecutor | ||
|
|
||
| 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.
The new template_renderer.py module lacks dedicated unit tests. While the template_transform_node_spec.py tests the integration, there should be separate unit tests for the CodeExecutorJinja2TemplateRenderer class to cover:
- Successful template rendering
- Error conversion from CodeExecutionError to TemplateRenderError
- Handling of None result values
- Type validation for non-string results
- Different code_executor injection scenarios
| def __init__( | ||
| self, | ||
| id: str, | ||
| config: Mapping[str, Any], | ||
| graph_init_params: "GraphInitParams", | ||
| graph_runtime_state: "GraphRuntimeState", | ||
| *, | ||
| template_renderer: Jinja2TemplateRenderer | None = None, | ||
| ) -> None: | ||
| super().__init__( | ||
| id=id, | ||
| config=config, | ||
| graph_init_params=graph_init_params, | ||
| graph_runtime_state=graph_runtime_state, | ||
| ) | ||
| self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer() |
There was a problem hiding this comment.
The new init method signature doesn't include a graph parameter, but existing tests in this file (lines 58-114 and others not shown in the diff) pass graph=mock_graph when creating TemplateTransformNode instances. These tests will fail with a TypeError for unexpected keyword argument 'graph'. Either the init method should accept and ignore the graph parameter for backward compatibility, or all the existing tests need to be updated to remove the graph parameter.
| 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 code_executor parameter is passed to CodeExecutorJinja2TemplateRenderer, but at this point code_executor might still be None (before the default is applied). This should use self._code_executor instead, which has already been resolved to CodeExecutor if code_executor was None (as done on line 49).
| self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer(code_executor) | |
| self._template_renderer = template_renderer or CodeExecutorJinja2TemplateRenderer(self._code_executor) |
| 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 qodo-benchmark#433