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.

Sign up for GitHub

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

Jump to bottom

feat(alerts): apply SQL limit to all alerts #13150

Merged
merged 5 commits into from
Feb 17, 2021
Merged

feat(alerts): apply SQL limit to all alerts #13150

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
119e90f
feat(alerts): apply SQL limit to all alerts
dpgaspar Feb 16, 2021
4256f97
change limit to 2 and test
dpgaspar Feb 17, 2021
ccf6f0a
undo mock
dpgaspar Feb 17, 2021
9020e46
mock, mock and mock
dpgaspar Feb 17, 2021
83b84f5
lint
dpgaspar Feb 17, 2021
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
5 changes: 4 additions & 1 deletion superset/reports/commands/alert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,10 @@ def validate(self) -> None:
)
rendered_sql = sql_template.process_template(self._report_schedule.sql)
try:
df = self._report_schedule.database.get_df(rendered_sql)
limited_rendered_sql = self._report_schedule.database.apply_limit_to_sql(
rendered_sql, 1
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should set the limit to 2 - the system expects a single row and raises an error if multiple rows are returned. This error could surface an issue with the query, or the data being accessed. Setting the limit to 1 changes the behavior of the query in a way that could hide issues.

Copy link
Member Author

@dpgaspar dpgaspar Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Very good idea!

)
df = self._report_schedule.database.get_df(limited_rendered_sql)
except Exception as ex:
raise AlertQueryError(message=str(ex))

Expand Down
87 changes: 53 additions & 34 deletions tests/reports/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,17 @@
)
from superset.reports.commands.execute import AsyncExecuteReportScheduleCommand
from superset.utils.core import get_example_database
from tests.fixtures.world_bank_dashboard import (
load_world_bank_dashboard_with_slices_module_scope,
)
from tests.reports.utils import insert_report_schedule
from tests.test_app import app
from tests.utils import read_fixture

pytestmark = pytest.mark.usefixtures(
"load_world_bank_dashboard_with_slices_module_scope"
)


def get_target_from_report_schedule(report_schedule) -> List[str]:
return [
Expand Down Expand Up @@ -129,6 +136,22 @@ def cleanup_report_schedule(report_schedule: ReportSchedule) -> None:
db.session.commit()


@contextmanager
def create_test_table_context(database: Database):
database.get_sqla_engine().execute(
"CREATE TABLE test_table AS SELECT 1 as first, 2 as second"
)
database.get_sqla_engine().execute(
"INSERT INTO test_table (first, second) VALUES (1, 2)"
)
database.get_sqla_engine().execute(
"INSERT INTO test_table (first, second) VALUES (3, 4)"
)

yield db.session
database.get_sqla_engine().execute("DROP TABLE test_table")


@pytest.yield_fixture()
def create_report_email_chart():
with app.app_context():
Expand Down Expand Up @@ -233,7 +256,16 @@ def create_alert_slack_chart_grace():


@pytest.yield_fixture(
params=["alert1", "alert2", "alert3", "alert4", "alert5", "alert6", "alert7"]
params=[
"alert1",
"alert2",
"alert3",
"alert4",
"alert5",
"alert6",
"alert7",
"alert8",
]
)
def create_alert_email_chart(request):
param_config = {
Expand Down Expand Up @@ -272,39 +304,31 @@ def create_alert_email_chart(request):
"validator_type": ReportScheduleValidatorType.OPERATOR,
"validator_config_json": '{"op": "!=", "threshold": 11}',
},
"alert8": {
"sql": "SELECT first from test_table",
"validator_type": ReportScheduleValidatorType.OPERATOR,
"validator_config_json": '{"op": "<", "threshold": 10}',
},
}
with app.app_context():
chart = db.session.query(Slice).first()
example_database = get_example_database()
with create_test_table_context(example_database):

report_schedule = create_report_notification(
email_target="target@email.com",
chart=chart,
report_type=ReportScheduleType.ALERT,
database=example_database,
sql=param_config[request.param]["sql"],
validator_type=param_config[request.param]["validator_type"],
validator_config_json=param_config[request.param]["validator_config_json"],
)
yield report_schedule

cleanup_report_schedule(report_schedule)


@contextmanager
def create_test_table_context(database: Database):
database.get_sqla_engine().execute(
"CREATE TABLE test_table AS SELECT 1 as first, 2 as second"
)
database.get_sqla_engine().execute(
"INSERT INTO test_table (first, second) VALUES (1, 2)"
)
database.get_sqla_engine().execute(
"INSERT INTO test_table (first, second) VALUES (3, 4)"
)
report_schedule = create_report_notification(
email_target="target@email.com",
chart=chart,
report_type=ReportScheduleType.ALERT,
database=example_database,
sql=param_config[request.param]["sql"],
validator_type=param_config[request.param]["validator_type"],
validator_config_json=param_config[request.param][
"validator_config_json"
],
)
yield report_schedule

yield db.session
database.get_sqla_engine().execute("DROP TABLE test_table")
cleanup_report_schedule(report_schedule)


@pytest.yield_fixture(
Expand Down Expand Up @@ -369,15 +393,10 @@ def create_no_alert_email_chart(request):
cleanup_report_schedule(report_schedule)


@pytest.yield_fixture(params=["alert1", "alert2"])
@pytest.yield_fixture(params=["alert1"])
def create_mul_alert_email_chart(request):
param_config = {
"alert1": {
"sql": "SELECT first from test_table",
"validator_type": ReportScheduleValidatorType.OPERATOR,
"validator_config_json": '{"op": "<", "threshold": 10}',
},
"alert2": {
"sql": "SELECT first, second from test_table",
"validator_type": ReportScheduleValidatorType.OPERATOR,
"validator_config_json": '{"op": "<", "threshold": 10}',
Expand Down