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

fix: Escaping Jinja logical statements from assets #205

Merged
merged 10 commits into from
May 22, 2023
39 changes: 39 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,45 @@ The function can then be called from any template the following way:
params:
...

Disabling Jinja Templating
~~~~~~~~~~~~~~~~~~~~~~~~~~

Both the CLI and Superset support Jinja templating. To prevent the CLI from loading Superset Jinja syntax, the export operation automatically escapes Jinja syntax from YAML files. As a consequence, this query:

.. code-block:: yaml

sql: 'SELECT action, count(*) as times
FROM logs
{% if filter_values(''action_type'')|length %}
WHERE action is null
{% for action in filter_values(''action_type'') %}
or action = ''{{ action }}''
{% endfor %}
{% endif %}
GROUP BY action'

Becomes this:

.. code-block:: yaml

sql: 'SELECT action, count(*) as times
FROM logs
{{ '{% if' }} filter_values(''action_type'')|length {{ '%}' }}
WHERE action is null
{{ '{% for' }} action in filter_values(''action_type'') {{ '%}' }}
or action = ''{{ '{{' }} action {{ '}}' }}''
{{ '{% endfor %}' }}
{{ '{% endif %}' }}
GROUP BY action'

When performing the import, the CLI would load any templating syntax that isn't escaped, and remove escaping. However, this escaping syntax isn't compatible with UI imports.
To avoid issues when running migrations using both the CLI and the UI, you can use:

- ``--disable-jinja-escaping`` flag with the ``export-assets`` command to disable the escaping (so that exported assets can be imported via the UI)
- ``--disable-jinja-templating`` flag with the ``sync native`` command to disable jinja templating (so that assets exported via the UI can be imported via the CLI)

Note that using these flags would remove the ability to dynamically modify the content through the CLI.

Synchronizing to and from dbt
-----------------------------

Expand Down
59 changes: 48 additions & 11 deletions src/preset_cli/cli/superset/export.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
A command to export Superset resources into a directory.
"""

import re
from collections import defaultdict
from pathlib import Path
from typing import List, Set, Tuple
Expand All @@ -27,6 +28,12 @@
default=False,
help="Overwrite existing resources",
)
@click.option(
"--disable-jinja-escaping",
is_flag=True,
default=False,
help="Disable Jinja template escaping",
)
@click.option(
"--asset-type",
help="Asset type",
Expand Down Expand Up @@ -62,6 +69,7 @@ def export_assets( # pylint: disable=too-many-locals, too-many-arguments
chart_ids: List[str],
dashboard_ids: List[str],
overwrite: bool = False,
disable_jinja_escaping: bool = False,
) -> None:
"""
Export DBs/datasets/charts/dashboards to a directory.
Expand Down Expand Up @@ -89,16 +97,18 @@ def export_assets( # pylint: disable=too-many-locals, too-many-arguments
root,
client,
overwrite,
disable_jinja_escaping,
skip_related=not ids_requested,
)


def export_resource( # pylint: disable=too-many-arguments
def export_resource( # pylint: disable=too-many-arguments, too-many-locals
resource_name: str,
requested_ids: Set[int],
root: Path,
client: SupersetClient,
overwrite: bool,
disable_jinja_escaping: bool,
skip_related: bool = True,
) -> None:
"""
Expand Down Expand Up @@ -131,21 +141,48 @@ def export_resource( # pylint: disable=too-many-arguments
target.parent.mkdir(parents=True, exist_ok=True)

# escape any pre-existing Jinja2 templates
file_contents = file_contents.replace(
"{{",
f"{JINJA2_OPEN_MARKER} '{{{{' {JINJA2_CLOSE_MARKER}",
)
file_contents = file_contents.replace(
"}}",
f"{JINJA2_OPEN_MARKER} '}}}}' {JINJA2_CLOSE_MARKER}",
)
file_contents = file_contents.replace(JINJA2_OPEN_MARKER, "{{")
file_contents = file_contents.replace(JINJA2_CLOSE_MARKER, "}}")
if not disable_jinja_escaping:
file_contents = jinja_escaper(file_contents)

with open(target, "w", encoding="utf-8") as output:
output.write(file_contents)


def jinja_escaper(value: str) -> str:
"""
Escape Jinja macros and logical statements that shouldn't be handled by CLI
"""
logical_statements_patterns = [
r"(\{%-?\s*if)", # {%if || {% if || {%-if || {%- if
r"(\{%-?\s*elif)", # {%elif || {% elif || {%-elif || {%- elif
r"(\{%-?\s*else)", # {%else || {% else || {%-else || {%- else
r"(\{%-?\s*endif)", # {%endif || {% endif || {%-endif || {%- endif
r"(\{%-?\s*for)", # {%for || {% for || {%-for || {%- for
r"(\{%-?\s*endfor)", # {%endfor || {% endfor || {%-endfor || {%- endfor
r"(%})", # %}
r"(-%})", # -%}
]

for syntax in logical_statements_patterns:
replacement = JINJA2_OPEN_MARKER + " '" + r"\1" + "' " + JINJA2_CLOSE_MARKER
value = re.sub(syntax, replacement, value)

# escaping macros
value = value.replace(
"{{",
f"{JINJA2_OPEN_MARKER} '{{{{' {JINJA2_CLOSE_MARKER}",
)
value = value.replace(
"}}",
f"{JINJA2_OPEN_MARKER} '}}}}' {JINJA2_CLOSE_MARKER}",
)
value = value.replace(JINJA2_OPEN_MARKER, "{{")
value = value.replace(JINJA2_CLOSE_MARKER, "}}")
value = re.sub(r"' }} {{ '", " ", value)

return value


@click.command()
@click.argument(
"path",
Expand Down
29 changes: 26 additions & 3 deletions src/preset_cli/cli/superset/sync/native/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ def is_yaml_config(path: Path) -> bool:
)


def load_yaml(path: Path) -> Dict[str, Any]:
"""
Load a YAML file and returns it as a dictionary.
"""
with open(path, encoding="utf-8") as input_:
content = input_.read()

return yaml.load(content, Loader=yaml.SafeLoader)


def render_yaml(path: Path, env: Dict[str, Any]) -> Dict[str, Any]:
"""
Load a YAML file as a template, render it, and deserialize it.
Expand Down Expand Up @@ -108,6 +118,12 @@ def render_yaml(path: Path, env: Dict[str, Any]) -> Dict[str, Any]:
multiple=True,
help="Custom values for templates (eg, country=BR)",
)
@click.option(
"--disable-jinja-templating",
is_flag=True,
default=False,
help="By default, the CLI supports Jinja templating. This flag disables it",
)
@click.option(
"--disallow-edits",
is_flag=True,
Expand All @@ -130,11 +146,12 @@ def render_yaml(path: Path, env: Dict[str, Any]) -> Dict[str, Any]:
help="Split imports into individual assets",
)
@click.pass_context
def native( # pylint: disable=too-many-locals, too-many-arguments
def native( # pylint: disable=too-many-locals, too-many-arguments, too-many-branches
ctx: click.core.Context,
directory: str,
option: Tuple[str, ...],
overwrite: bool = False,
disable_jinja_templating: bool = False,
disallow_edits: bool = True, # pylint: disable=unused-argument
external_url_prefix: str = "",
load_env: bool = False,
Expand Down Expand Up @@ -171,11 +188,17 @@ def native( # pylint: disable=too-many-locals, too-many-arguments
if path_name.is_dir() and not path_name.stem.startswith("."):
queue.extend(path_name.glob("*"))
elif is_yaml_config(relative_path):
config = render_yaml(path_name, env)
if disable_jinja_templating:
config = load_yaml(path_name)
else:
config = render_yaml(path_name, env)
Copy link
Member

Choose a reason for hiding this comment

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

Small nit:

config = load_yaml(path_name) if disable_jinja_templating else render_yaml(path_name, env)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! changed as suggested


overrides_path = path_name.with_suffix(".overrides" + path_name.suffix)
if overrides_path.exists():
overrides = render_yaml(overrides_path, env)
if disable_jinja_templating:
overrides = load_yaml(overrides_path)
else:
overrides = render_yaml(overrides_path, env)
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this one as well.

dict_merge(config, overrides)

config["is_managed_externally"] = disallow_edits
Expand Down
Loading