From c7e705cf18ee6fb484bf42c00b3c01e87631a1f5 Mon Sep 17 00:00:00 2001 From: Wouter Born Date: Sat, 24 Oct 2020 18:10:46 +0200 Subject: [PATCH] [mapdb] Fixes and improvements (#8852) * Fix index out of bounds when persisting empty StringType values * Fix deserialization when strings contain type separator * Improve debug logging * Improve test coverage Fixes #8790 Signed-off-by: Wouter Born --- .../internal/MapDbPersistenceService.java | 20 +++---- .../mapdb/internal/StateTypeAdapter.java | 14 +++-- .../mapdb/StateTypeAdapterTest.java | 60 +++++++++++++++---- 3 files changed, 66 insertions(+), 28 deletions(-) diff --git a/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/MapDbPersistenceService.java b/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/MapDbPersistenceService.java index 5e09943cfb8f..d78381c9bef1 100644 --- a/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/MapDbPersistenceService.java +++ b/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/MapDbPersistenceService.java @@ -13,7 +13,6 @@ package org.openhab.persistence.mapdb.internal; import java.io.File; -import java.util.Collections; import java.util.Date; import java.util.List; import java.util.Locale; @@ -143,20 +142,19 @@ public void store(Item item, @Nullable String alias) { String json = serialize(mItem); map.put(localAlias, json); commit(); - logger.debug("Stored '{}' with state '{}' in MapDB database", localAlias, state.toString()); + if (logger.isDebugEnabled()) { + logger.debug("Stored '{}' with state '{}' as '{}' in MapDB database", localAlias, state, json); + } } @Override public Iterable query(FilterCriteria filter) { String json = map.get(filter.getItemName()); if (json == null) { - return Collections.emptyList(); + return List.of(); } Optional item = deserialize(json); - if (!item.isPresent()) { - return Collections.emptyList(); - } - return Collections.singletonList(item.get()); + return item.isPresent() ? List.of(item.get()) : List.of(); } private String serialize(MapDbItem item) { @@ -169,7 +167,10 @@ private Optional deserialize(String json) { if (item == null || !item.isValid()) { logger.warn("Deserialized invalid item: {}", item); return Optional.empty(); + } else if (logger.isDebugEnabled()) { + logger.debug("Deserialized '{}' with state '{}' from '{}'", item.getName(), item.getState(), json); } + return Optional.of(item); } @@ -178,10 +179,7 @@ private void commit() { } private static Stream streamOptional(Optional opt) { - if (!opt.isPresent()) { - return Stream.empty(); - } - return Stream.of(opt.get()); + return opt.isPresent() ? Stream.of(opt.get()) : Stream.empty(); } @Override diff --git a/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/StateTypeAdapter.java b/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/StateTypeAdapter.java index eadd8469ddf0..5d1866974754 100644 --- a/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/StateTypeAdapter.java +++ b/bundles/org.openhab.persistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/StateTypeAdapter.java @@ -13,7 +13,6 @@ package org.openhab.persistence.mapdb.internal; import java.io.IOException; -import java.util.Collections; import java.util.List; import org.openhab.core.types.State; @@ -45,14 +44,17 @@ public State read(JsonReader reader) throws IOException { String value = reader.nextString(); try { - String[] parts = value.split(TYPE_SEPARATOR); - String valueTypeName = parts[0]; - String valueAsString = parts[1]; + int index = value.indexOf(TYPE_SEPARATOR); + if (index == -1) { + logger.warn("Couldn't deserialize state '{}': type separator '{}' not found", value, TYPE_SEPARATOR); + return null; + } + String valueTypeName = value.substring(0, index); + String valueAsString = value.substring(index + TYPE_SEPARATOR.length()); @SuppressWarnings("unchecked") Class valueType = (Class) Class.forName(valueTypeName); - List> types = Collections.singletonList(valueType); - return TypeParser.parseState(types, valueAsString); + return TypeParser.parseState(List.of(valueType), valueAsString); } catch (Exception e) { logger.warn("Couldn't deserialize state '{}': {}", value, e.getMessage()); } diff --git a/bundles/org.openhab.persistence.mapdb/src/test/java/org/openhab/persistence/mapdb/StateTypeAdapterTest.java b/bundles/org.openhab.persistence.mapdb/src/test/java/org/openhab/persistence/mapdb/StateTypeAdapterTest.java index 225b1aad68eb..bf195dfde055 100644 --- a/bundles/org.openhab.persistence.mapdb/src/test/java/org/openhab/persistence/mapdb/StateTypeAdapterTest.java +++ b/bundles/org.openhab.persistence.mapdb/src/test/java/org/openhab/persistence/mapdb/StateTypeAdapterTest.java @@ -15,11 +15,23 @@ import static org.hamcrest.CoreMatchers.*; import static org.hamcrest.MatcherAssert.assertThat; -import org.junit.jupiter.api.Test; +import java.math.BigDecimal; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.eclipse.jdt.annotation.NonNullByDefault; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import org.openhab.core.library.types.DecimalType; import org.openhab.core.library.types.HSBType; import org.openhab.core.library.types.OnOffType; import org.openhab.core.library.types.PercentType; +import org.openhab.core.library.types.QuantityType; import org.openhab.core.library.types.StringType; +import org.openhab.core.library.unit.ImperialUnits; +import org.openhab.core.library.unit.SIUnits; +import org.openhab.core.library.unit.SmartHomeUnits; import org.openhab.core.types.State; import org.openhab.persistence.mapdb.internal.StateTypeAdapter; @@ -30,18 +42,44 @@ * * @author Martin Kühl - Initial contribution */ +@NonNullByDefault public class StateTypeAdapterTest { - Gson mapper = new GsonBuilder().registerTypeHierarchyAdapter(State.class, new StateTypeAdapter()).create(); - - @Test - public void readWriteRoundtripShouldRecreateTheWrittenState() { - assertThat(roundtrip(OnOffType.ON), is(equalTo(OnOffType.ON))); - assertThat(roundtrip(PercentType.HUNDRED), is(equalTo(PercentType.HUNDRED))); - assertThat(roundtrip(HSBType.GREEN), is(equalTo(HSBType.GREEN))); - assertThat(roundtrip(StringType.valueOf("test")), is(equalTo(StringType.valueOf("test")))); + private Gson mapper = new GsonBuilder().registerTypeHierarchyAdapter(State.class, new StateTypeAdapter()).create(); + + private static final List DECIMAL_TYPE_VALUES = List.of(DecimalType.ZERO, new DecimalType(1.123), + new DecimalType(10000000)); + + private static final List HSB_TYPE_VALUES = List.of(HSBType.BLACK, HSBType.GREEN, HSBType.WHITE, + HSBType.fromRGB(1, 2, 3), HSBType.fromRGB(11, 22, 33), HSBType.fromRGB(0, 0, 255)); + + private static final List ON_OFF_TYPE_VALUES = List.of(OnOffType.ON, OnOffType.OFF); + + private static final List PERCENT_TYPE_VALUES = List.of(PercentType.ZERO, PercentType.HUNDRED, + PercentType.valueOf("0.0000001"), PercentType.valueOf("12"), PercentType.valueOf("99.999")); + + private static final List> QUANTITY_TYPE_VALUES = List.of(QuantityType.valueOf("0 W"), + QuantityType.valueOf("1 kW"), QuantityType.valueOf(20, SmartHomeUnits.AMPERE), + new QuantityType<>(new BigDecimal("21.23"), SIUnits.CELSIUS), + new QuantityType<>(new BigDecimal("75"), ImperialUnits.MILES_PER_HOUR), + QuantityType.valueOf(1000, SmartHomeUnits.KELVIN), + QuantityType.valueOf(100, SmartHomeUnits.METRE_PER_SQUARE_SECOND)); + + private static final List STRING_TYPE_VALUES = List.of(StringType.valueOf("test"), + StringType.valueOf("a b c 1 2 3"), StringType.valueOf(""), StringType.valueOf("@@@### @@@")); + + private static final List VALUES = Stream.of(DECIMAL_TYPE_VALUES, HSB_TYPE_VALUES, ON_OFF_TYPE_VALUES, + PERCENT_TYPE_VALUES, QUANTITY_TYPE_VALUES, STRING_TYPE_VALUES).flatMap(list -> list.stream()) + .collect(Collectors.toList()); + + @ParameterizedTest + @MethodSource + public void readWriteRoundtripShouldRecreateTheWrittenState(State state) { + String json = mapper.toJson(state); + State actual = mapper.fromJson(json, State.class); + assertThat(actual, is(equalTo(state))); } - private State roundtrip(State state) { - return mapper.fromJson(mapper.toJson(state), State.class); + public static Stream readWriteRoundtripShouldRecreateTheWrittenState() { + return VALUES.stream(); } }