[discovery] Fix Instant serialization/deserialization regression

[jlaur]

Fix regression from #4026

See https://github.com/openhab/openhab-core/pull/4026/files#r1445323983

[jlaur]

@J-N-K - this supports:

backwards compatibility:

      "timestamp": 1699728096882,

current:

      "timestamp": "2024-01-09T20:17:27.818829400Z",

But doesn't support:

      "timestamp": {
        "seconds": 1704747106,
        "nanos": 550136800
      },

which by mistake was the result of serialization since yesterday's snapshot. Do you think we need to support this as well? If not, inbox results produced since yesterday cannot be parsed and will need to be manually removed from the JSON file. OTOH, this would introduce some technical debt for supporting a few days snapshot issues. WDYT?

[J-N-K]

Isn't this the standard serializer?

[jlaur]

By default this would be produced (in Java 17):

"timestamp": {
  "seconds": 1704747106,
  "nanos": 550136800
}

Since Java 17 JDK internals are strongly encapsulated so this is from using a reflection-based type adapter, i.e. we would be relying on implementation details outside of our control.

[J-N-K]

Ok. I agree. But why don't we use Instant.toEpochMilli und Instant.fromEpochMilli? This is immediately backward compatible.

[jlaur]

Do you mean in the type adapter while still keeping Instant in the DTO:

openhab-core/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/internal/DiscoveryResultImpl.java

Line 46 in 6b2182d

private Instant timestamp = Instant.MIN;

?

We could, but the only advantage IMO would be that the deserializer is a bit simpler because it doesn't need to support both old and new format. Over time, the code for converting the old format will be executed less and less since the new format is parsed first, so in terms of performance it shouldn't be a big deal.

The disadvantage would be still keeping epoch milliseconds which are less readable when inspecting the JSON file.

In both cases we need to parse a string, so the epoch version of the deserializer would still need to be:

        try {
            return Instant.ofEpochMilli(Long.parseLong(element.getAsString()));
        } catch (NumberFormatException e) {
            throw new JsonParseException("Could not parse as Instant: " + content, e);
        }

(like in the fallback in the current implementation)

[jlaur]

Writing that made me wonder about other possibilities. so just pushed a simplification:

return Instant.ofEpochMilli(element.getAsLong());

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/latest-snapshot-error/152934/2