From f10bc6d8fe7f3fa4056db2aaff8256f9c3e1550b Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 12 Nov 2021 12:28:17 -0800 Subject: [PATCH] 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 --- 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 f81babe5eba78..5affcf20a7dc6 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 @@ -765,15 +766,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 2180e4d22ac8a..c8486b8e3d274 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -21,6 +21,7 @@ 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 38243823f9f8b..9c275addf6b71 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 7526d11cdbb38..8df8b4bc8bee5 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 @@ def test_create_report_schedule_schema(self): 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 c905df2797d35..9042ccad58534 100644 --- a/tests/unit_tests/tasks/test_cron_util.py +++ b/tests/unit_tests/tasks/test_cron_util.py @@ -64,6 +64,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", [