[boschshc] Fix NPE during deserialization, make long polling more robust (#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>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>

bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/LongPolling.java

@@ -211,7 +211,7 @@ public class LongPolling {
      * @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 {
@@ -239,6 +239,10 @@ public class LongPolling {
             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

bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/dto/UserDefinedState.java

@@ -37,7 +37,7 @@ public class UserDefinedState extends BoschSHCServiceState {
     private boolean state;
 
     public UserDefinedState() {
-        super("UserDefinedState");
+        super("userDefinedState");
     }
 
     public String getId() {

bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/serialization/BoschServiceDataDeserializer.java

@@ -32,6 +32,7 @@ import com.google.gson.JsonParseException;
  * 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
@@ -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");
+        JsonElement dataTypeElement = jsonObject.get("@type");
-        switch (dataType.getAsString()) {
+        if (dataTypeElement == null) {
-            case "DeviceServiceData" -> {
+            throw new IllegalArgumentException("Received a service state without a @type property: " + jsonObject);
-                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());
-            }
         }
+
+        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);
+        };
     }
 }

bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/devices/bridge/LongPollingTest.java

@@ -14,6 +14,7 @@ package org.openhab.binding.boschshc.internal.devices.bridge;
 
 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;
@@ -24,6 +25,7 @@ import static org.mockito.ArgumentMatchers.any;
 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;
@@ -461,10 +463,35 @@ class LongPollingTest {
         ArgumentCaptor<Throwable> throwableCaptor = ArgumentCaptor.forClass(Throwable.class);
         verify(failureHandler).accept(throwableCaptor.capture());
         Throwable t = throwableCaptor.getValue();
-        assertEquals(
+        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>'",
+                "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());
+        assertThat(t.getCause(), instanceOf(JsonSyntaxException.class));
-        assertTrue(t.getCause() instanceof JsonSyntaxException);
+    }
+
+    @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

bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/devices/bridge/dto/UserDefinedStateTest.java

@@ -53,7 +53,7 @@ public class UserDefinedStateTest {
     @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());
     }

bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/serialization/BoschServiceDataDeserializerTest.java

@@ -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
@@ -60,12 +67,104 @@ class BoschServiceDataDeserializerTest {
                 """;
 
         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());
+        var resultClasses = new HashSet<>(results.stream().map(e -> e.getClass().getName()).toList());
-        assertEquals(2, resultClasses.size());
+        assertThat(resultClasses, hasSize(2));
-        assertTrue(resultClasses.contains(DeviceServiceData.class.getName()));
+        assertThat(resultClasses, containsInAnyOrder(DeviceServiceData.class.getName(), Scenario.class.getName()));
-        assertTrue(resultClasses.contains(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));
     }
 }

bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/services/dto/BoschSHCServiceStateTest.java

@@ -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;
@@ -85,8 +90,9 @@ class BoschSHCServiceStateTest {
     @Test
     void fromJsonReturnsUserStateServiceStateForValidJson() {
         var state = BoschSHCServiceState.fromJson(new JsonPrimitive("false"), UserStateServiceState.class);
-        assertNotEquals(null, state);
+        // note: when using assertThat() to check that the value is non-null, we get compiler warnings.
-        assertTrue(state.getClass().isAssignableFrom(UserStateServiceState.class));
+        assertNotNull(state);
-        assertFalse(state.isState());
+        assertThat(state, instanceOf(UserStateServiceState.class));
+        assertThat(state.isState(), is(false));
     }
 }