Skip to content
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

Allow list and dict item to be rendered as taget renderer instead of showing as JSON #14024

Merged
merged 1 commit into from
Feb 4, 2021

Conversation

VBhojawala
Copy link
Contributor

@VBhojawala VBhojawala commented Feb 2, 2021

closes: #13988

In #11061 i believe @turbaszek merged code for better rendering dictionary, but code

if isinstance(content, (dict, list)):
content = json.dumps(content, sort_keys=True, indent=4)
html_dict[template_field] = renderers[renderer](content)

also dumps list as json which is passed to render() function as string containing json which is causing this issue.

def render(obj, lexer):
"""Render a given Python object with a given Pygments lexer"""
out = ""
if isinstance(obj, str):
out = Markup(pygment_html_render(obj, lexer))
elif isinstance(obj, (tuple, list)):
for i, text_to_render in enumerate(obj):
out += Markup("<div>List item #{}</div>").format(i) # noqa
out += Markup("<div>" + pygment_html_render(text_to_render, lexer) + "</div>")
elif isinstance(obj, dict):

If we remove list from

if isinstance(content, (dict, list)):

it will pass list to render function and render each list item as target render.
In this case as SQL

rendertmp

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Feb 2, 2021
@VBhojawala VBhojawala changed the title Removed list from rendering as JSON Allow list item to be rendered as taget renderer instead of showing JSON Feb 2, 2021
@@ -898,7 +898,7 @@ def rendered_templates(self):
content = getattr(task, template_field)
renderer = task.template_fields_renderers.get(template_field, template_field)
if renderer in renderers:
if isinstance(content, (dict, list)):
if isinstance(content, dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isinstance(content, dict):
if renderer == "json" and isinstance(content, (dict, list)):

This will probably be better approach as will solve your problem and preserve the current behavior for fields marked as json. WDYT?

Copy link

@YangMuye YangMuye Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a work-around, we can handle it here. But I actually believe the logic should be handled by the renderer itself.
Just make JSON renderer the fallback renderer when no renderer is specified.

Copy link
Contributor Author

@VBhojawala VBhojawala Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree , If we are also rendering the each dictionary item instead of rending as json.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if somebody using YAML wants this feature too. I think only some generic serialization format is capable of rendering types other than strings.

Copy link
Contributor Author

@VBhojawala VBhojawala Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO can we extract renderer code for JSON from the view and my be map a custom render function for json here ?

'json': lambda x: render(x, lexers.JsonLexer),

Copy link

@YangMuye YangMuye Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this? Actually I am even thinking whether a field accepts a list or not is also decided by the Operator...

def get_attr_renderer():
    """Return Dictionary containing different Pygments Lexers for Rendering & Highlighting"""
    return {
-        'json': lambda x: render(x, lexers.JsonLexer),
+        'json': lambda x: render(x if isinstance(content, (dict, list)) else json.dumps(x), lexers.JsonLexer),
-        'yaml': lambda x: render(x, lexers.YamlLexer),
+        'yaml': lambda x: render(x if isinstance(content, (dict, list)) else json.dumps(x), lexers.YamlLexer),

        'py': lambda x: render(get_python_source(x), lexers.PythonLexer),
        'python_callable': lambda x: render(get_python_source(x), lexers.PythonLexer),

    }

python fields are already doing similar transformation.

@VBhojawala
Copy link
Contributor Author

Hi @turbaszek ,

I have separated json renderer filtering code from view and created a "json_render()" function and mapped in "wrapped_markdown" function.

@VBhojawala VBhojawala changed the title Allow list item to be rendered as taget renderer instead of showing JSON Allow list and dict item to be rendered as taget renderer instead of showing as JSON Feb 3, 2021
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List and Dict template fields are rendered as JSON.
3 participants