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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
