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

fire proper event for inline query error #7960

Merged
merged 9 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20230628-144125.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: Inline query emit proper error message
time: 2023-06-28T14:41:25.699046-07:00
custom:
Author: ChenyuLInx
Issue: "7940"
9 changes: 9 additions & 0 deletions core/dbt/events/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1227,6 +1227,15 @@ message UnsupportedConstraintMaterializationMsg {
UnsupportedConstraintMaterialization data = 2;
}

// I069
message ParseNodeError{
NodeInfo node_info = 1;
string exc = 2;
}
message ParseNodeErrorMsg {
EventInfo info = 1;
ParseNodeError data = 2;
}

// M - Deps generation

Expand Down
8 changes: 8 additions & 0 deletions core/dbt/events/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,14 @@ def message(self) -> str:
return line_wrap_message(warning_tag(msg))


class ParseNodeError(ErrorLevel):
def code(self):
return "I069"

def message(self) -> str:
return "Error while parsing node: " + self.node_info.node_name + "\n" + self.exc


# =======================================================
# M - Deps generation
# =======================================================
Expand Down
5,184 changes: 4,321 additions & 863 deletions core/dbt/events/types_pb2.py

Large diffs are not rendered by default.

37 changes: 27 additions & 10 deletions core/dbt/task/compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@
from dbt.contracts.results import RunStatus, RunResult
from dbt.events.base_types import EventLevel
from dbt.events.functions import fire_event
from dbt.events.types import CompiledNode, Note
from dbt.exceptions import DbtInternalError, DbtRuntimeError
from dbt.events.types import CompiledNode, Note, ParseNodeError
from dbt.exceptions import (
CompilationError,
DbtInternalError,
DbtRuntimeError,
Exception as DbtException,
)
from dbt.graph import ResourceTypeSelector
from dbt.node_types import NodeType
from dbt.parser.manifest import write_manifest, process_node
Expand Down Expand Up @@ -129,14 +134,26 @@ def defer_to_manifest(self, adapter, selected_uids: AbstractSet[str]):

def _runtime_initialize(self):
if getattr(self.args, "inline", None):
block_parser = SqlBlockParser(
project=self.config, manifest=self.manifest, root_project=self.config
)
sql_node = block_parser.parse_remote(self.args.inline, "inline_query")
process_node(self.config, self.manifest, sql_node)
# keep track of the node added to the manifest
self._inline_node_id = sql_node.unique_id

try:
block_parser = SqlBlockParser(
project=self.config, manifest=self.manifest, root_project=self.config
)
sql_node = block_parser.parse_remote(self.args.inline, "inline_query")
process_node(self.config, self.manifest, sql_node)
# keep track of the node added to the manifest
self._inline_node_id = sql_node.unique_id
except CompilationError as exc:
fire_event(
ParseNodeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

I made this request before, but somehow github lost it... "ParseNodeError" is too generic of a name, it sounds like it might apply to any parsing error, but it's only fired for inline nodes. Could we call it something like "ParseInlineNodeError"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't name this to be specific for inline node is the same error can happened for any node.
Given the exception I am catching here, do you think NodeCompilationError would be a better one that's not too generic? Or any other suggestions that doesn't contain inline node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per offline discussion with Gerda, we decided that raising a narrower scoped exception is fine because distinguishing between inline node parsing and project node parsing might be useful.

exc=str(exc.msg),
node_info={
"node_path": "sql/inline_query",
"node_name": "inline_query",
"unique_id": "sqloperation.test.inline_query",
},
)
)
raise DbtException("Error parsing inline query")
super()._runtime_initialize()

def after_run(self, adapter, results):
Expand Down
1 change: 1 addition & 0 deletions dev-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,4 @@ types-pytz
types-requests
types-setuptools
wheel
mocker
23 changes: 19 additions & 4 deletions tests/functional/compile/test_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import pytest

from dbt.cli.main import dbtRunner
from dbt.exceptions import DbtRuntimeError, TargetNotFoundError
from dbt.exceptions import DbtRuntimeError, Exception as DbtException
from dbt.tests.util import run_dbt, run_dbt_and_capture
from tests.functional.compile.fixtures import (
first_model_sql,
Expand Down Expand Up @@ -155,9 +155,7 @@ def test_select_pass_empty(self, project):
assert "Compiled node 'second_model' is:" in log_output

def test_inline_fail(self, project):
with pytest.raises(
TargetNotFoundError, match="depends on a node named 'third_model' which was not found"
):
with pytest.raises(DbtException, match="Error parsing inline query"):
run_dbt(["compile", "--inline", "select * from {{ ref('third_model') }}"])

def test_multiline_jinja(self, project):
Expand Down Expand Up @@ -187,12 +185,29 @@ def test_compile_inline_not_add_node(self, project):
manifest = parse_result.result
assert len(manifest.nodes) == 4
dbt = dbtRunner(manifest=manifest)
dbt = dbtRunner()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to remove this

dbt.invoke(
["compile", "--inline", "select * from {{ ref('second_model') }}"],
populate_cache=False,
)
assert len(manifest.nodes) == 4

def test_compile_inline_syntax_error(self, project, mocker):
patched_fire_event = mocker.patch("dbt.task.compile.fire_event")
dbt = dbtRunner()
res = dbt.invoke(["compile", "--inline", "select * from {{ ref(1) }}"])
# Event for parsing error
patched_fire_event.assert_called_once()
Copy link
Contributor Author

@ChenyuLInx ChenyuLInx Jun 28, 2023

Choose a reason for hiding this comment

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

I didn't really find a better way to test things here other than making sure we call fire event and re-raised a proper error.
Also running with run_and_capture doesn't work because of the final exception being raise. I also think we should reduce the need to depending on testing the logs in the long term.

Happy to take suggestions here.

assert str(res.exception) == "Error parsing inline query"

def test_compile_inline_ref_node_not_exist(self, project, mocker):
patched_fire_event = mocker.patch("dbt.task.compile.fire_event")
dbt = dbtRunner()
res = dbt.invoke(["compile", "--inline", "select * from {{ ref('i') }}"])
# Event for parsing error
patched_fire_event.assert_called_once()
assert str(res.exception) == "Error parsing inline query"

def test_graph_summary_output(self, project):
"""Ensure that the compile command generates a file named graph_summary.json
in the target directory, that the file contains valid json, and that the
Expand Down
1 change: 1 addition & 0 deletions tests/unit/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ def test_event_codes(self):
ref_model_latest_version="",
),
types.UnsupportedConstraintMaterialization(materialized=""),
types.ParseNodeError(exc=""),
# M - Deps generation ======================
types.GitSparseCheckoutSubdirectory(subdir=""),
types.GitProgressCheckoutRevision(revision=""),
Expand Down