Skip to content

Commit

Permalink
[mapdb] Fixes and improvements (openhab#8852)
Browse files Browse the repository at this point in the history 
* Fix index out of bounds when persisting empty StringType values
* Fix deserialization when strings contain type separator
* Improve debug logging
* Improve test coverage

Fixes openhab#8790

Signed-off-by: Wouter Born <github@maindrain.net>
  • Loading branch information
@wborn
wborn committed Oct 24, 2020
1 parent baaea5f commit c7e705c
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 28 deletions.
20 changes: 9 additions & 11 deletions ...e.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/MapDbPersistenceService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<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 Collections.emptyList();
}
return Collections.singletonList(item.get());
return item.isPresent() ? List.of(item.get()) : List.of();
}

private String serialize(MapDbItem item) {
Expand All @@ -169,7 +167,10 @@ private Optional<MapDbItem> 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);
}

Expand All @@ -178,10 +179,7 @@ private void commit() {
}

private static <T> Stream<T> streamOptional(Optional<T> opt) {
if (!opt.isPresent()) {
return Stream.empty();
}
return Stream.of(opt.get());
return opt.isPresent() ? Stream.of(opt.get()) : Stream.empty();
}

@Override
Expand Down
14 changes: 8 additions & 6 deletions ...sistence.mapdb/src/main/java/org/openhab/persistence/mapdb/internal/StateTypeAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<? extends State> valueType = (Class<? extends State>) Class.forName(valueTypeName);
List<Class<? extends State>> 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());
}
Expand Down
60 changes: 49 additions & 11 deletions ...b.persistence.mapdb/src/test/java/org/openhab/persistence/mapdb/StateTypeAdapterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<DecimalType> DECIMAL_TYPE_VALUES = List.of(DecimalType.ZERO, new DecimalType(1.123),
new DecimalType(10000000));

private static final List<HSBType> 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<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) {
return mapper.fromJson(mapper.toJson(state), State.class);
public static Stream<State> readWriteRoundtripShouldRecreateTheWrittenState() {
return VALUES.stream();
}
}

0 comments on commit c7e705c

Please sign in to comment.