You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2021/11/22 19:55:00 UTC
[superset] 03/03: fix: add fallback and validation for report and cron timezones (#17338)
This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch 1.4
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7d9f63eda71635681c9592f6ed739ccc37d30966
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Fri Nov 12 12:28:17 2021 -0800
fix: add fallback and validation for report and cron timezones (#17338)
* add fallback and validation for report and cron timezones
* add logging to exception catch
* Run black
Co-authored-by: Beto Dealmeida <ro...@dealmeida.net>
(cherry picked from commit f10bc6d8fe7f3fa4056db2aaff8256f9c3e1550b)
---
CONTRIBUTING.md | 13 +++++++---
superset/reports/schemas.py | 13 ++++++++--
superset/tasks/cron_util.py | 14 +++++++---
tests/integration_tests/reports/api_tests.py | 33 ++++++++++++++++++++++++
tests/unit_tests/tasks/test_cron_util.py | 38 ++++++++++++++++++++++++++++
5 files changed, 103 insertions(+), 8 deletions(-)
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 134cdef..8a82a8d 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -420,7 +420,7 @@ For example, the image referenced above actually lives in `superset-frontend/src
#### OS Dependencies
-Make sure your machine meets the [OS dependencies](https://superset.apache.org/docs/installation/installing-superset-from-scratch#os-dependencies) before following these steps.
+Make sure your machine meets the [OS dependencies](https://superset.apache.org/docs/installation/installing-superset-from-scratch#os-dependencies) before following these steps.
You also need to install MySQL or [MariaDB](https://mariadb.com/downloads).
Ensure that you are using Python version 3.7 or 3.8, then proceed with:
@@ -523,6 +523,7 @@ Frontend assets (TypeScript, JavaScript, CSS, and images) must be compiled in or
##### nvm and node
First, be sure you are using the following versions of Node.js and npm:
+
- `Node.js`: Version 16
- `npm`: Version 7
@@ -752,15 +753,21 @@ Note that the test environment uses a temporary directory for defining the
SQLite databases which will be cleared each time before the group of test
commands are invoked.
-There is also a utility script included in the Superset codebase to run python tests. The [readme can be
+There is also a utility script included in the Superset codebase to run python integration tests. The [readme can be
found here](https://github.com/apache/superset/tree/master/scripts/tests)
-To run all tests for example, run this script from the root directory:
+To run all integration tests for example, run this script from the root directory:
```bash
scripts/tests/run.sh
```
+You can run unit tests found in './tests/unit_tests' for example with pytest. It is a simple way to run an isolated test that doesn't need any database setup
+
+```bash
+pytest ./link_to_test.py
+```
+
### Frontend Testing
We use [Jest](https://jestjs.io/) and [Enzyme](https://airbnb.io/enzyme/) to test TypeScript/JavaScript. Tests can be run with:
diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py
index 2180e4d..c8486b8 100644
--- a/superset/reports/schemas.py
+++ b/superset/reports/schemas.py
@@ -21,6 +21,7 @@ from flask_babel import gettext as _
from marshmallow import fields, Schema, validate, validates_schema
from marshmallow.validate import Length, Range, ValidationError
from marshmallow_enum import EnumField
+from pytz import all_timezones
from superset.models.reports import (
ReportCreationMethodType,
@@ -153,7 +154,11 @@ class ReportSchedulePostSchema(Schema):
allow_none=False,
required=True,
)
- timezone = fields.String(description=timezone_description, default="UTC")
+ timezone = fields.String(
+ description=timezone_description,
+ default="UTC",
+ validate=validate.OneOf(choices=tuple(all_timezones)),
+ )
sql = fields.String(
description=sql_description, example="SELECT value FROM time_series_table"
)
@@ -233,7 +238,11 @@ class ReportSchedulePutSchema(Schema):
validate=[validate_crontab, Length(1, 1000)],
required=False,
)
- timezone = fields.String(description=timezone_description, default="UTC")
+ timezone = fields.String(
+ description=timezone_description,
+ default="UTC",
+ validate=validate.OneOf(choices=tuple(all_timezones)),
+ )
sql = fields.String(
description=sql_description,
example="SELECT value FROM time_series_table",
diff --git a/superset/tasks/cron_util.py b/superset/tasks/cron_util.py
index 3824382..9c275ad 100644
--- a/superset/tasks/cron_util.py
+++ b/superset/tasks/cron_util.py
@@ -15,21 +15,29 @@
# specific language governing permissions and limitations
# under the License.
+import logging
from datetime import datetime, timedelta, timezone as dt_timezone
from typing import Iterator
-import pytz
from croniter import croniter
+from pytz import timezone as pytz_timezone, UnknownTimeZoneError
from superset import app
+logger = logging.getLogger(__name__)
+
def cron_schedule_window(cron: str, timezone: str) -> Iterator[datetime]:
window_size = app.config["ALERT_REPORTS_CRON_WINDOW_SIZE"]
# create a time-aware datetime in utc
time_now = datetime.now(tz=dt_timezone.utc)
- tz = pytz.timezone(timezone)
- utc = pytz.timezone("UTC")
+ try:
+ tz = pytz_timezone(timezone)
+ except UnknownTimeZoneError:
+ # fallback to default timezone
+ tz = pytz_timezone("UTC")
+ logger.warning("Timezone %s was invalid. Falling back to 'UTC'", timezone)
+ utc = pytz_timezone("UTC")
# convert the current time to the user's local time for comparison
time_now = time_now.astimezone(tz)
start_at = time_now - timedelta(seconds=1)
diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py
index 7526d11..8df8b4b 100644
--- a/tests/integration_tests/reports/api_tests.py
+++ b/tests/integration_tests/reports/api_tests.py
@@ -19,6 +19,8 @@
from datetime import datetime
import json
+import pytz
+
import pytest
import prison
from sqlalchemy.sql import func
@@ -706,6 +708,37 @@ class TestReportSchedulesApi(SupersetTestCase):
data = json.loads(rv.data.decode("utf-8"))
assert data == {"message": {"timezone": ["Field may not be null."]}}
+ # Test that report cannot be created with an invalid timezone
+ report_schedule_data = {
+ "type": ReportScheduleType.ALERT,
+ "name": "new5",
+ "description": "description",
+ "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+ "crontab": "0 9 * * *",
+ "recipients": [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {"target": "target@superset.org"},
+ },
+ {
+ "type": ReportRecipientType.SLACK,
+ "recipient_config_json": {"target": "channel"},
+ },
+ ],
+ "working_timeout": 3600,
+ "timezone": "this is not a timezone",
+ "dashboard": dashboard.id,
+ "database": example_db.id,
+ }
+ rv = self.client.post(uri, json=report_schedule_data)
+ assert rv.status_code == 400
+ data = json.loads(rv.data.decode("utf-8"))
+ assert data == {
+ "message": {
+ "timezone": [f"Must be one of: {', '.join(pytz.all_timezones)}."]
+ }
+ }
+
# Test that report should reflect the timezone value passed in
report_schedule_data = {
"type": ReportScheduleType.ALERT,
diff --git a/tests/unit_tests/tasks/test_cron_util.py b/tests/unit_tests/tasks/test_cron_util.py
index c905df2..9042cca 100644
--- a/tests/unit_tests/tasks/test_cron_util.py
+++ b/tests/unit_tests/tasks/test_cron_util.py
@@ -67,6 +67,44 @@ def test_cron_schedule_window_los_angeles(
@pytest.mark.parametrize(
"current_dttm, cron, expected",
[
+ ("2020-01-01T00:59:01Z", "0 1 * * *", []),
+ (
+ "2020-01-01T00:59:02Z",
+ "0 1 * * *",
+ [FakeDatetime(2020, 1, 1, 1, 0).strftime("%A, %d %B %Y, %H:%M:%S")],
+ ),
+ (
+ "2020-01-01T00:59:59Z",
+ "0 1 * * *",
+ [FakeDatetime(2020, 1, 1, 1, 0).strftime("%A, %d %B %Y, %H:%M:%S")],
+ ),
+ (
+ "2020-01-01T01:00:00Z",
+ "0 1 * * *",
+ [FakeDatetime(2020, 1, 1, 1, 0).strftime("%A, %d %B %Y, %H:%M:%S")],
+ ),
+ ("2020-01-01T01:00:01Z", "0 1 * * *", []),
+ ],
+)
+def test_cron_schedule_window_invalid_timezone(
+ app_context: AppContext, current_dttm: str, cron: str, expected: List[FakeDatetime]
+) -> None:
+ """
+ Reports scheduler: Test cron schedule window for "invalid timezone"
+ """
+
+ with freeze_time(current_dttm):
+ datetimes = cron_schedule_window(cron, "invalid timezone")
+ # it should default to UTC
+ assert (
+ list(cron.strftime("%A, %d %B %Y, %H:%M:%S") for cron in datetimes)
+ == expected
+ )
+
+
+@pytest.mark.parametrize(
+ "current_dttm, cron, expected",
+ [
("2020-01-01T05:59:01Z", "0 1 * * *", []),
(
"2020-01-01T05:59:02Z",