Skip to content

Commit

Permalink
[boschshc] Fix NPE during deserialization, make long polling more rob…
Browse files Browse the repository at this point in the history 
…ust (openhab#17190)

* Fix NPE while deserializing service data JSON objects
* Catch exceptions while handling long poll results
* Correct @type name for user-defined states
* Add unit tests and enhance existing unit tests

Signed-off-by: David Pace <dev@davidpace.de>
  • Loading branch information
@david-pace @digitaldan
david-pace authored and digitaldan committed Aug 29, 2024
1 parent 050f02c commit ac0d70a
Show file tree
Hide file tree
Showing 7 changed files with 167 additions and 48 deletions.
6 changes: 5 additions & 1 deletion ...chshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/LongPolling.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ private void onLongPollComplete(BoschHttpClient httpClient, String subscriptionI
* @param subscriptionId Id of subscription the response is for
* @param content Content of the response
*/
private void handleLongPollResponse(BoschHttpClient httpClient, String subscriptionId, @Nullable String content) {
void handleLongPollResponse(BoschHttpClient httpClient, String subscriptionId, @Nullable String content) {
logger.debug("Long poll response: {}", content);

try {
Expand Down Expand Up @@ -239,6 +239,10 @@ private void handleLongPollResponse(BoschHttpClient httpClient, String subscript
this.handleFailure.accept(
new LongPollingFailedException("Could not deserialize long poll response: '" + content + "'", e));
return;
} catch (Exception e) {
this.handleFailure.accept(
new LongPollingFailedException("Error while handling long poll response: '" + content + "'", e));
return;
}

// Execute next run
Expand Down
2 changes: 1 addition & 1 deletion .../main/java/org/openhab/binding/boschshc/internal/devices/bridge/dto/UserDefinedState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class UserDefinedState extends BoschSHCServiceState {
private boolean state;

public UserDefinedState() {
super("UserDefinedState");
super("userDefinedState");
}

public String getId() {
Expand Down
43 changes: 13 additions & 30 deletions ...ava/org/openhab/binding/boschshc/internal/serialization/BoschServiceDataDeserializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
* Utility class for JSON deserialization of device data and triggered scenarios using Google Gson.
*
* @author Patrick Gell - Initial contribution
* @author David Pace - Fixed NPEs and simplified code, added sanity check
*
*/
@NonNullByDefault
Expand All @@ -42,36 +43,18 @@ public class BoschServiceDataDeserializer implements JsonDeserializer<BoschSHCSe
public BoschSHCServiceState deserialize(JsonElement jsonElement, Type type,
JsonDeserializationContext jsonDeserializationContext) throws JsonParseException {
JsonObject jsonObject = jsonElement.getAsJsonObject();
JsonElement dataType = jsonObject.get("@type");
switch (dataType.getAsString()) {
case "DeviceServiceData" -> {
var deviceServiceData = new DeviceServiceData();
deviceServiceData.deviceId = jsonObject.get("deviceId").getAsString();
deviceServiceData.state = jsonObject.get("state");
deviceServiceData.id = jsonObject.get("id").getAsString();
deviceServiceData.path = jsonObject.get("path").getAsString();
return deviceServiceData;
}
case "scenarioTriggered" -> {
var scenario = new Scenario();
scenario.id = jsonObject.get("id").getAsString();
scenario.name = jsonObject.get("name").getAsString();
scenario.lastTimeTriggered = jsonObject.get("lastTimeTriggered").getAsString();
return scenario;
}
case "userDefinedState" -> {
var state = new UserDefinedState();
state.setId(jsonObject.get("id").getAsString());
state.setName(jsonObject.get("name").getAsString());
state.setState(jsonObject.get("state").getAsBoolean());
return state;
}
case "message" -> {
return GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(jsonElement, Message.class);
}
default -> {
return new BoschSHCServiceState(dataType.getAsString());
}
JsonElement dataTypeElement = jsonObject.get("@type");
if (dataTypeElement == null) {
throw new IllegalArgumentException("Received a service state without a @type property: " + jsonObject);
}

String dataType = dataTypeElement.getAsString();
return switch (dataType) {
case "DeviceServiceData" -> GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(jsonObject, DeviceServiceData.class);
case "scenarioTriggered" -> GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(jsonObject, Scenario.class);
case "userDefinedState" -> GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(jsonObject, UserDefinedState.class);
case "message" -> GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(jsonElement, Message.class);
default -> new BoschSHCServiceState(dataType);
};
}
}
35 changes: 31 additions & 4 deletions ...c/src/test/java/org/openhab/binding/boschshc/internal/devices/bridge/LongPollingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
Expand All @@ -24,6 +25,7 @@
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.same;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -461,10 +463,35 @@ void startLongPollingInvalidLongPollResponse()
ArgumentCaptor<Throwable> throwableCaptor = ArgumentCaptor.forClass(Throwable.class);
verify(failureHandler).accept(throwableCaptor.capture());
Throwable t = throwableCaptor.getValue();
assertEquals(
"Could not deserialize long poll response: '<HTML><HEAD><TITLE>400</TITLE></HEAD><BODY><H1>400 Unsupported HTTP Protocol Version: /remote/json-rpcHTTP/1.1</H1></BODY></HTML>'",
t.getMessage());
assertTrue(t.getCause() instanceof JsonSyntaxException);
assertThat(t.getMessage(), is(
"Could not deserialize long poll response: '<HTML><HEAD><TITLE>400</TITLE></HEAD><BODY><H1>400 Unsupported HTTP Protocol Version: /remote/json-rpcHTTP/1.1</H1></BODY></HTML>'"));
assertThat(t.getCause(), instanceOf(JsonSyntaxException.class));
}

@Test
void testHandleLongPollResponseNPE() {
doThrow(NullPointerException.class).when(longPollHandler).accept(any());

var resultJson = """
{
"result": [
{
"@type": "DeviceServiceData",
"deleted": true,
"id": "CommunicationQuality",
"deviceId": "hdm:ZigBee:30fb10fffe46d732"
}
],
"jsonrpc":"2.0"
}
""";
fixture.handleLongPollResponse(httpClient, "subscriptionId", resultJson);

ArgumentCaptor<Throwable> throwableCaptor = ArgumentCaptor.forClass(Throwable.class);
verify(failureHandler).accept(throwableCaptor.capture());
Throwable t = throwableCaptor.getValue();
assertThat(t.getMessage(), is("Error while handling long poll response: '" + resultJson + "'"));
assertThat(t.getCause(), instanceOf(NullPointerException.class));
}

@AfterEach
Expand Down
2 changes: 1 addition & 1 deletion ...t/java/org/openhab/binding/boschshc/internal/devices/bridge/dto/UserDefinedStateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ void testIsValid() {
@Test
void testToString() {
assertEquals(
String.format("UserDefinedState{id='%s', name='test user state', state=true, type='UserDefinedState'}",
String.format("UserDefinedState{id='%s', name='test user state', state=true, type='userDefinedState'}",
testId),
fixture.toString());
}
Expand Down
113 changes: 106 additions & 7 deletions ...org/openhab/binding/boschshc/internal/serialization/BoschServiceDataDeserializerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,29 @@
*/
package org.openhab.binding.boschshc.internal.serialization;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.hasSize;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.HashSet;
import java.util.List;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.junit.jupiter.api.Test;
import org.openhab.binding.boschshc.internal.devices.bridge.dto.DeviceServiceData;
import org.openhab.binding.boschshc.internal.devices.bridge.dto.LongPollResult;
import org.openhab.binding.boschshc.internal.devices.bridge.dto.Scenario;
import org.openhab.binding.boschshc.internal.devices.bridge.dto.UserDefinedState;
import org.openhab.binding.boschshc.internal.services.dto.BoschSHCServiceState;

/**
* Unit tests for {@link BoschServiceDataDeserializer}.
*
* @author Patrick Gell - Initial contribution
* @author David Pace - Added tests for all supported service data classes
*
*/
@NonNullByDefault
Expand Down Expand Up @@ -60,12 +67,104 @@ void deserializationOfLongPollingResult() {
""";

var longPollResult = GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(resultJson, LongPollResult.class);
// note: when using assertThat() to check that the value is non-null, we get compiler warnings.
assertNotNull(longPollResult);
assertEquals(2, longPollResult.result.size());
List<BoschSHCServiceState> results = longPollResult.result;
assertThat(results, is(notNullValue()));
assertThat(results, hasSize(2));

var resultClasses = new HashSet<>(longPollResult.result.stream().map(e -> e.getClass().getName()).toList());
assertEquals(2, resultClasses.size());
assertTrue(resultClasses.contains(DeviceServiceData.class.getName()));
assertTrue(resultClasses.contains(Scenario.class.getName()));
var resultClasses = new HashSet<>(results.stream().map(e -> e.getClass().getName()).toList());
assertThat(resultClasses, hasSize(2));
assertThat(resultClasses, containsInAnyOrder(DeviceServiceData.class.getName(), Scenario.class.getName()));
}

@Test
void testDeserializeDeletedDeviceServiceData() {
var resultJson = """
{
"result": [
{
"@type": "DeviceServiceData",
"deleted": true,
"id": "CommunicationQuality",
"deviceId": "hdm:ZigBee:30fb10fffe46d732"
}
],
"jsonrpc":"2.0"
}
""";

var longPollResult = GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(resultJson, LongPollResult.class);
// note: when using assertThat() to check that the value is non-null, we get compiler warnings.
assertNotNull(longPollResult);
List<BoschSHCServiceState> results = longPollResult.result;
assertThat(results, is(notNullValue()));
assertThat(results, hasSize(1));

DeviceServiceData deviceServiceData = (DeviceServiceData) longPollResult.result.get(0);
assertThat(deviceServiceData.type, is("DeviceServiceData"));
assertThat(deviceServiceData.id, is("CommunicationQuality"));
assertThat(deviceServiceData.deviceId, is("hdm:ZigBee:30fb10fffe46d732"));
}

@Test
void testDeserializeScenarioTriggered() {
String resultJson = """
{
"result": [
{
"@type": "scenarioTriggered",
"name": "My Scenario",
"id": "509bd737-eed0-40b7-8caa-e8686a714399",
"lastTimeTriggered": "1693758693032"
}
],
"jsonrpc": "2.0"
}
""";

var longPollResult = GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(resultJson, LongPollResult.class);
// note: when using assertThat() to check that the value is non-null, we get compiler warnings.
assertNotNull(longPollResult);
List<BoschSHCServiceState> results = longPollResult.result;
assertThat(results, is(notNullValue()));
assertThat(results, hasSize(1));

Scenario scenario = (Scenario) longPollResult.result.get(0);
assertThat(scenario.type, is("scenarioTriggered"));
assertThat(scenario.name, is("My Scenario"));
assertThat(scenario.id, is("509bd737-eed0-40b7-8caa-e8686a714399"));
assertThat(scenario.lastTimeTriggered, is("1693758693032"));
}

@Test
void testDeserializeUserDefinedState() {
String resultJson = """
{
"result": [
{
"@type": "userDefinedState",
"deleted": false,
"name": "Test State",
"id": "3d8023d6-69ca-4e79-89dd-7090295cefbf",
"state": true
}
],
"jsonrpc": "2.0"
}
""";

var longPollResult = GsonUtils.DEFAULT_GSON_INSTANCE.fromJson(resultJson, LongPollResult.class);
// note: when using assertThat() to check that the value is non-null, we get compiler warnings.
assertNotNull(longPollResult);
List<BoschSHCServiceState> results = longPollResult.result;
assertThat(results, is(notNullValue()));
assertThat(results, hasSize(1));

UserDefinedState userDefinedState = (UserDefinedState) longPollResult.result.get(0);
assertThat(userDefinedState.type, is("userDefinedState"));
assertThat(userDefinedState.getName(), is("Test State"));
assertThat(userDefinedState.getId(), is("3d8023d6-69ca-4e79-89dd-7090295cefbf"));
assertThat(userDefinedState.isState(), is(true));
}
}
14 changes: 10 additions & 4 deletions ...est/java/org/openhab/binding/boschshc/internal/services/dto/BoschSHCServiceStateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@
*/
package org.openhab.binding.boschshc.internal.services.dto;

import static org.junit.jupiter.api.Assertions.*;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -85,8 +90,9 @@ void fromJsonStateObjectForValidJsonAfterOtherState() {
@Test
void fromJsonReturnsUserStateServiceStateForValidJson() {
var state = BoschSHCServiceState.fromJson(new JsonPrimitive("false"), UserStateServiceState.class);
assertNotEquals(null, state);
assertTrue(state.getClass().isAssignableFrom(UserStateServiceState.class));
assertFalse(state.isState());
// note: when using assertThat() to check that the value is non-null, we get compiler warnings.
assertNotNull(state);
assertThat(state, instanceOf(UserStateServiceState.class));
assertThat(state.isState(), is(false));
}
}

0 comments on commit ac0d70a

Please sign in to comment.