You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "tkobayas (via GitHub)" <gi...@apache.org> on 2026/06/25 03:01:50 UTC
[PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
tkobayas opened a new pull request, #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774
- https://github.com/apache/incubator-kie-drools/issues/6773
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression wildcard validation [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#issuecomment-4805793109
@porcelli I wrote
> `99` is a special `int` to express `*` internally
but, it means only for `startAt` value. Sorry for the confusion.
"0 1/99 * * * ?" means `for (i = 1; i <= 59; i += 99)` , so only minute 1 is triggered.
"0 1-5/99 * * * ?" means `for (i = 1; i <= 5; i += 99)`, so only minute 1 is triggered.
The expression appears to be a user mistake, but it is not syntactically invalid. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#issuecomment-4796780238
GHA `CI :: Build / macos-latest, Java 17` failure is not related to this PR. (Filed https://github.com/apache/incubator-kie-drools/issues/6775)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#discussion_r3472010079
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
Review Comment:
This `if` allows `*`, but rejects `99`, `99-9999`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#discussion_r3472010079
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
Review Comment:
`*` is internally `99` (`ALL_SPEC_INT`). This guard means "if `*` or `99` is used, range (`99-XXX`) is not allowed". `incr` is used for `*/5`, so it's allowed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#discussion_r3472123438
##########
drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/TimerAndCalendarWithPseudoTimeTest.java:
##########
@@ -298,6 +298,13 @@ public void testMalformedCronExpression(KieBaseTestConfiguration kieBaseTestConf
wrongTimerExpression(kieBaseTestConfiguration, "cron: 0/30 * * * * *");
}
+ @ParameterizedTest(name = "KieBase type={0}")
+ @MethodSource("parameters")
+ @Timeout(10000)
+ public void testLargeRangeValueInCronExpression(KieBaseTestConfiguration kieBaseTestConfiguration) {
Review Comment:
outdated
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
+ throw new ParseException(
+ "Wildcard value is not valid as a range bound", -1);
+ }
Review Comment:
outdated
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
+ throw new ParseException(
+ "Wildcard value is not valid as a range bound", -1);
+ }
Review Comment:
outdated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#discussion_r3472010079
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
Review Comment:
This `if` allows `*`, but rejects `99`, `99-9999`, `*-9999`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#discussion_r3472069247
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
+ throw new ParseException(
+ "Wildcard value is not valid as a range bound", -1);
+ }
Review Comment:
The new ALL_SPEC_INT (99) guard closes the plain "99" and range-start "99-…" cases, but it still allows the increment form "99/5" (or similar) to parse successfully. Due to the sentinel collision, "99/5" is interpreted like "*/5" (see storeExpressionVals() digit path -> checkNext('/') -> addToSet(val=99,end=-1,incr>0), and addToSet() later normalizes startAt==ALL_SPEC_INT to 0). That means an out-of-range value can still bypass validation and change semantics instead of throwing ParseException.
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
+ throw new ParseException(
+ "Wildcard value is not valid as a range bound", -1);
+ }
Review Comment:
This guard is also triggered by the numeric value "99" (because ALL_SPEC_INT=99), even when the input isn't a wildcard or a range bound. In that case the ParseException message "Wildcard value is not valid as a range bound" is misleading for users and makes debugging harder. Consider rewording it to describe the actual problem (the reserved sentinel value being used as a numeric value / range bound).
##########
drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/TimerAndCalendarWithPseudoTimeTest.java:
##########
@@ -298,6 +298,13 @@ public void testMalformedCronExpression(KieBaseTestConfiguration kieBaseTestConf
wrongTimerExpression(kieBaseTestConfiguration, "cron: 0/30 * * * * *");
}
+ @ParameterizedTest(name = "KieBase type={0}")
+ @MethodSource("parameters")
+ @Timeout(10000)
+ public void testLargeRangeValueInCronExpression(KieBaseTestConfiguration kieBaseTestConfiguration) {
Review Comment:
@Timeout(10000) uses JUnit 5's default unit (seconds), so this new test can run for ~2.7 hours if the cron parsing regresses into a slow path. Since this test is specifically guarding against pathological cron input, it should use a small, explicit timeout unit (e.g., 10 seconds).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools#6773] Improve CronExpression range validation [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6774:
URL: https://github.com/apache/incubator-kie-drools/pull/6774#discussion_r3472010079
##########
drools-core/src/main/java/org/drools/core/time/impl/KieCronExpression.java:
##########
@@ -798,6 +798,11 @@ protected void addToSet(int val, int end, int incr, int type)
TreeSet set = getSet(type);
+ if (val == ALL_SPEC_INT && (end != -1 || incr < 0)) {
Review Comment:
This `if` allows `*`, but rejects `99`, `99-9999`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org