[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 <github@maindrain.net>

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 class MapDbPersistenceService implements QueryablePersistenceService {
         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<HistoricItem> query(FilterCriteria filter) {
         String json = map.get(filter.getItemName());
         if (json == null) {
-            return Collections.emptyList();
+            return List.of();
         }
         Optional<MapDbItem> item = deserialize(json);
-        if (!item.isPresent()) {
+        return item.isPresent() ? List.of(item.get()) : List.of();
-            return Collections.emptyList();
-        }
-        return Collections.singletonList(item.get());
     }
 
     private String serialize(MapDbItem item) {
@@ -169,7 +167,10 @@ public class MapDbPersistenceService implements QueryablePersistenceService {
         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 @@ public class MapDbPersistenceService implements QueryablePersistenceService {
     }
 
     private static <T> Stream<T> streamOptional(Optional<T> opt) {
-        if (!opt.isPresent()) {
+        return opt.isPresent() ? Stream.of(opt.get()) : Stream.empty();
-            return Stream.empty();
-        }
-        return Stream.of(opt.get());
     }
 
     @Override

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 class StateTypeAdapter extends TypeAdapter<State> {
         String value = reader.nextString();
 
         try {
-            String[] parts = value.split(TYPE_SEPARATOR);
+            int index = value.indexOf(TYPE_SEPARATOR);
-            String valueTypeName = parts[0];
+            if (index == -1) {
-            String valueAsString = parts[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<? extends State> valueType = (Class<? extends State>) Class.forName(valueTypeName);
-            List<Class<? extends State>> types = Collections.singletonList(valueType);
+            return TypeParser.parseState(List.of(valueType), valueAsString);
-            return TypeParser.parseState(types, valueAsString);
         } catch (Exception e) {
             logger.warn("Couldn't deserialize state '{}': {}", value, e.getMessage());
         }

bundles/org.openhab.persistence.mapdb/src/test/java/org/openhab/persistence/mapdb/StateTypeAdapterTest.java

@@ -15,11 +15,23 @@ package org.openhab.persistence.mapdb;
 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 @@ import com.google.gson.GsonBuilder;
  *
  * @author Martin Kühl - Initial contribution
  */
+@NonNullByDefault
 public class StateTypeAdapterTest {
-    Gson mapper = new GsonBuilder().registerTypeHierarchyAdapter(State.class, new StateTypeAdapter()).create();
+    private Gson mapper = new GsonBuilder().registerTypeHierarchyAdapter(State.class, new StateTypeAdapter()).create();
 
-    @Test
+    private static final List<DecimalType> DECIMAL_TYPE_VALUES = List.of(DecimalType.ZERO, new DecimalType(1.123),
-    public void readWriteRoundtripShouldRecreateTheWrittenState() {
+            new DecimalType(10000000));
-        assertThat(roundtrip(OnOffType.ON), is(equalTo(OnOffType.ON)));
+
-        assertThat(roundtrip(PercentType.HUNDRED), is(equalTo(PercentType.HUNDRED)));
+    private static final List<HSBType> HSB_TYPE_VALUES = List.of(HSBType.BLACK, HSBType.GREEN, HSBType.WHITE,
-        assertThat(roundtrip(HSBType.GREEN), is(equalTo(HSBType.GREEN)));
+            HSBType.fromRGB(1, 2, 3), HSBType.fromRGB(11, 22, 33), HSBType.fromRGB(0, 0, 255));
-        assertThat(roundtrip(StringType.valueOf("test")), is(equalTo(StringType.valueOf("test"))));
+
+    private static final List<OnOffType> ON_OFF_TYPE_VALUES = List.of(OnOffType.ON, OnOffType.OFF);
+
+    private static final List<PercentType> PERCENT_TYPE_VALUES = List.of(PercentType.ZERO, PercentType.HUNDRED,
+            PercentType.valueOf("0.0000001"), PercentType.valueOf("12"), PercentType.valueOf("99.999"));
+
+    private static final List<QuantityType<?>> 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<StringType> STRING_TYPE_VALUES = List.of(StringType.valueOf("test"),
+            StringType.valueOf("a b c 1 2 3"), StringType.valueOf(""), StringType.valueOf("@@@###   @@@"));
+
+    private static final List<State> 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) {
+    public static Stream<State> readWriteRoundtripShouldRecreateTheWrittenState() {
-        return mapper.fromJson(mapper.toJson(state), State.class);
+        return VALUES.stream();
     }
 }