reject invalid cron values

[tshepang]

I can use values greater than 59 for seconds and minutes, and greater than 24 for hours, in a cron expressions, and that gives surprising values without errors.

Example of accepted expression:

0/100 0/100 0/100 * * *

[koenichiwa]

Quick thought, why don't you implement ScheduleFields like:

impl ScheduleFields {
    pub(crate) fn new(
        seconds: Seconds,
        minutes: Minutes,
        hours: Hours,
        days_of_month: DaysOfMonth,
        months: Months,
        days_of_week: DaysOfWeek,
        years: Years,
    ) -> Result<ScheduleFields, Error> {
        if !(seconds.ordinals() - Seconds::all().ordinals()).is_empty() {
            return Err(Error::from(ErrorKind::Expression("Seconds out of range".to_owned())));
        }
        if !(minutes.ordinals() - Minutes::all().ordinals()).is_empty() {
            return Err(Error::from(ErrorKind::Expression("Minutes out of range".to_owned())));
        }
       //...
        if !(years.ordinals() - Years::all().ordinals()).is_empty() {
            // Or maybe format it like this?
            return Err(
                Error::from(
                    ErrorKind::Expression(
                        format!("Years out of range. Invalid values: {:#?}", years.ordinals() - Years::all().ordinals())
                    )
                )
            );
        }
        Ok(ScheduleFields {
            years,
            days_of_week,
            months,
            days_of_month,
            hours,
            minutes,
            seconds,
        })
    }
}

After that it only needs to be propagated through the nom parser.

This might also fix #11

Maybe also look into using something like ordinals.difference(other_ordinals).has_next() instead of the minus sign, because it creates a iter instead of a new BTreeSet.

Edit: apparently has_next doesn't exist. if ordinals.difference(other_ordinals).next().is_some() { ... } Could also do the trick.

[koenichiwa]

I tried to make a draft, but it seems I was mistaken. The nom parser seems to filter the values somewhere(?) I'm not sure how it really works.

[zslayton]

There's already code in cron that validates the ordinals themselves:

cron/src/time_unit/mod.rs

Lines 240 to 260 in 22ea6bc

	fn validate_ordinal(ordinal: Ordinal) -> Result<Ordinal, Error> {
	//println!("validate_ordinal for {} => {}", Self::name(), ordinal);
	match ordinal {
	i if i < Self::inclusive_min() => Err(ErrorKind::Expression(format!(
	"{} must be greater than or equal to {}. ('{}' \
	specified.)",
	Self::name(),
	Self::inclusive_min(),
	i
	))
	.into()),
	i if i > Self::inclusive_max() => Err(ErrorKind::Expression(format!(
	"{} must be less than {}. ('{}' specified.)",
	Self::name(),
	Self::inclusive_max(),
	i
	))
	.into()),
	i => Ok(i),
	}
	}

The trouble in this example's case is that the divisor of the period is out of bounds:

0/100 0/100 0/100 * * *

To address this we'd need to bounds-check the variable step in the function ordinals_from_root_specifier:

cron/src/time_unit/mod.rs

Line 310 in 22ea6bc

base_set.into_iter().step_by(*step as usize).collect()

[zslayton]

Without looking into it too closely, I'd expect each instance of 0/100 to simplify to 0 in the above expression. That's probably not hurting anything, but I agree that returning an Err when the period divisor is nonsensical would be less surprising.