[#QUARTZ-354] Improvements to NthIncludedDayTrigger fireAtTime

Quartz Scheduler

Improvements to NthIncludedDayTrigger fireAtTime

Created: 05/Mar/06 05:32 PM Updated: 24/Dec/07 01:30 AM

Component/s:

Core

Affects Version/s:

1.5.2

Fix Version/s:

1.6

File Attachments:

NthIncludedDayTrigger.java (40 kb)

Flags:

Patch

Description

« Hide

A few modifications. First, setFireAtTime should throw the IllegalArgumentException with a nested exeption, otherwise you can get some strange error messages. For the (currently invalid) string "17:45:00":

java.lang.IllegalArgumentException: Could not parse time expression: For input string: "45:00"

-Also added the ability to specify a second portion of the fireTime optionally.
-getFireAtTime is modified to return a correctly formatted string (previously 1:04 would have returned as 1:4 -- it will now return as 01:04:00).
-other validations ensure valid fireAtTimes (hour limited to 0-23, minute and second to 0-59)

The included code NEEDS TO BE TESTED. I tested individual sections, in another class, but I haven't really beat the thing up. Please make sure you do :-)

All

Comments

Change History

Sort Order:

[ Permlink | « Hide ]

Aaron Craven - [05/Mar/06 05:34 PM ]

The updated class... I actually copied this out of my browser from xwork. I hope that doesn't cause any problems. If so, let me know and I guess I could use the download link instead and re-edit.

[ Permlink | « Hide ]

Jasper Rosenberg - [21/Mar/06 02:51 PM ]

Just a note that IllegalArgumentException didn't support a nested exception in Java 1.3 which I understand is the earliest Java version Quartz should support.

See: http://java.sun.com/j2se/1.3/docs/api/java/lang/IllegalArgumentException.html

[ Permlink | « Hide ]

Aaron Craven - [21/Mar/06 06:04 PM ]

Hmm...

Well, I guess we'll need to change the line

throw new IllegalArgumentException("Could not parse time expression '" + fireAtTime + "'", e);

to something a bit more compatible... At a minimum, we can simply remove the nested exception:

throw new IllegalArgumentException("Could not parse time expression '" + fireAtTime + "'");

But I'd really like to include the root cause (not just e.getMessage) in the message of the IllegalArgumentException.

Of course, the other option would be to finally tell everybody it's time to move to 1.4 :-) (just kidding)

[ Permlink | « Hide ]

Jasper Rosenberg - [17/May/06 08:39 AM ]

Fine with me, I'm on Java 5 ;)

Seriously though, it might makes sense to detect if Exception supports initCause() and, if so, use it.

Another issue I noticed with your change, is that if setFireAtTime() throws an exception, it sets the time back to 12:00:00, but that wouldn't have been the behavior I would expect as a user I don't think... I would expect this unit test to pass (namely a setFireAtTime() exception would leave the time unchanged):

trigger.setFireAtTime("14:30:10");
try {
trigger.setFireAtTime("blah");
} catch (IllegalArgumentException ignore) {
}

assertEquals("14:30:10", trigger.getFireAtTime());

[ Permlink | « Hide ]

Jasper Rosenberg - [17/May/06 11:09 PM ]

Sorry, I just realized that side effect behavior above was actually a pre-existing issue not introduced by your patch.

[ Permlink | « Hide ]

Aaron Craven - [17/May/06 11:46 PM ]

Not a problem... I wrote the original code anyway :D

I forget why I originally did that, but it makes sense to me to make it act more like a "rollback" than to set it back to the default as you suggest.

[ Permlink | « Hide ]

Jasper Rosenberg - [18/May/06 09:48 AM ]

All set, including some basic unit testing. If you have a chance Aaron, please review my merge, thanks!

[ Permlink | « Hide ]

Aaron Craven - [18/May/06 10:26 AM ]

Looks good to me. Thanks for putting this stuff in.