dbunit Merge Request #44: Add relative value support to Date/Time/Timestamp data types (merged)

Based on the discussion in my previous MR, I have modified Date, Time and Timestamp data types to accept a special syntax specifying a value relative to the current date/time.
Tests are included.
The below is the spec copied from RelativeDateTimeParser's javadoc comment.

---

The basic format is [now{diff...}{time}].
'diff' consists of two parts 1) a number with a leading plus or minus sign and 2) a character represents temporal unit. See the table below for the supported units. There can be multiple 'diff's and they can be specified in any order.
'time' is a string that can be parsed by LocalTime#parse(). If specified, it is used instead of the current time.
Both 'diff' and 'time' are optional.
Whitespaces are allowed before and after each 'diff'.

Unit

• y : years
• M : months
• d : days
• h : hours
• m : minutes
• s : seconds

Here are some examples.

• [now] : current date time.
• [now-1d] : the same time yesterday.
• [now+1y+1M-2h] : a year and a month from today, two hours earlier.
• [now+1d 10:00] : 10 o'clock tomorrow.

---

As there is nothing urgent with this change, please take your time.
I'll update the doc (src/site/xdoc/datatypes.xml) once this request gets the green light.

Thank you,
Iwao

[Jeff Jensen]

Nice implementation. Thanks for the changeup to enhancing DataTypes; I think this is a simpler approach. What do you think of this approach?

A few initial thoughts, please let me know what you think:

1. Can the implementation cache RelativeDateTimeParser? Make it stateless and have parse() take the parameters (remove the constructors, overload parse()). Current implementation creates a RelativeDateTimeParser instance for each applicable column in a row, which is a lot with many rows and multiple tables. Caching minimizes object creation (and removes potential thread issues, however unlikely).

2. testInvalidFormat_MissingEndBracket has fail("IllegalArgumentException must be thrown when input is null."); correct the fail message.

3. Current implementation has a potential milliseconds time difference across columns/rows/tables when the column data does not specify time because LocalDateTime.now(clock) is not cached, it is instantiated for each use (each column). We need to provide consistency across the test run. Should probably cache LocalDate.now(clock) as well to handle the edge case of running at 23:59:59.5!

[Iwao AVE!]

Thank you for reviewing this, Jeff!

I like this approach better because it is simpler and does not require extra preparation.
The other approach may be more flexible, but that level of flexibility is rarely needed, I would think.

I also agree with the points you have raised and plan to add new commits within a day or two.

[Iwao AVE!]

I've committed the changes.

An instance of RelativeDateTimeParser is cached in each data type.
For consistency, the same fixed Clock instance is used in all calculations.

The Clock is passed via constructors mainly for better testability.
If modifying constructors is not appropriate, please let me know and I'll make adjustments.

I'll update the doc (datatypes.xml) maybe tomorrow!

[Jeff Jensen]

Thank you for the updates. Some more thoughts:

1. RelativeDateTimeParser:

   • I'm glad you changed back from static; prefer to avoid static, especially when data involved.
   • I think it should have no instance variables too; pass Clock in parse(), same as used to pass LocalDateTime.

2. Please keep existing no-args constructors otherwise risk backwards compatibility problems for users of the respective class. Use defaults in those constructors and call new constructors specifying them. They could use CLOCK in parent and not have a constructor arg (each already has dependency to it via inheritance).

3. We need ability to share same now date/time instance between RelativeDateTimeParser and ReplacementDataSet setup so is to the same nanoseconds; so need way to externalize/get/set that date/time. Can have multiple constructors and chain them: one no args and makes a LocalDateTime or an Instant, one taking LocalDateTime or an Instant & makes a Clock, one taking Clock.

4. Another approach is to keep Clock in RelativeDateTimeParser and not expose to DataTypes, and share the same RelativeDateTimeParser instance where needed (instance in DataType or make it a Singleton). I think this may be the preferred approach as it lessens the proliferation of RelativeDateTimeParser's needs. Allow getting/setting the date/time source to enable the shared source instance with ReplacementDataSet setup.

5. Please format commit messages per git norms; this is a good reference: https://wiki.openstack.org/wiki/GitCommitMessages
   No need to adjust them now, just wait until we're done and squash them with an adjusted message. If you want, can squash now before making further adjustments since I have seen the diffs to now.

[Iwao AVE!]

Makes sense. :)

I took the approach you mentioned in 4.
DataType.RELATIVE_DATE_TIME_PARSER is the public shared instance of the parser.

To allow users to setup ReplacementDataSet, getClock() and setClock() is added to the parser (I am not so sure about the necessity of the setter, but if you think it's worth making the class mutable, I have no objection).
In case the class needs to be thread-safe, I would probably just make the setter/getter synchronized.

I believe the point 1, 2 and 3 are resolved as well.
Regarding the point 5, do you mean I should squash all commits into a single commit and force-push?
If so, I'll do it when the review process is completed.

[Jeff Jensen]

I took the approach you mentioned in 4.

Thank you.

In case the class needs to be thread-safe

Typically the needs for this (having same source across class configs, such as an Instant) is only at setup/config time, not during (parallel) execution.

Regarding the point 5, do you mean I should squash all commits into a single commit and force-push?

Yes. Unless you have more planned changes, please do so now for final review ease.

[Jeff Jensen]

As part of finalizing the MR, please:

1. Create a feature tracker entry describing this as a proposed change (as if haven't implemented it yet), unless you find an existing one(s) that works. I don't think [#138] or [#152] work for this because they are for changing ReplacementDataSet.
2. Add an entry to changes.xml for this (let me know if need help on that; dev is me, type is add, due-to is you; add terse summary to end of description).
3. Prefix commit message with "Fnnn: ", where nnn is the feature number.

[Iwao AVE!]

I rebased the branch onto master and squashed all commits (tests are passing).
[feature-requests:#224] is the new ticket.

I'm not so confident about the commit message and changes.xml, though.
Please let me know if there's any correction needed to be made!

[Jeff Jensen]

Thank you for the updates! Looking very good...

A few more questions:

1. *DataType class changes all have if (stringValue.charAt(0) == '[') block:
   • Interesting it delegates "now" to the RE (handles case sensitivity more easily!).
   • Since the if-block logic is same across all three classes, should we extract and share it?
2. RelativeDateTimeParser:
   • Are there any tweaks we can do to help future understanding and maintenance?
     For example, could create constants for things like the matcher group number, matcher.group(N), more readily knowing which section it refers to.
3. RelativeDateTimeParserTest:
   • Good test coverage, thank you!
   • Has tests for null/empty input and the others test when "now" present. Do we need tests for when "now" not specified and diff and/or time values specified? What should happen when "now" keyword not specified?
   • Should input validation specifically check for "now" and have specific error message for not specified for diagnosis clarity?

[Iwao AVE!]

Thank you for the thorough review!

> 1

BLOB has similar extended syntax (i.e. [TEXT], [FILE], etc.).
So, if you are comfortable with making a rule like "extended data type syntax always starts with '['", it would make sense to extract this check to a method (e.g. isExtendedSyntax(String)) in DataType.
It could be useful even in the existing ByteDataType#typeCast to save the relatively expensive regex operation.

If, on the other hand, you consider this check to be specific to this relative date/time enhancement, extracting it to a static method in RelativeDateTimeParser, maybe?

> 2

Good point. I'll add constants GROUP_DIFFS and GROUP_TIME.

> 3

I'll add a test for the missing 'now' case.
From the parser's point of view, however, it's just another 'does not match the pattern' error and specifically checking 'now' seems a little bit redundant.
For clarity, how about improving the exception message instead?
e.g.

'[+1d]' does not match the expected pattern [now{diff}{time}]. Please see the data types documentation for the details. http://dbunit.sourceforge.net/datatypes.html#relativedatetime

Prefix validation may make sense if another extended syntax which has a different prefix is added in the future (it'll be done in the data types, probably).

---

Please let me know what you think about 1 and 3.

[Jeff Jensen]

1: sounds good to me. You still agree?
And refactoring to DataType is preferred to centralize for all, agreed? If so, please make that refactoring a second commit, not with your new change (either before your new change or after, your pref).

3: Thanks for adding the tests. Yeah, the parser catches that but it's not necessarily clear for the developer-user. Yes, improving the exception message is great, makes the expected format very clear.

[Iwao AVE!]

And refactoring to DataType is preferred to centralize for all, agreed?

Yes. :)

I have updated the branch.
Please let me know if you want me to squash some commits after review.

[Jeff Jensen]

Thank you again! Looks great.

You can squash them all. If you were to include refactoring the other spots to use isExtendedSyntax, then keep the last commit separate with them all.

Please remove the period from end of git topic in commit message.

[Iwao AVE!]

I squashed all commits, then added an extra commit that adds the quick check to ByteDataType#typeCast().

Regarding the extra commit, it seemed to be better to do it separately rather than as a refactoring as there was no comparable check before,
I also didn't apply the formatter because it wasn't applied previously.

[Jeff Jensen]

Very good, thank you.
Anything else or is it merge time?

[Iwao AVE!]

It looks good to me. :)
If I noticed anything, I'll open a new ticket or post message to the mailing list.
Thank you again for helping me with the request!