toToday doesn't work over DST changes when created from DT Item State

rkoshak commented 7 months ago

Expected Behavior

A ZonedDateTime of HH:MM:SS retains the same HH:MM:SS after DST changes when calling toToday on an Item's state.

Current Behavior

Given time.toZDT(i.state).toToday(), if the state of the Item is before the DST change, the result of the call to toToday retains the timezone from before the DST change resulting in a time that is one hour off.

For example, our DST change was on the 5th of this month. Given the following code:

// My current zone is -7, and we just moved from -6
var now = time.toZDT();
var beforeDSTchange = time.toZDT(now.minusDays(4).toString());
//items.TestDateTime.postUpdate(beforeDSTchange);
//Java.type('java.lang.Thread').sleep(500);
console.info('Now:        ' + now);
console.info('Before DST: ' + beforeDSTchange);
console.info('Item:       ' + items.TestDateTime.state);
console.info('To Today:   ' + time.toZDT(items.TestDateTime.state).toToday());

The result is:

2023-11-07 14:06:02.740 [INFO ] [nhab.automation.script.ui.scratchpad] - Now:        2023-11-07T14:06:02.229-07:00[SYSTEM]
2023-11-07 14:06:02.741 [INFO ] [nhab.automation.script.ui.scratchpad] - Before DST: 2023-11-03T14:06:02.229-06:00[SYSTEM]
2023-11-07 14:06:02.741 [INFO ] [nhab.automation.script.ui.scratchpad] - Item:       2023-11-03T14:02:04.274-0600
2023-11-07 14:06:02.744 [INFO ] [nhab.automation.script.ui.scratchpad] - To Today:   2023-11-07T14:02:04.274-06:00

Note that this is only a problem when converting the Item's state to a ZonedDateTime first. If creating the ZonedDateTime myself it works as expected. Given

// My current zone is -7, and we just moved from -6
var now = time.toZDT();
var beforeDSTchange = now.minusDays(4);
console.info('Now:        ' + now);
console.info('Before DST: ' + beforeDSTchange);
console.info('To Today:   ' + beforeDSTchange.toToday());

The result is:

2023-11-07 14:14:36.546 [INFO ] [nhab.automation.script.ui.scratchpad] - Now:        2023-11-07T14:14:36.544-07:00[SYSTEM]
2023-11-07 14:14:36.546 [INFO ] [nhab.automation.script.ui.scratchpad] - Before DST: 2023-11-03T14:14:36.544-06:00[SYSTEM]
2023-11-07 14:14:36.548 [INFO ] [nhab.automation.script.ui.scratchpad] - To Today:   2023-11-07T14:14:36.544-07:00[SYSTEM]

This works, which is probably why this bug has been missed until now.

The difference seems to be the [SYSTEM]. I notice that after parsing the state of the DateTime Item the ZDT only has the offset, and doesn't have the zone ID. I strongly suspect this is the root of the problem. Without the zone ID, Joda doesn't know that DST has occurred so it leaves the TZ alone.

Possible Solution(s)

I'm not sure of the full implication but the first thing I would try (and will try when I get a chance) is to add the zone ID when parsing the full ISO8601 String we get back from the DateTime Item, changing line 99 of time.js from

    case REGEX.ISO_8160_FULL.test(isoStr): return time.ZonedDateTime.parse(isoStr);

(is that a typo? 8160 is a standard having to do with yarn) to

case REGEX.ISO_8160_FULL.test(isoStr): return time.ZonedDateTime.parse(isoStr).withZoneSameLocal(time.ZoneID.systemDefault());

That should give us the ZoneID which will allow toToday() to work after parsing the ISO string from the DateTime Item.

Alternatively, we could modify Item to add "[SYSTEM]" to the ISO8601 String we get from the this.rawState.toString() for DateTimeTypes. Kind of the opposite side of the coin from _toOpenhabString().

I've verified appending "[SYSTEM]" to the ISO8601 string to work.

// My current zone is -7, and we just moved from -6
var now = time.toZDT();
var beforeDSTchange = time.toZDT(now.minusDays(4).toString());
console.info('Now:        ' + now);
console.info('Before DST: ' + beforeDSTchange);
console.info('Item:       ' + items.TestDateTime.state);
console.info('To Today:   ' + time.toZDT(items.TestDateTime.state+"[SYSTEM]").toToday());

2023-11-07 14:37:00.870 [INFO ] [nhab.automation.script.ui.scratchpad] - Now:        2023-11-07T14:37:00.863-07:00[SYSTEM]
2023-11-07 14:37:00.870 [INFO ] [nhab.automation.script.ui.scratchpad] - Before DST: 2023-11-03T14:37:00.863-06:00[SYSTEM]
2023-11-07 14:37:00.871 [INFO ] [nhab.automation.script.ui.scratchpad] - Item:       2023-11-03T14:02:04.274-0600
2023-11-07 14:37:00.874 [INFO ] [nhab.automation.script.ui.scratchpad] - To Today:   2023-11-07T14:02:04.274-07:00[SYSTEM]

The least intrusive approach would be to just append the ZoneId to the ZDT inside toToday(). But it doesn't feel right to muck around with the ZoneID inside a ZDT that doesn't have a ZoneID. It would be better to append the ZoneID before we get to this point so that there is still the option to have a ZDT without a ZDT and have toToday work.

time.ZonedDateTime.prototype.toToday = function () {
  const now = time.ZonedDateTime.now();
  return this.withYear(now.year())
    .withMonth(now.month())
    .withDayOfMonth(now.dayOfMonth()
    .withZoneSameLocal(time.ZoneId.systemDefault());
};

I've also confirmed this to work.

I'll gladly file a PR but would like some advice as to which approach is the preferred. There might be side effects which I do not know.

My inclination would be to fix this as close to the source as possible so that would be either in the ISO8601 parser or the Item.

Steps to Reproduce (for Bugs)

See above

Context

This was reported on my Time State Machine rule template and I experienced the problem myself.

Your Environment

Version used: (e.g., openHAB and JS Scripting add-on version)
Environment name and version (e.g. Chrome 76, Java 8, Node.js 12.9, ...):
Operating System and version (desktop or mobile, Windows 10, Raspbian Buster, ...): OH 4.1.0 M3 openhab-js 4.6

florian-h05 commented 7 months ago

Given that for all other ISO partings the time zone is added, I am in favour of solution 1:

I'm not sure of the full implication but the first thing I would try (and will try when I get a chance) is to add the zone ID when parsing the full ISO8601 String we get back from the DateTime Item, changing line 99 of time.js from

is that a typo? 8160 is a standard having to do with yarn

Yes, this is a typo. Not sure why I should have reference such an ISO, so it should be ISO 8601.

The only question remaining is, what’s the difference between time.ZoneId.SYSTEM and time.ZoneId.systemDefault()?

rkoshak commented 7 months ago

The only question remaining is, what’s the difference between time.ZoneId.SYSTEM and time.ZoneId.systemDefault()?

From what I can tell, nothing. Both resolve to SYSTEM_DEFAULT_ZONE_ID_INSTANCE in the ZoneIdFactory.

I'm not sure why they have the method. As far as I can tell they are the same. See https://js-joda.github.io/js-joda/file/packages/core/src/ZoneIdFactory.js.html#errorLines=199 and https://js-joda.github.io/js-joda/file/packages/core/src/ZoneIdFactory.js.html#errorLines=34,36

I'll submit a PR later today with option 1. Should I fix the typo here or create a separate PR?

florian-h05 commented 7 months ago

From what I can tell, nothing. Both resolve to SYSTEM_DEFAULT_ZONE_ID_INSTANCE in the ZoneIdFactory.

I wonder how I came to the idea to use .SYSTEM, I can't find it in the docs right now ... Since they are both the same, I would say let's continue to use .SYSTEM for consistency.

I'll submit a PR later today with option 1. Should I fix the typo here or create a separate PR?

:+1: You can fix the typo in that PR, no need to create an extra PR for that. Since I am on the other side of the atlantic, I will review that PR tomorrow ;-)

openhab / openhab-js