addressing PR feedback

docs/docs/configuration/alerts-reports.mdx

@@ -195,13 +195,13 @@ Please refer to `ExecutorType` in the codebase for other executor types.
  its default value of `http://0.0.0.0:8080/`.
 
 
-It's also possible to specify a minimum interval (in minutes) between each report's execution through the config file:
+It's also possible to specify a minimum interval between each report's execution through the config file:
 
 ``` python
 # Set a minimum interval threshold between executions (for each Alert/Report)
-# Value should be the amount of minutes (integer). Min: 1, Max: 60
-ALERT_MINIMUM_INTERVAL_MINUTES = 10 # Change it as desired
-REPORT_MINIMUM_INTERVAL_MINUTES = 10 # Change it as desired
+# Value should be an integer
+ALERT_MINIMUM_INTERVAL = int(timedelta(minutes=10).total_seconds())
+REPORT_MINIMUM_INTERVAL = int(timedelta(minutes=5).total_seconds())
 ```
 
 

superset/commands/report/base.py

@@ -17,6 +17,7 @@
 import logging
 from typing import Any
 
+from croniter import croniter
 from flask import current_app
 from marshmallow import ValidationError
 
@@ -92,29 +93,31 @@ def validate_report_frequency(
  :param report_type: The report type (Alert/Report).
  """
  config_key = (
- "ALERT_MINIMUM_INTERVAL_MINUTES"
+ "ALERT_MINIMUM_INTERVAL"
  if report_type == ReportScheduleType.ALERT
- else "REPORT_MINIMUM_INTERVAL_MINUTES"
+ else "REPORT_MINIMUM_INTERVAL"
  )
- minimum_interval = current_app.config.get(config_key)
- if not minimum_interval or minimum_interval < 2:
+ minimum_interval = current_app.config.get(config_key, 0)
+
+ if not isinstance(minimum_interval, int):
+ logger.error(
+ "Invalid value for %s: %s", config_key, minimum_interval, exc_info=True
+ )
+ return
+
+ # Since configuration is in minutes, we only need to validate
+ # in case `minimum_interval` is <= 120 (2min)
+ if minimum_interval < 120:
  return
 
- minutes_interval = cron_schedule.split(" ")[0]
- # Check if cron is set to every minute (*)
- # or if it has an every-minute block (1-5)
- if "-" in minutes_interval or minutes_interval == "*":
- raise ReportScheduleFrequencyNotAllowed(report_type, minimum_interval)
-
- # Calculate minimum interval (in minutes)
- minutes_list = minutes_interval.split(",")
- if len(minutes_list) > 1:
- scheduled_minutes = [int(min) for min in minutes_list]
- scheduled_minutes.sort()
- differences = [
- scheduled_minutes[i] - scheduled_minutes[i - 1]
- for i in range(1, len(scheduled_minutes))
- ]
-
- if min(differences) < minimum_interval:
- raise ReportScheduleFrequencyNotAllowed(report_type, minimum_interval)
+ iterations = 60 if minimum_interval <= 3660 else 24
+ schedule = croniter(cron_schedule)
+ current_exec = next(schedule)
+
+ for _ in range(iterations):
+ next_exec = next(schedule)
+ diff, current_exec = next_exec - current_exec, next_exec
+ if int(diff) < minimum_interval:
+ raise ReportScheduleFrequencyNotAllowed(
+ report_type=report_type, minimum_interval=minimum_interval
+ )

superset/commands/report/exceptions.py

@@ -99,14 +99,23 @@ class ReportScheduleFrequencyNotAllowed(ValidationError):
  frequently than allowed
  """
 
- def __init__(self, report_type: str, minimum_interval: str) -> None:
+ def __init__(
+ self,
+ report_type: str = "Report",
+ minimum_interval: int = 120,
+ ) -> None:
+ interval_in_minutes = (
+ minimum_interval / 60
+ if not minimum_interval % 60
+ else minimum_interval // 60 + 1
+ )
  super().__init__(
  _(
  "%(report_type)s schedule frequency exceeding limit."
  " Please configure a schedule with a minimum interval of"
- " %(minimum_interval)s minutes per execution.",
+ " %(minimum_interval)d minutes per execution.",
  report_type=report_type,
- minimum_interval=minimum_interval,
+ minimum_interval=interval_in_minutes,
  ),
  field_name="crontab",
  )

superset/config.py

@@ -1326,9 +1326,9 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
 ALERT_REPORTS_MIN_CUSTOM_SCREENSHOT_WIDTH = 600
 ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH = 2400
 # Set a minimum interval threshold between executions (for each Alert/Report)
-# Value should be the amount of minutes (integer). Min: 1, Max: 60
-ALERT_MINIMUM_INTERVAL_MINUTES = None
-REPORT_MINIMUM_INTERVAL_MINUTES = None
+# Value should be an integer i.e. int(timedelta(minutes=5).total_seconds())
+ALERT_MINIMUM_INTERVAL = None
+REPORT_MINIMUM_INTERVAL = None
 
 # A custom prefix to use on all Alerts & Reports emails
 EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] "

tests/integration_tests/reports/api_tests.py

@@ -16,8 +16,7 @@
 # under the License.
 # isort:skip_file
 """Unit tests for Superset"""
-
-from datetime import datetime
+from datetime import datetime, timedelta
 from unittest.mock import patch
 import json
 
@@ -1294,8 +1293,8 @@ def test_create_report_schedule_valid_schedule(self):
  with patch.dict(
  "superset.commands.report.base.current_app.config",
  {
- "ALERT_MINIMUM_INTERVAL_MINUTES": 2,
- "REPORT_MINIMUM_INTERVAL_MINUTES": 5,
+ "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=2).total_seconds()),
+ "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=5).total_seconds()),
  },
  ):
  uri = "api/v1/report/"
@@ -1341,8 +1340,8 @@ def test_create_report_schedule_invalid_schedule(self):
  with patch.dict(
  "superset.commands.report.base.current_app.config",
  {
- "ALERT_MINIMUM_INTERVAL_MINUTES": 6,
- "REPORT_MINIMUM_INTERVAL_MINUTES": 8,
+ "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=6).total_seconds()),
+ "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=8).total_seconds()),
  },
  ):
  uri = "api/v1/report/"
@@ -1391,8 +1390,8 @@ def test_update_report_schedule_valid_schedule(self) -> None:
  with patch.dict(
  "superset.commands.report.base.current_app.config",
  {
- "ALERT_MINIMUM_INTERVAL_MINUTES": 5,
- "REPORT_MINIMUM_INTERVAL_MINUTES": 3,
+ "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=5).total_seconds()),
+ "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=3).total_seconds()),
  },
  ):
  # Test alert minimum interval
@@ -1433,8 +1432,8 @@ def test_update_report_schedule_invalid_schedule(self) -> None:
  with patch.dict(
  "superset.commands.report.base.current_app.config",
  {
- "ALERT_MINIMUM_INTERVAL_MINUTES": 6,
- "REPORT_MINIMUM_INTERVAL_MINUTES": 4,
+ "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=6).total_seconds()),
+ "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=4).total_seconds()),
  },
  ):
  # Exceed alert minimum interval

tests/unit_tests/commands/report/base_test.py

@@ -15,6 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 
+from datetime import timedelta
 from functools import wraps
 from typing import Any, Callable
 from unittest.mock import patch
@@ -55,8 +56,8 @@ def app_custom_config(
  """
  Decorator to mock the current_app.config values dynamically for each test.
 
- :param alert_minimum_interval: Minimum interval in minutes for alerts. Defaults to None.
- :param report_minimum_interval: Minimum interval in minutes for reports. Defaults to None.
+ :param alert_minimum_interval: Minimum interval. Defaults to None.
+ :param report_minimum_interval: Minimum interval. Defaults to None.
 
  :returns: A decorator that wraps a function.
  """
@@ -67,32 +68,35 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
  with patch(
  "superset.commands.report.base.current_app.config"
  ) as mock_config:
- mock_config.get.side_effect = lambda key: {
- "ALERT_MINIMUM_INTERVAL_MINUTES": alert_minimum_interval,
- "REPORT_MINIMUM_INTERVAL_MINUTES": report_minimum_interval,
- }.get(key)
+ mock_config.get.side_effect = lambda key, default=0: {
+ "ALERT_MINIMUM_INTERVAL": alert_minimum_interval,
+ "REPORT_MINIMUM_INTERVAL": report_minimum_interval,
+ }.get(key, default)
  return func(*args, **kwargs)
 
  return wrapper
 
  return decorator
 
 
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
 @app_custom_config()
-def test_validate_report_frequency() -> None:
+def test_validate_report_frequency(report_type: str, schedule: str) -> None:
  """
  Test the ``validate_report_frequency`` method when there's
  no minimum frequency configured.
  """
- for report_type in REPORT_TYPES:
- for schedule in TEST_SCHEDULES:
- BaseReportScheduleCommand().validate_report_frequency(
- schedule,
- report_type,
- )
+ BaseReportScheduleCommand().validate_report_frequency(
+ schedule,
+ report_type,
+ )
 
 
-@app_custom_config(alert_minimum_interval=4, report_minimum_interval=5)
+@app_custom_config(
+ alert_minimum_interval=int(timedelta(minutes=4).total_seconds()),
+ report_minimum_interval=int(timedelta(minutes=5).total_seconds()),
+)
 def test_validate_report_frequency_minimum_set() -> None:
  """
  Test the ``validate_report_frequency`` method when there's
@@ -109,7 +113,10 @@ def test_validate_report_frequency_minimum_set() -> None:
  )
 
 
-@app_custom_config(alert_minimum_interval=2, report_minimum_interval=5)
+@app_custom_config(
+ alert_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+ report_minimum_interval=int(timedelta(minutes=5).total_seconds()),
+)
 def test_validate_report_frequency_invalid_schedule() -> None:
  """
  Test the ``validate_report_frequency`` method when the configured
@@ -128,64 +135,92 @@ def test_validate_report_frequency_invalid_schedule() -> None:
  )
 
 
-@app_custom_config(alert_minimum_interval=10)
-def test_validate_report_frequency_alert_only() -> None:
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
+@app_custom_config(
+ alert_minimum_interval=int(timedelta(minutes=10).total_seconds()),
+)
+def test_validate_report_frequency_alert_only(schedule: str) -> None:
  """
  Test the ``validate_report_frequency`` method when there's
  only a configuration for alerts and user is creating report.
  """
- for schedule in TEST_SCHEDULES:
- BaseReportScheduleCommand().validate_report_frequency(
- schedule,
- ReportScheduleType.REPORT,
- )
+ BaseReportScheduleCommand().validate_report_frequency(
+ schedule,
+ ReportScheduleType.REPORT,
+ )
 
 
-@app_custom_config(report_minimum_interval=10)
-def test_validate_report_frequency_report_only() -> None:
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
+@app_custom_config(
+ report_minimum_interval=int(timedelta(minutes=10).total_seconds()),
+)
+def test_validate_report_frequency_report_only(schedule: str) -> None:
  """
  Test the ``validate_report_frequency`` method when there's
  only a configuration for reports and user is creating alert.
  """
- for schedule in TEST_SCHEDULES:
- BaseReportScheduleCommand().validate_report_frequency(
- schedule,
- ReportScheduleType.ALERT,
- )
+ BaseReportScheduleCommand().validate_report_frequency(
+ schedule,
+ ReportScheduleType.ALERT,
+ )
 
 
-@app_custom_config(alert_minimum_interval=1, report_minimum_interval=1)
-def test_validate_report_frequency_accepts_every_minute_with_one() -> None:
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
+@app_custom_config(
+ alert_minimum_interval=int(timedelta(minutes=1).total_seconds()),
+ report_minimum_interval=int(timedelta(minutes=1).total_seconds()),
+)
+def test_validate_report_frequency_accepts_every_minute_with_one(
+ report_type: str, schedule: str
+) -> None:
  """
  Test the ``validate_report_frequency`` method when configuration
  is set to `1`. Validates the usage of `-` and `*` in the cron.
  """
- for report_type in REPORT_TYPES:
- for schedule in TEST_SCHEDULES:
- BaseReportScheduleCommand().validate_report_frequency(
- schedule,
- report_type,
- )
+ BaseReportScheduleCommand().validate_report_frequency(
+ schedule,
+ report_type,
+ )
+
+
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES_SINGLE_MINUTES)
+@app_custom_config(
+ alert_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+ report_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+)
+def test_validate_report_frequency_accepts_every_minute_with_two(
+ report_type: str,
+ schedule: str,
+) -> None:
+ """
+ Test the ``validate_report_frequency`` method when configuration
+ is set to `2`.
+ """
+ BaseReportScheduleCommand().validate_report_frequency(
+ schedule,
+ report_type,
+ )
 
 
-@app_custom_config(alert_minimum_interval=2, report_minimum_interval=2)
-def test_validate_report_frequency_accepts_every_minute_with_two() -> None:
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES_EVERY_MINUTE)
+@app_custom_config(
+ alert_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+ report_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+)
+def test_validate_report_frequency_accepts_every_minute_with_two_raises(
+ report_type: str,
+ schedule: str,
+) -> None:
  """
  Test the ``validate_report_frequency`` method when configuration
  is set to `2`. Validates the usage of `-` and `*` in the cron.
  """
- for report_type in REPORT_TYPES:
- # Should fail for schedules with `-` and `*`
- for schedule in TEST_SCHEDULES_EVERY_MINUTE:
- with pytest.raises(ReportScheduleFrequencyNotAllowed):
- BaseReportScheduleCommand().validate_report_frequency(
- schedule,
- report_type,
- )
-
- # Should work for schedules with single with bigger intervals
- for schedule in TEST_SCHEDULES_SINGLE_MINUTES:
- BaseReportScheduleCommand().validate_report_frequency(
- schedule,
- report_type,
- )
+ # Should fail for schedules with `-` and `*`
+ with pytest.raises(ReportScheduleFrequencyNotAllowed):
+ BaseReportScheduleCommand().validate_report_frequency(
+ schedule,
+ report_type,
+ )