-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(workflow): add Jinja2 renderer abstraction for template transform #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devin_pr_code_review_bench_100_devin2_base_refactorworkflow_add_jinja2_renderer_abstraction_for_template_transform_pr433
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 render_template can return None instead of str, causing TypeError in caller
The render_template method in CodeExecutorJinja2TemplateRenderer can return None when the result dict doesn't contain a "result" key or when the value is None, despite declaring a return type of str.
Click to expand
Bug mechanism
In template_renderer.py:37-40:
rendered = result.get("result")
if rendered is not None and not isinstance(rendered, str):
raise TemplateRenderError("Template render result must be a string.")
return renderedThe condition rendered is not None and not isinstance(rendered, str) only raises an error when rendered is not None and not a string. When rendered is None, the condition evaluates to False, so no error is raised and None is returned.
Impact
The caller in template_transform_node.py:69 does:
if len(rendered) > MAX_TEMPLATE_TRANSFORM_OUTPUT_LENGTH:If rendered is None, this will raise TypeError: object of type 'NoneType' has no len().
Actual vs Expected
- Actual: Returns
Nonewhen result doesn't contain "result" key or value isNone - Expected: Should raise
TemplateRenderErrorwhen the result is not a valid string
Recommendation: Change the condition to also handle the None case:
rendered = result.get("result")
if not isinstance(rendered, str):
raise TemplateRenderError("Template render result must be a string.")
return renderedWas this helpful? React with 👍 or 👎 to provide feedback.
Benchmark PR from qodo-benchmark#433