Added better validation for cron trigger expressions

Fixes #189.

4 files changed, 52 insertions, 4 deletions

diff --git a/apscheduler/triggers/cron/expressions.py b/apscheduler/triggers/cron/expressions.py
index 21493d5..131a80f 100644
--- a/apscheduler/triggers/cron/expressions.py
+++ b/apscheduler/triggers/cron/expressions.py
@@ -20,6 +20,14 @@ class AllExpression(object):
         if self.step == 0:
             raise ValueError('Increment must be higher than 0')
 
+    def validate_range(self, field_name):
+        from apscheduler.triggers.cron.fields import MIN_VALUES, MAX_VALUES
+
+        value_range = MAX_VALUES[field_name] - MIN_VALUES[field_name]
+        if self.step and self.step > value_range:
+            raise ValueError('the step value ({}) is higher than the total range of the '
+                             'expression ({})'.format(self.step, value_range))
+
     def get_next_value(self, date, field):
         start = field.get_value(date)
         minval = field.get_min(date)
@@ -52,7 +60,7 @@ class RangeExpression(AllExpression):
         r'(?P<first>\d+)(?:-(?P<last>\d+))?(?:/(?P<step>\d+))?$')
 
     def __init__(self, first, last=None, step=None):
-        AllExpression.__init__(self, step)
+        super(RangeExpression, self).__init__(step)
         first = asint(first)
         last = asint(last)
         if last is None and step is None:
@@ -62,6 +70,21 @@ class RangeExpression(AllExpression):
         self.first = first
         self.last = last
 
+    def validate_range(self, field_name):
+        from apscheduler.triggers.cron.fields import MIN_VALUES, MAX_VALUES
+
+        super(RangeExpression, self).validate_range(field_name)
+        if self.first < MIN_VALUES[field_name]:
+            raise ValueError('the first value ({}) is lower than the minimum value ({})'
+                             .format(self.first, MIN_VALUES[field_name]))
+        if self.last is not None and self.last > MAX_VALUES[field_name]:
+            raise ValueError('the last value ({}) is higher than the maximum value ({})'
+                             .format(self.last, MAX_VALUES[field_name]))
+        value_range = (self.last or MAX_VALUES[field_name]) - self.first
+        if self.step and self.step > value_range:
+            raise ValueError('the step value ({}) is higher than the total range of the '
+                             'expression ({})'.format(self.step, value_range))
+
     def get_next_value(self, date, field):
         startval = field.get_value(date)
         minval = field.get_min(date)
@@ -119,7 +142,7 @@ class WeekdayRangeExpression(RangeExpression):
         else:
             last_num = None
 
-        RangeExpression.__init__(self, first_num, last_num)
+        super(WeekdayRangeExpression, self).__init__(first_num, last_num)
 
     def __str__(self):
         if self.last != self.first and self.last is not None:
@@ -139,6 +162,7 @@ class WeekdayPositionExpression(AllExpression):
                           '|'.join(options), re.IGNORECASE)
 
     def __init__(self, option_name, weekday_name):
+        super(WeekdayPositionExpression, self).__init__(None)
         try:
             self.option_num = self.options.index(option_name.lower())
         except ValueError:
@@ -183,7 +207,7 @@ class LastDayOfMonthExpression(AllExpression):
     value_re = re.compile(r'last', re.IGNORECASE)
 
     def __init__(self):
-        pass
+        super(LastDayOfMonthExpression, self).__init__(None)
 
     def get_next_value(self, date, field):
         return monthrange(date.year, date.month)[1]
diff --git a/apscheduler/triggers/cron/fields.py b/apscheduler/triggers/cron/fields.py
index 1f101c8..811127b 100644
--- a/apscheduler/triggers/cron/fields.py
+++ b/apscheduler/triggers/cron/fields.py
@@ -2,6 +2,8 @@
 
 from calendar import monthrange
 
+import six
+
 from apscheduler.triggers.cron.expressions import (
     AllExpression, RangeExpression, WeekdayPositionExpression, LastDayOfMonthExpression,
     WeekdayRangeExpression)
@@ -13,7 +15,7 @@ __all__ = ('MIN_VALUES', 'MAX_VALUES', 'DEFAULT_VALUES', 'BaseField', 'WeekField
 
 MIN_VALUES = {'year': 1970, 'month': 1, 'day': 1, 'week': 1, 'day_of_week': 0, 'hour': 0,
               'minute': 0, 'second': 0}
-MAX_VALUES = {'year': 9999, 'month': 12, 'day:': 31, 'week': 53, 'day_of_week': 6, 'hour': 23,
+MAX_VALUES = {'year': 9999, 'month': 12, 'day': 31, 'week': 53, 'day_of_week': 6, 'hour': 23,
               'minute': 59, 'second': 59}
 DEFAULT_VALUES = {'year': '*', 'month': 1, 'day': 1, 'week': '*', 'day_of_week': '*', 'hour': 0,
                   'minute': 0, 'second': 0}
@@ -62,6 +64,13 @@ class BaseField(object):
             match = compiler.value_re.match(expr)
             if match:
                 compiled_expr = compiler(**match.groupdict())
+
+                try:
+                    compiled_expr.validate_range(self.name)
+                except ValueError as e:
+                    exc = ValueError('Error validating expression {!r}: {}'.format(expr, e))
+                    six.raise_from(exc, None)
+
                 self.expressions.append(compiled_expr)
                 return
 
diff --git a/docs/versionhistory.rst b/docs/versionhistory.rst
index 632ae54..98b8ea5 100644
--- a/docs/versionhistory.rst
+++ b/docs/versionhistory.rst
@@ -11,6 +11,8 @@ APScheduler, see the :doc:`migration section <migration>`.
 
 * Added the ``jitter`` options to ``IntervalTrigger`` and ``CronTrigger`` (thanks to gilbsgilbs)
 
+* Added better validation for the steps and ranges of different expressions in ``CronTrigger``
+
 * Fixed memory leak due to a cyclic reference when jobs raise exceptions
   (thanks to gilbsgilbs for help on solving this)
 
diff --git a/tests/test_triggers.py b/tests/test_triggers.py
index d854800..0632d92 100644
--- a/tests/test_triggers.py
+++ b/tests/test_triggers.py
@@ -374,6 +374,19 @@ class TestCronTrigger(object):
             next_fire_time = trigger.get_next_fire_time(None, start_date)
             assert abs(next_fire_time - correct_next_date) <= timedelta(seconds=5)
 
+    @pytest.mark.parametrize('values, expected', [
+        (dict(day='*/31'), "Error validating expression '\*/31': the step value \(31\) is higher "
+                           "than the total range of the expression \(30\)"),
+        (dict(day='4-6/3'), "Error validating expression '4-6/3': the step value \(3\) is higher "
+                            "than the total range of the expression \(2\)"),
+        (dict(hour='0-24'), "Error validating expression '0-24': the last value \(24\) is higher "
+                            "than the maximum value \(23\)"),
+        (dict(day='0-3'), "Error validating expression '0-3': the first value \(0\) is lower than "
+                          "the minimum value \(1\)")
+    ], ids=['too_large_step_all', 'too_large_step_range', 'too_high_last', 'too_low_first'])
+    def test_invalid_ranges(self, values, expected):
+        pytest.raises(ValueError, CronTrigger, **values).match(expected)
+
 
 class TestDateTrigger(object):
     @pytest.mark.parametrize('run_date,alter_tz,previous,now,expected', [