Scripting: Switch watcher to use joda bwc time objects

[rjernst]

This commit converts the watcher execution context to use the joda
compat java time objects. It also again removes the joda methods from
the painless whitelist.

[elasticmachine]

Pinging @elastic/es-core-features

[elasticmachine]

Pinging @elastic/es-core-infra

[nik9000]

Makes sense to me but I'm not a watcher expert.

[nik9000]

but the hamcrest matcher doesn't exist

[spinscale]

Judging from a quick peek into JodaCompatibleZonedDateTime, it seems that its equals methods is the culprit not the ZonedDateTime? I think this snippet would fail

JodaCompatibleZonedDateTime d = new JodaCompatibleZonedDateTime(Instant.now(), ZoneOffset.UTC);
assertThat(d.equals(d), is(true));

not sure this is intentional?

[rjernst]

It is semi-intentional, in that it just compares the underlying ZonedDateTime instances. To compare times, apparently isEqual is the thing to use...

[spinscale]

I left one question regarding the equals methods, I think this is also the reason for some of the test failures.

[spinscale]

Judging from a quick peek into JodaCompatibleZonedDateTime, it seems that its equals methods is the culprit not the ZonedDateTime? I think this snippet would fail

JodaCompatibleZonedDateTime d = new JodaCompatibleZonedDateTime(Instant.now(), ZoneOffset.UTC);
assertThat(d.equals(d), is(true));

not sure this is intentional?

[spinscale]

this commit fixes the ScriptConditionTests spinscale@3af5140

[rjernst]

@elasticmachine test this please

[rjernst]

@elasticmachine test this please

[spinscale]

the remaining tests fail because of a different default formatting of the toString() method in mustache of ZonedDateTime, which is used in JodaCompatibleZonedDateTime.toString(). One could fix this like

new JodaCompatibleZonedDateTime(Instant.ofEpochMilli(scheduledTime.getMillis()), ZoneOffset.UTC){
                @Override
                public String toString() {
                    return DateFormatters.forPattern("strict_date_time").format(getZonedDateTime());
                }
            });

on a per case base. But I think it makes sense to just put this into the toString() method of JodaCompatibleZonedDateTime or allow to pass a date formatter?

[rjernst]

This is an interesting problem. I had not thought about the toString() behavior. I had thought they were the same between joda/java. I guess in this case the upgrade to 7.0 will have to be hard break? Most of the time it is the same?

[spinscale]

AbstractDateTime.toString() used by DateTime uses Output the date time in ISO8601 format (yyyy-MM-ddTHH:mm:ss.SSSZZ). according to its java docs. So I think it is fine to go with that format for the JodaCompatibleZonedDateTime. The java time formatting is more lenient by default by omitting some zero based values (and interestingly not all of them, I have not understood the intentation behind that to be honest).

[spinscale]

I tried what I mentioned above and added a formatter to the toString() method. looks good during my test runs

[rjernst]

Thanks @spinscale. I modified it as you suggested, but I do want to note the behavior would still change in 7.0 since the joda compatible object would disappear in favor of ZonedDateTime. It's probably fine, as anyone relying on whether nano seconds exist in a tostring inside a script are asking for trouble, but we should probably note this in migration docs when removing the compat object in master.

[spinscale]

mentioning in the logs sounds good to me!