diff --git a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java index efe146dba15..945b3889c12 100644 --- a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java +++ b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java @@ -56,6 +56,16 @@ public class BoschHttpClient extends HttpClient { private final Logger logger = LoggerFactory.getLogger(getClass()); + /** + * Default number of seconds for HTTP request timeouts + */ + public static final long DEFAULT_TIMEOUT_SECONDS = 10; + + /** + * The time unit used for default HTTP request timeouts + */ + public static final TimeUnit DEFAULT_TIMEOUT_UNIT = TimeUnit.SECONDS; + private final String ipAddress; private final String systemPassword; @@ -299,7 +309,7 @@ public class BoschHttpClient extends HttpClient { Request request = this.newRequest(url).method(method).header("Content-Type", "application/json") .header("api-version", "3.2") // see https://github.com/BoschSmartHome/bosch-shc-api-docs/issues/80 - .timeout(10, TimeUnit.SECONDS); // Set default timeout + .timeout(DEFAULT_TIMEOUT_SECONDS, DEFAULT_TIMEOUT_UNIT); // Set default timeout if (content != null) { final String body; diff --git a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java index 5ef163a304b..fd7d5f8d4b1 100644 --- a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java +++ b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java @@ -31,6 +31,7 @@ import org.eclipse.jetty.util.ssl.SslContextFactory; import org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants; import org.openhab.binding.boschshc.internal.devices.bridge.BoschHttpClient; import org.openhab.binding.boschshc.internal.devices.bridge.dto.PublicInformation; +import org.openhab.core.cache.ExpiringCacheMap; import org.openhab.core.config.discovery.DiscoveryResult; import org.openhab.core.config.discovery.DiscoveryResultBuilder; import org.openhab.core.config.discovery.mdns.MDNSDiscoveryParticipant; @@ -46,23 +47,32 @@ import org.slf4j.LoggerFactory; import com.google.gson.Gson; /** - * The {@link BridgeDiscoveryParticipant} is responsible discovering the - * Bosch Smart Home Controller as a Bridge with the mDNS services. + * The {@link BridgeDiscoveryParticipant} is responsible discovering the Bosch + * Smart Home Controller as a Bridge with the mDNS services. * * @author Gerd Zanker - Initial contribution + * @author David Pace - Discovery result caching */ @NonNullByDefault @Component(configurationPid = "discovery.boschsmarthomebridge") public class BridgeDiscoveryParticipant implements MDNSDiscoveryParticipant { - private static final long TTL_SECONDS = Duration.ofHours(1).toSeconds(); + private static final String NAME_PREFIX_BOSCH_SHC = "Bosch SHC"; + private static final Duration TTL_DURATION = Duration.ofMinutes(10); + private static final long TTL_SECONDS = TTL_DURATION.toSeconds(); + public static final Set SUPPORTED_THING_TYPES_UIDS = Set.of(BoschSHCBindingConstants.THING_TYPE_SHC); private final Logger logger = LoggerFactory.getLogger(BridgeDiscoveryParticipant.class); private final HttpClient httpClient; private final Gson gson = new Gson(); - /// SHC Bridge Information, read via public REST API if bridge is detected. Otherwise, strings are empty. - private PublicInformation bridgeInformation = new PublicInformation(); + /** + * Cache for bridge discovery results. Uses the IP address of mDNS events as + * key. If the value is null, no Bosch SHC controller could be + * identified at the corresponding IP address. + */ + private ExpiringCacheMap discoveryResultCache = new ExpiringCacheMap<>( + TTL_DURATION); @Activate public BridgeDiscoveryParticipant(@Reference HttpClientFactory httpClientFactory) { @@ -89,9 +99,43 @@ public class BridgeDiscoveryParticipant implements MDNSDiscoveryParticipant { return "_http._tcp.local."; } + /** + * This method is frequently called by the mDNS discovery framework in different + * threads with individual service info instances. + *

+ * Different service info objects can refer to the same Bosch SHC controller, + * e.g. the controller may be reachable via a 192.168.*.* IP and an + * IP in the 169.254.*.* range. The response from the controller + * contains the actual resolved IP address. + *

+ * We ignore mDNS events if they do not contain any IP addresses or if the name + * property does not start with Bosch SHC. + */ @Override public @Nullable DiscoveryResult createResult(ServiceInfo serviceInfo) { - logger.trace("Bridge Discovery started for {}", serviceInfo); + if (logger.isTraceEnabled()) { + logger.trace("Bridge discovery invoked with mDNS service info {}", serviceInfo); + } + + String name = serviceInfo.getName(); + if (name == null || !name.startsWith(NAME_PREFIX_BOSCH_SHC)) { + if (logger.isTraceEnabled()) { + logger.trace("Ignoring mDNS service event because name '{}' does not start with '{}')", name, + NAME_PREFIX_BOSCH_SHC); + } + return null; + } + + @Nullable + String ipAddress = getFirstIPAddress(serviceInfo); + if (ipAddress == null || ipAddress.isBlank()) { + return null; + } + + PublicInformation publicInformation = getOrComputePublicInformation(ipAddress); + if (publicInformation == null) { + return null; + } @Nullable final ThingUID uid = getThingUID(serviceInfo); @@ -99,63 +143,103 @@ public class BridgeDiscoveryParticipant implements MDNSDiscoveryParticipant { return null; } - logger.trace("Discovered Bosch Smart Home Controller at {}", bridgeInformation.shcIpAddress); - return DiscoveryResultBuilder.create(uid) - .withLabel("Bosch Smart Home Controller (" + bridgeInformation.shcIpAddress + ")") - .withProperty("ipAddress", bridgeInformation.shcIpAddress) - .withProperty("shcGeneration", bridgeInformation.shcGeneration) - .withProperty("apiVersions", bridgeInformation.apiVersions).withTTL(TTL_SECONDS).build(); + .withLabel("Bosch Smart Home Controller (" + publicInformation.shcIpAddress + ")") + .withProperty("ipAddress", publicInformation.shcIpAddress) + .withProperty("shcGeneration", publicInformation.shcGeneration) + .withProperty("apiVersions", publicInformation.apiVersions).withTTL(TTL_SECONDS).build(); + } + + private @Nullable String getFirstIPAddress(ServiceInfo serviceInfo) { + String[] hostAddresses = serviceInfo.getHostAddresses(); + if (hostAddresses != null && hostAddresses.length > 0 && !hostAddresses[0].isEmpty()) { + return hostAddresses[0]; + } + + return null; + } + + /** + * Provides a cached discovery result if available, or performs an actual + * communication attempt to the device with the given IP address. + *

+ * This method is synchronized because multiple threads try to access discovery + * results concurrently. We only want one thread to "win" and to invoke the + * actual HTTP communication. + * + * @param ipAddress IP address to contact if no cached result is available + * @return the {@link PublicInformation} of the Bosch Smart Home Controller or + * null if the device with the given IP address could not + * be identified as Bosch Smart Home Controller + */ + protected synchronized @Nullable PublicInformation getOrComputePublicInformation(String ipAddress) { + return discoveryResultCache.putIfAbsentAndGet(ipAddress, () -> { + logger.trace("No cached bridge discovery result available for IP {}, trying to contact SHC", ipAddress); + return discoverBridge(ipAddress); + }); } @Override public @Nullable ThingUID getThingUID(ServiceInfo serviceInfo) { - String ipAddress = discoverBridge(serviceInfo).shcIpAddress; - if (!ipAddress.isBlank()) { - return new ThingUID(BoschSHCBindingConstants.THING_TYPE_SHC, ipAddress.replace('.', '-')); + String ipAddress = getFirstIPAddress(serviceInfo); + if (ipAddress != null) { + @Nullable + PublicInformation publicInformation = getOrComputePublicInformation(ipAddress); + if (publicInformation != null) { + String resolvedIpAddress = publicInformation.shcIpAddress; + return new ThingUID(BoschSHCBindingConstants.THING_TYPE_SHC, resolvedIpAddress.replace('.', '-')); + } } return null; } - protected PublicInformation discoverBridge(ServiceInfo serviceInfo) { - logger.trace("Discovering serviceInfo {}", serviceInfo); - - if (serviceInfo.getHostAddresses() != null && serviceInfo.getHostAddresses().length > 0 - && !serviceInfo.getHostAddresses()[0].isEmpty()) { - String address = serviceInfo.getHostAddresses()[0]; - logger.trace("Discovering InetAddress {}", address); - // store all information for later access - bridgeInformation = getPublicInformationFromPossibleBridgeAddress(address); + protected @Nullable PublicInformation discoverBridge(String ipAddress) { + logger.debug("Attempting to contact Bosch Smart Home Controller at IP {}", ipAddress); + PublicInformation bridgeInformation = getPublicInformationFromPossibleBridgeAddress(ipAddress); + if (bridgeInformation != null && bridgeInformation.shcIpAddress != null + && !bridgeInformation.shcIpAddress.isBlank()) { + return bridgeInformation; } - return bridgeInformation; + return null; } - protected PublicInformation getPublicInformationFromPossibleBridgeAddress(String ipAddress) { + /** + * Attempts to send a HTTP request to the given IP address in order to determine + * if the device is a Bosch Smart Home Controller. + * + * @param ipAddress the IP address of the potential Bosch Smart Home Controller + * @return a {@link PublicInformation} object if the bridge was successfully + * contacted or null if the communication failed + */ + protected @Nullable PublicInformation getPublicInformationFromPossibleBridgeAddress(String ipAddress) { String url = BoschHttpClient.getPublicInformationUrl(ipAddress); - logger.trace("Discovering ipAddress {}", url); + logger.trace("Requesting SHC information via URL {}", url); try { httpClient.start(); - ContentResponse contentResponse = httpClient.newRequest(url).method(HttpMethod.GET).send(); + ContentResponse contentResponse = httpClient.newRequest(url).method(HttpMethod.GET) + .timeout(BoschHttpClient.DEFAULT_TIMEOUT_SECONDS, BoschHttpClient.DEFAULT_TIMEOUT_UNIT).send(); + // check HTTP status code if (!HttpStatus.getCode(contentResponse.getStatus()).isSuccess()) { - logger.debug("Discovering failed with status code: {}", contentResponse.getStatus()); - return new PublicInformation(); + logger.debug("Discovery failed with status code {}: {}", contentResponse.getStatus(), + contentResponse.getContentAsString()); + return null; } // get content from response String content = contentResponse.getContentAsString(); - logger.trace("Discovered SHC - public info {}", content); + logger.debug("Discovered SHC at IP {}, public info: {}", ipAddress, content); PublicInformation bridgeInfo = gson.fromJson(content, PublicInformation.class); - if (bridgeInfo.shcIpAddress != null) { + if (bridgeInfo != null && bridgeInfo.shcIpAddress != null && !bridgeInfo.shcIpAddress.isBlank()) { return bridgeInfo; } } catch (TimeoutException | ExecutionException e) { - logger.debug("Discovering failed with exception {}", e.getMessage()); + logger.debug("Discovery could not reach SHC at IP {}: {}", ipAddress, e.getMessage()); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } catch (Exception e) { - logger.debug("Discovering failed during http client request {}", e.getMessage()); + logger.warn("Discovery failed during HTTP client request: {}", e.getMessage(), e); } - return new PublicInformation(); + return null; } } diff --git a/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml b/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml index b32da8ee02b..cfaa69b1bfc 100644 --- a/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml +++ b/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml @@ -8,4 +8,22 @@ This is the binding for Bosch Smart Home. local + + + mdns + + + mdnsServiceType + _http._tcp.local. + + + + + name + Bosch SHC.* + + + + + diff --git a/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java b/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java index 6e7552db048..c98d24142f0 100644 --- a/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java +++ b/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java @@ -13,10 +13,20 @@ package org.openhab.binding.boschshc.internal.discovery; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.*; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.net.ConnectException; +import java.util.concurrent.ExecutionException; import javax.jmdns.ServiceInfo; @@ -31,10 +41,12 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; +import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; import org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants; +import org.openhab.binding.boschshc.internal.devices.bridge.dto.PublicInformation; import org.openhab.core.config.discovery.DiscoveryResult; import org.openhab.core.thing.ThingUID; @@ -48,31 +60,47 @@ import org.openhab.core.thing.ThingUID; @NonNullByDefault class BridgeDiscoveryParticipantTest { - @Nullable - private BridgeDiscoveryParticipant fixture; + private @NonNullByDefault({}) BridgeDiscoveryParticipant fixture; private final String url = "https://192.168.0.123:8446/smarthome/public/information"; + private final String urlOtherDevice = "https://192.168.0.1:8446/smarthome/public/information"; private @Mock @NonNullByDefault({}) ServiceInfo shcBridge; private @Mock @NonNullByDefault({}) ServiceInfo otherDevice; + private @Mock @NonNullByDefault({}) ContentResponse contentResponse; + + /** + * Spy needed because some final methods can't be mocked + */ + private @Spy @NonNullByDefault({}) HttpClient mockHttpClient; + @BeforeEach public void beforeEach() throws Exception { when(shcBridge.getHostAddresses()).thenReturn(new String[] { "192.168.0.123" }); - when(otherDevice.getHostAddresses()).thenReturn(new String[] { "192.168.0.1" }); + when(shcBridge.getName()).thenReturn("Bosch SHC [xx-xx-xx-xx-xx-xx]"); + + when(otherDevice.getHostAddresses()).thenReturn(new String[] { "192.168.0.1" }); + when(otherDevice.getName()).thenReturn("Other Device"); - ContentResponse contentResponse = mock(ContentResponse.class); when(contentResponse.getContentAsString()).thenReturn( "{\"apiVersions\":[\"2.9\",\"3.2\"], \"shcIpAddress\":\"192.168.0.123\", \"shcGeneration\":\"SHC_1\"}"); when(contentResponse.getStatus()).thenReturn(HttpStatus.OK_200); Request mockRequest = mock(Request.class); when(mockRequest.send()).thenReturn(contentResponse); - when(mockRequest.method((HttpMethod) any())).thenReturn(mockRequest); + when(mockRequest.method(any(HttpMethod.class))).thenReturn(mockRequest); + when(mockRequest.timeout(anyLong(), any())).thenReturn(mockRequest); - HttpClient mockHttpClient = spy(HttpClient.class); // spy needed, because some final methods can't be mocked when(mockHttpClient.newRequest(url)).thenReturn(mockRequest); + Request failingRequest = mock(Request.class); + when(failingRequest.method(any(HttpMethod.class))).thenReturn(failingRequest); + when(failingRequest.timeout(anyLong(), any())).thenReturn(failingRequest); + when(failingRequest.send()).thenThrow(new ExecutionException(new ConnectException("Connection refused"))); + + when(mockHttpClient.newRequest(urlOtherDevice)).thenReturn(failingRequest); + fixture = new BridgeDiscoveryParticipant(mockHttpClient); } @@ -84,7 +112,6 @@ class BridgeDiscoveryParticipantTest { @Test void testGetSupportedThingTypeUIDs() { - assert fixture != null; assertTrue(fixture.getSupportedThingTypeUIDs().contains(BoschSHCBindingConstants.THING_TYPE_SHC)); } @@ -95,13 +122,11 @@ class BridgeDiscoveryParticipantTest { */ @Test void testGetServiceType() throws Exception { - assert fixture != null; assertThat(fixture.getServiceType(), is("_http._tcp.local.")); } @Test void testCreateResult() throws Exception { - assert fixture != null; DiscoveryResult result = fixture.createResult(shcBridge); assertNotNull(result); assertThat(result.getBindingId(), is(BoschSHCBindingConstants.BINDING_ID)); @@ -112,14 +137,19 @@ class BridgeDiscoveryParticipantTest { @Test void testCreateResultOtherDevice() throws Exception { - assert fixture != null; DiscoveryResult result = fixture.createResult(otherDevice); assertNull(result); } + @Test + void testCreateResultNoIPAddress() throws Exception { + when(shcBridge.getHostAddresses()).thenReturn(new String[] { "" }); + DiscoveryResult result = fixture.createResult(shcBridge); + assertNull(result); + } + @Test void testGetThingUID() throws Exception { - assert fixture != null; ThingUID thingUID = fixture.getThingUID(shcBridge); assertNotNull(thingUID); assertThat(thingUID.getBindingId(), is(BoschSHCBindingConstants.BINDING_ID)); @@ -128,64 +158,53 @@ class BridgeDiscoveryParticipantTest { @Test void testGetThingUIDOtherDevice() throws Exception { - assert fixture != null; assertNull(fixture.getThingUID(otherDevice)); } @Test void testGetBridgeAddress() throws Exception { - assert fixture != null; - assertThat(fixture.discoverBridge(shcBridge).shcIpAddress, is("192.168.0.123")); + @Nullable + PublicInformation bridgeInformation = fixture.discoverBridge("192.168.0.123"); + assertThat(bridgeInformation, not(nullValue())); + assertThat(bridgeInformation.shcIpAddress, is("192.168.0.123")); } @Test void testGetBridgeAddressOtherDevice() throws Exception { - assert fixture != null; - assertThat(fixture.discoverBridge(otherDevice).shcIpAddress, is("")); + assertThat(fixture.discoverBridge("192.168.0.1"), is(nullValue())); } @Test void testGetPublicInformationFromPossibleBridgeAddress() throws Exception { - assert fixture != null; - assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123").shcIpAddress, - is("192.168.0.123")); + @Nullable + PublicInformation bridgeInformation = fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123"); + assertThat(bridgeInformation, not(nullValue())); + assertThat(bridgeInformation.shcIpAddress, is("192.168.0.123")); } @Test void testGetPublicInformationFromPossibleBridgeAddressInvalidContent() throws Exception { - assert fixture != null; - - ContentResponse contentResponse = mock(ContentResponse.class); when(contentResponse.getContentAsString()).thenReturn("{\"nothing\":\"useful\"}"); - when(contentResponse.getStatus()).thenReturn(HttpStatus.OK_200); - - Request mockRequest = mock(Request.class); - when(mockRequest.send()).thenReturn(contentResponse); - when(mockRequest.method((HttpMethod) any())).thenReturn(mockRequest); - - HttpClient mockHttpClient = spy(HttpClient.class); // spy needed, because some final methods can't be mocked - when(mockHttpClient.newRequest(url)).thenReturn(mockRequest); fixture = new BridgeDiscoveryParticipant(mockHttpClient); - assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("shcAddress").shcIpAddress, is("")); + assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123"), is(nullValue())); } @Test void testGetPublicInformationFromPossibleBridgeAddressInvalidStatus() throws Exception { - assert fixture != null; - - ContentResponse contentResponse = mock(ContentResponse.class); - // when(contentResponse.getContentAsString()).thenReturn("{\"nothing\":\"useful\"}"); no content needed when(contentResponse.getStatus()).thenReturn(HttpStatus.BAD_REQUEST_400); - Request mockRequest = mock(Request.class); - when(mockRequest.send()).thenReturn(contentResponse); - when(mockRequest.method((HttpMethod) any())).thenReturn(mockRequest); - - HttpClient mockHttpClient = spy(HttpClient.class); // spy needed, because some final methods can't be mocked - when(mockHttpClient.newRequest(url)).thenReturn(mockRequest); - fixture = new BridgeDiscoveryParticipant(mockHttpClient); - assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("shcAddress").shcIpAddress, is("")); + assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123"), is(nullValue())); + } + + @Test + void testGetOrComputePublicInformation() throws Exception { + @Nullable + PublicInformation result = fixture.getOrComputePublicInformation("192.168.0.123"); + assertNotNull(result); + @Nullable + PublicInformation result2 = fixture.getOrComputePublicInformation("192.168.0.123"); + assertSame(result, result2); } }