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

Soften error to warning in template processor when fails to read file #1924

Merged
merged 5 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -58,38 +58,45 @@ def expand_templates_in_file(
if src.is_dir():
return

with self.edit_file(dest) as file:
if not has_client_side_templates(file.contents) and not (
_is_sql_file(dest) and has_sql_templates(file.contents)
):
return

src_file_name = src.relative_to(self._bundle_ctx.project_root)
cc.step(f"Expanding templates in {src_file_name}")
with cc.indented():
try:
jinja_env = (
choose_sql_jinja_env_based_on_template_syntax(
file.contents, reference_name=src_file_name
src_file_name = src.relative_to(self._bundle_ctx.project_root)

try:
with self.edit_file(dest) as file:
if not has_client_side_templates(file.contents) and not (
_is_sql_file(dest) and has_sql_templates(file.contents)
):
return
cc.step(f"Expanding templates in {src_file_name}")
with cc.indented():
try:
jinja_env = (
choose_sql_jinja_env_based_on_template_syntax(
file.contents, reference_name=src_file_name
)
if _is_sql_file(dest)
else get_client_side_jinja_env()
)
if _is_sql_file(dest)
else get_client_side_jinja_env()
)
expanded_template = jinja_env.from_string(file.contents).render(
template_context or get_cli_context().template_context
)

# For now, we are printing the source file path in the error message
# instead of the destination file path to make it easier for the user
# to identify the file that has the error, and edit the correct file.
except jinja2.TemplateSyntaxError as e:
raise InvalidTemplateInFileError(src_file_name, e, e.lineno) from e

except jinja2.UndefinedError as e:
raise InvalidTemplateInFileError(src_file_name, e) from e

if expanded_template != file.contents:
file.edited_contents = expanded_template
expanded_template = jinja_env.from_string(file.contents).render(
template_context or get_cli_context().template_context
)

# For now, we are printing the source file path in the error message
# instead of the destination file path to make it easier for the user
# to identify the file that has the error, and edit the correct file.
except jinja2.TemplateSyntaxError as e:
raise InvalidTemplateInFileError(
src_file_name, e, e.lineno
) from e

except jinja2.UndefinedError as e:
raise InvalidTemplateInFileError(src_file_name, e) from e

if expanded_template != file.contents:
file.edited_contents = expanded_template
except UnicodeDecodeError as err:
Copy link
Contributor

Choose a reason for hiding this comment

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

why would we get this error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it makes sense for binary files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So that's where those errors are coming from 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sfc-gh-mchok you saw these on the dashboard?! Yeah our template processor was dying every time there was a file it couldn't read :D

cc.warning(
f"Could not read file {src_file_name}, error: {err.reason}. Skipping this file."
)

@span("templates_processor")
def process(
Expand Down
26 changes: 25 additions & 1 deletion tests/nativeapp/codegen/templating/test_templates_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@
from snowflake.cli.api.exceptions import InvalidTemplate
from snowflake.cli.api.project.schemas.v1.native_app.path_mapping import PathMapping

from tests.nativeapp.utils import CLI_GLOBAL_TEMPLATE_CONTEXT
from tests.nativeapp.utils import (
CLI_GLOBAL_TEMPLATE_CONTEXT,
TEMPLATE_PROCESSOR,
)


@dataclass
Expand Down Expand Up @@ -213,3 +216,24 @@ def test_file_with_undefined_variable():
assert "does not contain a valid template" in str(e.value)
assert bundle_result.output_files[0].is_symlink()
assert bundle_result.output_files[0].read_text() == file_contents[0]


@mock.patch(CLI_GLOBAL_TEMPLATE_CONTEXT, {})
@mock.patch(f"{TEMPLATE_PROCESSOR}.cc.warning")
def test_expand_templates_in_file_unicode_decode_error(mock_cc_warning):
file_name = ["test_file.txt"]
file_contents = ["This is a test file"]
with TemporaryDirectory() as tmp_dir:
bundle_result = bundle_files(tmp_dir, file_name, file_contents)
templates_processor = TemplatesProcessor(bundle_ctx=bundle_result.bundle_ctx)
with mock.patch(
f"{TEMPLATE_PROCESSOR}.TemplatesProcessor.edit_file",
side_effect=UnicodeDecodeError("utf-8", b"", 0, 1, "invalid start byte"),
):
src_path = Path(
bundle_result.bundle_ctx.project_root / "src" / file_name[0]
).relative_to(bundle_result.bundle_ctx.project_root)
templates_processor.process(bundle_result.artifact_to_process, None)
mock_cc_warning.assert_called_once_with(
f"Could not read file {src_path}, error: invalid start byte. Skipping this file."
)
4 changes: 4 additions & 0 deletions tests/nativeapp/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@
f"{APP_PACKAGE_ENTITY}.verify_project_distribution"
)

CODE_GEN = "snowflake.cli._plugins.nativeapp.codegen"
TEMPLATE_PROCESSOR = f"{CODE_GEN}.templates.templates_processor"
ARTIFACT_PROCESSOR = f"{CODE_GEN}.artifact_processor"

SQL_EXECUTOR_EXECUTE = f"{API_MODULE}.sql_execution.BaseSqlExecutor.execute_query"
SQL_EXECUTOR_EXECUTE_QUERIES = (
f"{API_MODULE}.sql_execution.BaseSqlExecutor.execute_queries"
Expand Down
Loading