Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[discovery] Fix Instant serialization/deserialization regression #4029

Merged
merged 3 commits into from
Jan 12, 2024
Merged

[discovery] Fix Instant serialization/deserialization regression #4029

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a771ee7
Fix Instant serialization/deserialization regression
jlaur Jan 9, 2024
01536b3
Consolidate serialization and deserialization in same type adapter
jlaur Jan 10, 2024
89863aa
Simplify deserializer
jlaur Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 50 additions & 0 deletions ...torage.json/src/main/java/org/openhab/core/storage/json/internal/InstantDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright (c) 2010-2024 Contributors to the openHAB project
*
* See the NOTICE file(s) distributed with this work for additional
* information.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.openhab.core.storage.json.internal;

import java.lang.reflect.Type;
import java.time.Instant;
import java.time.format.DateTimeParseException;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;

import com.google.gson.JsonDeserializationContext;
import com.google.gson.JsonDeserializer;
import com.google.gson.JsonElement;
import com.google.gson.JsonParseException;

/**
* The {@link InstantDeserializer} converts a formatted UTC string to {@link Instant}.
* As fallback, milliseconds since epoch is supported as well.
*
* @author Jacob Laursen - Initial contribution
*/
@NonNullByDefault
public class InstantDeserializer implements JsonDeserializer<Instant> {
@Override
public @Nullable Instant deserialize(JsonElement element, Type arg1, JsonDeserializationContext arg2)
throws JsonParseException {
String content = element.getAsString();
try {
return Instant.parse(content);
} catch (DateTimeParseException e) {
// Fallback to milliseconds since epoch for backwards compatibility.
}
try {
return Instant.ofEpochMilli(Long.parseLong(content));
} catch (NumberFormatException e) {
throw new JsonParseException("Could not parse as Instant: " + content, e);
}
}
}
36 changes: 36 additions & 0 deletions ....storage.json/src/main/java/org/openhab/core/storage/json/internal/InstantSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* Copyright (c) 2010-2024 Contributors to the openHAB project
*
* See the NOTICE file(s) distributed with this work for additional
* information.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.openhab.core.storage.json.internal;

import java.lang.reflect.Type;
import java.time.Instant;

import org.eclipse.jdt.annotation.NonNullByDefault;

import com.google.gson.JsonElement;
import com.google.gson.JsonPrimitive;
import com.google.gson.JsonSerializationContext;
import com.google.gson.JsonSerializer;

/**
* The {@link InstantSerializer} converts an {@link Instant} to a formatted UTC string.
*
* @author Jacob Laursen - Initial contribution
*/
@NonNullByDefault
public class InstantSerializer implements JsonSerializer<Instant> {
@Override
public JsonElement serialize(Instant instant, Type typeOfSrc, JsonSerializationContext context) {
return new JsonPrimitive(instant.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the standard serializer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jlaur jlaur Jan 10, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}
}
3 changes: 3 additions & 0 deletions ...b.core.storage.json/src/main/java/org/openhab/core/storage/json/internal/JsonStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.io.FileOutputStream;
import java.io.FileReader;
import java.io.IOException;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -112,6 +113,8 @@ public JsonStorage(File file, @Nullable ClassLoader classLoader, int maxBackupFi
.registerTypeHierarchyAdapter(Map.class, new OrderingMapSerializer())//
.registerTypeHierarchyAdapter(Set.class, new OrderingSetSerializer())//
.registerTypeAdapter(Configuration.class, new ConfigurationDeserializer()) //
.registerTypeAdapter(Instant.class, new InstantSerializer()) //
.registerTypeAdapter(Instant.class, new InstantDeserializer()) //
.setPrettyPrinting() //
.create();

Expand Down