diff --git a/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java index bd397840..1d3f8634 100644 --- a/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java @@ -19,7 +19,7 @@ import com.cronutils.utils.Preconditions; import com.cronutils.utils.StringUtils; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.ResourceBundle; import java.util.Set; @@ -59,7 +59,7 @@ class TimeDescriptionStrategy extends DescriptionStrategy { this.hours = ensureInstance(hours, always()); this.minutes = ensureInstance(minutes, always()); this.seconds = ensureInstance(seconds, new On(new IntegerFieldValue(DEFAULTSECONDS))); - descriptions = new HashSet<>(); + descriptions = new LinkedHashSet<>(); registerFunctions(); } @@ -85,20 +85,23 @@ private FieldExpression ensureInstance(final FieldExpression expression, final F public String describe() { final TimeFields fields = new TimeFields(hours, minutes, seconds); for (final Function function : descriptions) { - if (!"".equals(function.apply(fields))) { - return function.apply(fields); + final String description = function.apply(fields); + if (!"".equals(description)) { + return description; } } String secondsDesc = ""; String minutesDesc = ""; String hoursDesc = ""; + // minute 0 may only stay implicit while seconds are too, else we claim an unbounded sub-minute frequency + final boolean defaultSeconds = seconds instanceof On && isDefault((On) seconds); if (!(hours instanceof Always)) { hoursDesc = addTimeExpressions(describe(hours), bundle.getString(HOUR), bundle.getString("hours")); } - if (!(minutes instanceof On && isDefault((On) minutes)) && !((minutes instanceof Always) && (hours instanceof Always))) { + if (!(minutes instanceof On && isDefault((On) minutes) && defaultSeconds) && !((minutes instanceof Always) && (hours instanceof Always))) { minutesDesc = addTimeExpressions(describe(minutes), bundle.getString(MINUTE), bundle.getString("minutes")); } - if (!(seconds instanceof On && isDefault((On) seconds))) { + if (!defaultSeconds) { secondsDesc = addTimeExpressions(describe(seconds), bundle.getString(SECOND), bundle.getString("seconds")); } return String.format("%s %s %s", secondsDesc, minutesDesc, hoursDesc); diff --git a/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java b/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java new file mode 100644 index 00000000..b96de220 --- /dev/null +++ b/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java @@ -0,0 +1,51 @@ +package com.cronutils.utils.descriptor; + +import com.cronutils.descriptor.CronDescriptor; +import com.cronutils.model.CronType; +import com.cronutils.model.definition.CronDefinitionBuilder; +import com.cronutils.parser.CronParser; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.util.Locale; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Issue 3 - a cron firing on an explicit minute 0 lost that minute in its description, so + * "* 0 9-23 * * ?" read as "every second every hour between 9 and 23" and claimed roughly + * sixty times the firings the expression actually produces. + */ +public class Issue3Test { + + private final CronParser parser = new CronParser(CronDefinitionBuilder.instanceDefinitionFor(CronType.QUARTZ)); + + @ParameterizedTest + @CsvSource({ + "'* 0 9-23 * * ?', 'every second at 0 minute every hour between 9 and 23'", + "'5 0 9-23 * * ?', 'at 5 second at 0 minute every hour between 9 and 23'", + "'0/5 0 * * * ?', 'every 5 seconds from second 0 at 0 minute'", + "'* 0 */4 * * ?', 'every second at 0 minute every 4 hours'" + }) + public void explicitMinuteZeroIsDescribedWhenSecondsAreNotImplicit(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + + @ParameterizedTest + @CsvSource({ + "'0 0 9-23 * * ?', 'every hour between 9 and 23'", + "'0 0 10 * * ?', 'at 10:00'" + }) + public void minuteZeroStaysImplicitWhenSecondsAreImplicitToo(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + + @ParameterizedTest + @CsvSource({ + "'* 30 9-23 * * ?', 'every second at 30 minute every hour between 9 and 23'", + "'* 0 10 * * ?', 'every second at 10:00'" + }) + public void nonZeroAndCollapsibleMinutesAreUnaffected(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } +}