[boschshc] Update location properties when initializing things

When a device is initialized, an attempt is made to look up the room
name for the room id specified for the device and to store the room name
in a thing property. This property is also updated if a room change is
detected.

The legacy property `Location` is removed if present. From now on the
property `location` (with proper lower case spelling) is used.

* add constants for location properties
* implement location updates in abstract device handler
* extend bridge handler to provide a cached list of rooms
* add unit tests

Signed-off-by: David Pace <dev@davidpace.de>

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

@@ -128,4 +128,8 @@ public class BoschSHCBindingConstants {
 
     // static device/service names
     public static final String SERVICE_INTRUSION_DETECTION = "intrusionDetectionSystem";
+
+    // thing properties
+    public static final String PROPERTY_LOCATION_LEGACY = "Location";
+    public static final String PROPERTY_LOCATION = "location";
 }

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

@@ -12,6 +12,10 @@
  */
 package org.openhab.binding.boschshc.internal.devices;
 
+import static org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants.PROPERTY_LOCATION;
+import static org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY;
+
+import java.util.Map;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
 
@@ -83,9 +87,44 @@ public abstract class BoschSHCDeviceHandler extends BoschSHCHandler {
      *         otherwise
      */
     protected boolean processDeviceInfo(Device deviceInfo) {
+        try {
+            updateLocationPropertiesIfApplicable(deviceInfo);
+        } catch (Exception e) {
+            logger.warn("Error while updating location properties for thing {}.", getThing().getUID(), e);
+        }
+        // do not cancel thing initialization if location properties cannot be updated
         return true;
     }
 
+    private void updateLocationPropertiesIfApplicable(Device deviceInfo) throws BoschSHCException {
+        Map<String, String> thingProperties = getThing().getProperties();
+        removeLegacyLocationPropertyIfApplicable(thingProperties);
+        updateLocationPropertyIfApplicable(thingProperties, deviceInfo);
+    }
+
+    private void updateLocationPropertyIfApplicable(Map<String, String> thingProperties, Device deviceInfo)
+            throws BoschSHCException {
+        @Nullable
+        String roomName = getBridgeHandler().resolveRoomId(deviceInfo.roomId);
+        if (roomName != null) {
+            @Nullable
+            String currentLocation = thingProperties.get(PROPERTY_LOCATION);
+            if (currentLocation == null || !currentLocation.equals(roomName)) {
+                logger.debug("Updating property '{}' of thing {} to '{}'.", PROPERTY_LOCATION, getThing().getUID(),
+                        roomName);
+                updateProperty(PROPERTY_LOCATION, roomName);
+            }
+        }
+    }
+
+    private void removeLegacyLocationPropertyIfApplicable(Map<String, String> thingProperties) {
+        if (thingProperties.containsKey(PROPERTY_LOCATION_LEGACY)) {
+            logger.debug("Removing legacy property '{}' from thing {}.", PROPERTY_LOCATION_LEGACY, getThing().getUID());
+            // null value indicates that the property should be removed
+            updateProperty(PROPERTY_LOCATION_LEGACY, null);
+        }
+    }
+
     /**
      * Attempts to obtain information about the device with the specified ID via a REST call.
      * <p>

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

@@ -17,6 +17,7 @@ import static org.eclipse.jetty.http.HttpMethod.POST;
 import static org.eclipse.jetty.http.HttpMethod.PUT;
 
 import java.lang.reflect.Type;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -54,6 +55,7 @@ import org.openhab.binding.boschshc.internal.exceptions.PairingFailedException;
 import org.openhab.binding.boschshc.internal.serialization.GsonUtils;
 import org.openhab.binding.boschshc.internal.services.dto.BoschSHCServiceState;
 import org.openhab.binding.boschshc.internal.services.dto.JsonRestExceptionResponse;
+import org.openhab.core.cache.ExpiringCache;
 import org.openhab.core.library.types.StringType;
 import org.openhab.core.thing.Bridge;
 import org.openhab.core.thing.Channel;
@@ -88,6 +90,8 @@ public class BridgeHandler extends BaseBridgeHandler {
 
     private static final String HTTP_CLIENT_NOT_INITIALIZED = "HttpClient not initialized";
 
+    private static final Duration ROOM_CACHE_DURATION = Duration.ofMinutes(2);
+
     private final Logger logger = LoggerFactory.getLogger(BridgeHandler.class);
 
     /**
@@ -107,13 +111,22 @@ public class BridgeHandler extends BaseBridgeHandler {
 
     /**
      * SHC thing/device discovery service instance.
-     * Registered and unregistered if service is actived/deactived.
+     * Registered and unregistered if service is activated/deactivated.
      * Used to scan for things after bridge is paired with SHC.
      */
     private @Nullable ThingDiscoveryService thingDiscoveryService;
 
     private final ScenarioHandler scenarioHandler;
 
+    private ExpiringCache<List<Room>> roomCache = new ExpiringCache<>(ROOM_CACHE_DURATION, () -> {
+        try {
+            return getRooms();
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            return null;
+        }
+    });
+
     public BridgeHandler(Bridge bridge) {
         super(bridge);
         scenarioHandler = new ScenarioHandler();
@@ -437,6 +450,24 @@ public class BridgeHandler extends BaseBridgeHandler {
         }
     }
 
+    public @Nullable List<Room> getRoomsWithCache() {
+        return roomCache.getValue();
+    }
+
+    public @Nullable String resolveRoomId(@Nullable String roomId) {
+        if (roomId == null) {
+            return null;
+        }
+
+        @Nullable
+        List<Room> rooms = getRoomsWithCache();
+        if (rooms != null) {
+            return rooms.stream().filter(r -> r.id.equals(roomId)).map(r -> r.name).findAny().orElse(null);
+        }
+
+        return null;
+    }
+
     /**
      * Get public information from Bosch SHC.
      */

bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/ThingDiscoveryService.java

@@ -242,7 +242,7 @@ public class ThingDiscoveryService extends AbstractThingHandlerDiscoveryService<
         discoveryResult.withBridge(thingHandler.getThing().getUID());
 
         if (!roomName.isEmpty()) {
-            discoveryResult.withProperty("Location", roomName);
+            discoveryResult.withProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, roomName);
         }
         thingDiscovered(discoveryResult.build());
 

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

@@ -14,14 +14,20 @@ package org.openhab.binding.boschshc.internal.devices;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.openhab.binding.boschshc.internal.devices.bridge.dto.Device;
@@ -42,11 +48,34 @@ import org.openhab.core.thing.ThingStatusDetail;
 public abstract class AbstractBoschSHCDeviceHandlerTest<T extends BoschSHCDeviceHandler>
         extends AbstractBoschSHCHandlerTest<T> {
 
+    protected static final String TAG_LEGACY_LOCATION_PROPERTY = "LegacyLocationProperty";
+    protected static final String TAG_LOCATION_PROPERTY = "LocationProperty";
+    protected static final String DEFAULT_ROOM_ID = "hz_1";
+
     @Override
     protected void configureDevice(Device device) {
         super.configureDevice(device);
 
         device.id = getDeviceID();
+        device.roomId = DEFAULT_ROOM_ID;
+    }
+
+    @Override
+    protected void beforeHandlerInitialization(TestInfo testInfo) {
+        super.beforeHandlerInitialization(testInfo);
+        Set<String> tags = testInfo.getTags();
+        if (tags.contains(TAG_LEGACY_LOCATION_PROPERTY) || tags.contains(TAG_LOCATION_PROPERTY)) {
+            Map<String, String> properties = new HashMap<>();
+            when(getThing().getProperties()).thenReturn(properties);
+
+            if (tags.contains(TAG_LEGACY_LOCATION_PROPERTY)) {
+                properties.put(BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY, "Living Room");
+            }
+
+            if (tags.contains(TAG_LOCATION_PROPERTY)) {
+                when(getBridgeHandler().resolveRoomId(DEFAULT_ROOM_ID)).thenReturn("Kitchen");
+            }
+        }
     }
 
     @Override
@@ -80,4 +109,44 @@ public abstract class AbstractBoschSHCDeviceHandlerTest<T extends BoschSHCDevice
                 argThat(status -> status.getStatus().equals(ThingStatus.OFFLINE)
                         && status.getStatusDetail().equals(ThingStatusDetail.CONFIGURATION_ERROR)));
     }
+
+    @Tag(TAG_LEGACY_LOCATION_PROPERTY)
+    @Test
+    protected void deleteLegacyLocationProperty() {
+        verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY, null);
+        verify(getCallback()).thingUpdated(getThing());
+    }
+
+    @Tag(TAG_LOCATION_PROPERTY)
+    @Test
+    protected void locationPropertyDidNotChange() {
+        verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
+        verify(getCallback()).thingUpdated(getThing());
+
+        getThing().getProperties().put(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
+
+        // re-initialize
+        getFixture().initialize();
+
+        verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
+        verify(getCallback()).thingUpdated(getThing());
+    }
+
+    @Tag(TAG_LOCATION_PROPERTY)
+    @Test
+    protected void locationPropertyDidChange() {
+        verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
+        verify(getCallback()).thingUpdated(getThing());
+
+        getThing().getProperties().put(BoschSHCBindingConstants.PROPERTY_LOCATION, "Kitchen");
+
+        getDevice().roomId = "hz_2";
+        when(getBridgeHandler().resolveRoomId("hz_2")).thenReturn("Dining Room");
+
+        // re-initialize
+        getFixture().initialize();
+
+        verify(getThing()).setProperty(BoschSHCBindingConstants.PROPERTY_LOCATION, "Dining Room");
+        verify(getCallback(), times(2)).thingUpdated(getThing());
+    }
 }

bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/devices/relay/RelayHandlerTest.java

@@ -12,8 +12,11 @@
  */
 package org.openhab.binding.boschshc.internal.devices.relay;
 
+import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.collection.IsCollectionWithSize.hasSize;
+import static org.hamcrest.collection.IsMapContaining.hasKey;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.eq;
@@ -376,4 +379,55 @@ class RelayHandlerTest extends AbstractPowerSwitchHandlerTest<RelayHandler> {
         verify(getCallback(), times(2)).thingUpdated(argThat(t -> ImpulseSwitchService.IMPULSE_SWITCH_SERVICE_NAME
                 .equals(t.getProperties().get(RelayHandler.PROPERTY_MODE))));
     }
+
+    /**
+     * This has to be tested differently for the RelayHandler because the thing mock
+     * will be replaced by a real thing during the first initialization, which
+     * modifies the channels.
+     */
+    @Test
+    @Tag(TAG_LEGACY_LOCATION_PROPERTY)
+    @Override
+    protected void deleteLegacyLocationProperty() {
+        ArgumentCaptor<Thing> thingCaptor = ArgumentCaptor.forClass(Thing.class);
+        verify(getCallback(), times(3)).thingUpdated(thingCaptor.capture());
+        List<Thing> allValues = thingCaptor.getAllValues();
+        assertThat(allValues, hasSize(3));
+        assertThat(allValues.get(2).getProperties(), not(hasKey(BoschSHCBindingConstants.PROPERTY_LOCATION_LEGACY)));
+    }
+
+    /**
+     * This has to be tested differently for the RelayHandler because the thing mock
+     * will be replaced by a real thing during the first initialization, which
+     * modifies the channels.
+     */
+    @Test
+    @Tag(TAG_LOCATION_PROPERTY)
+    @Override
+    protected void locationPropertyDidNotChange() {
+        // re-initialize
+        getFixture().initialize();
+
+        verify(getCallback(), times(3)).thingUpdated(
+                argThat(t -> t.getProperties().get(BoschSHCBindingConstants.PROPERTY_LOCATION).equals("Kitchen")));
+    }
+
+    /**
+     * This has to be tested differently for the RelayHandler because the thing mock
+     * will be replaced by a real thing during the first initialization, which
+     * modifies the channels.
+     */
+    @Test
+    @Tag(TAG_LOCATION_PROPERTY)
+    @Override
+    protected void locationPropertyDidChange() {
+        getDevice().roomId = "hz_2";
+        when(getBridgeHandler().resolveRoomId("hz_2")).thenReturn("Dining Room");
+
+        // re-initialize
+        getFixture().initialize();
+
+        verify(getCallback(), times(4)).thingUpdated(
+                argThat(t -> t.getProperties().get(BoschSHCBindingConstants.PROPERTY_LOCATION).equals("Dining Room")));
+    }
 }

bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/ThingDiscoveryServiceTest.java

@@ -194,7 +194,8 @@ class ThingDiscoveryServiceTest {
         assertThat(result.getThingUID().getId(), is("testDevice_ID"));
         assertThat(result.getBridgeUID().getId(), is("testSHC"));
         assertThat(result.getLabel(), is("Test Name"));
-        assertThat(String.valueOf(result.getProperties().get("Location")), is("TestRoom"));
+        assertThat(String.valueOf(result.getProperties().get(BoschSHCBindingConstants.PROPERTY_LOCATION)),
+                is("TestRoom"));
     }
 
     @Test