[discovery] Added validation for relation between ThingUID and BridgeUID (#1481)

* Added validation for relation between ThingUID and BridgeUID

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/DiscoveryResultBuilder.java

@@ -88,7 +88,7 @@ public class DiscoveryResultBuilder {
      * @return the updated builder
      */
     public DiscoveryResultBuilder withProperty(String key, Object value) {
-        this.properties.put(key, value);
+        properties.put(key, value);
         return this;
     }
 
@@ -110,6 +110,7 @@ public class DiscoveryResultBuilder {
      * @return the updated builder
      */
     public DiscoveryResultBuilder withBridge(@Nullable ThingUID bridgeUID) {
+        validateThingUID(bridgeUID);
         this.bridgeUID = bridgeUID;
         return this;
     }
@@ -146,4 +147,12 @@ public class DiscoveryResultBuilder {
         return new DiscoveryResultImpl(thingTypeUID, thingUID, bridgeUID, properties, representationProperty, label,
                 ttl);
     }
+
+    private void validateThingUID(@Nullable ThingUID bridgeUID) {
+        if (bridgeUID != null && (!thingUID.getBindingId().equals(bridgeUID.getBindingId())
+                || !thingUID.getBridgeIds().contains(bridgeUID.getId()))) {
+            throw new IllegalArgumentException(
+                    "Thing UID '" + thingUID + "' does not match bridge UID '" + bridgeUID + "'");
+        }
+    }
 }

bundles/org.openhab.core.config.discovery/src/test/java/org/openhab/core/config/discovery/DiscoveryResultBuilderTest.java

@@ -0,0 +1,104 @@
+/**
+ * Copyright (c) 2010-2020 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.core.config.discovery;
+
+import static org.hamcrest.CoreMatchers.*;
+import static org.hamcrest.collection.IsMapContaining.hasEntry;
+import static org.junit.Assert.assertThat;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+import org.openhab.core.thing.ThingTypeUID;
+import org.openhab.core.thing.ThingUID;
+
+/**
+ * Tests the {@link DiscoveryResultBuilder}.
+ *
+ * @author Christoph Weitkamp - Initial contribution
+ */
+public class DiscoveryResultBuilderTest {
+
+    private static final String BINDING_ID = "bindingId";
+    private static final ThingUID BRIDGE_UID = new ThingUID(new ThingTypeUID(BINDING_ID, "bridgeTypeId"), "bridgeId");
+    private static final ThingTypeUID THING_TYPE_UID = new ThingTypeUID(BINDING_ID, "thingTypeId");
+    private static final ThingUID THING_UID = new ThingUID(THING_TYPE_UID, BRIDGE_UID, "thingId");
+    private static final String KEY1 = "key1";
+    private static final String KEY2 = "key2";
+    private static final String VALUE1 = "value1";
+    private static final String VALUE2 = "value2";
+    private final Map<String, Object> properties = new HashMap<String, Object>() {
+        private static final long serialVersionUID = 1L;
+        {
+            put(KEY1, VALUE1);
+            put(KEY2, VALUE2);
+        }
+    };
+    private DiscoveryResultBuilder builder;
+    private DiscoveryResult discoveryResult;
+
+    @Before
+    public void setup() {
+        builder = DiscoveryResultBuilder.create(THING_UID).withThingType(THING_TYPE_UID).withProperties(properties)
+                .withRepresentationProperty(KEY1).withLabel("Test");
+        discoveryResult = builder.build();
+    }
+
+    @Test
+    public void testDiscoveryResultBuilder() {
+        assertThat(discoveryResult.getThingUID(), is(THING_UID));
+        assertThat(discoveryResult.getThingTypeUID(), is(THING_TYPE_UID));
+        assertThat(discoveryResult.getBindingId(), is(BINDING_ID));
+        assertThat(discoveryResult.getLabel(), is("Test"));
+        assertThat(discoveryResult.getProperties().size(), is(2));
+        assertThat(discoveryResult.getProperties(), hasEntry(KEY1, VALUE1));
+        assertThat(discoveryResult.getProperties(), hasEntry(KEY2, VALUE2));
+        assertThat(discoveryResult.getRepresentationProperty(), is(KEY1));
+        assertThat(discoveryResult.getTimeToLive(), is(DiscoveryResult.TTL_UNLIMITED));
+    }
+
+    @Test
+    public void testDiscoveryResultBuilderWithTTL() {
+        DiscoveryResult otherDiscoveryResult = builder.withTTL(100L).build();
+
+        assertThat(otherDiscoveryResult.getTimeToLive(), is(100L));
+    }
+
+    @Test
+    public void testDiscoveryResultBuilderWithMatchingBridge() {
+        DiscoveryResult otherDiscoveryResult = builder.withBridge(BRIDGE_UID).build();
+
+        assertThat(otherDiscoveryResult.getBridgeUID(), is(BRIDGE_UID));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testDiscoveryResultBuilderWithBridge() {
+        @SuppressWarnings("unused")
+        DiscoveryResult otherDiscoveryResult = DiscoveryResultBuilder
+                .create(new ThingUID(THING_TYPE_UID, "otherThingId")).withBridge(BRIDGE_UID).build();
+    }
+
+    @Test
+    @Ignore
+    public void subsequentBuildsCreateIndependentDiscoveryResults() {
+        DiscoveryResult otherDiscoveryResult = builder.withLabel("Second Test").withProperties(Collections.emptyMap())
+                .build();
+
+        assertThat(otherDiscoveryResult.getLabel(), is(not(discoveryResult.getLabel())));
+        assertThat(otherDiscoveryResult.getProperties().size(), is(not(discoveryResult.getProperties().size())));
+    }
+}

itests/org.openhab.core.config.discovery.tests/src/main/java/org/openhab/core/config/discovery/DiscoveryServiceMock.java

@@ -15,26 +15,25 @@ package org.openhab.core.config.discovery;
 import java.util.Random;
 import java.util.Set;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.openhab.core.thing.ThingTypeUID;
 import org.openhab.core.thing.ThingUID;
 
 /**
- * The {@link DiscoveryServiceMock} is a mock for a {@link
+ * The {@link DiscoveryServiceMock} is a mock for a {@link DiscoveryService} which can simulate a working and faulty
- * org.openhab.core.config.discovery.DiscoveryService} which can simulate a working and faulty
  * discovery.<br>
- * If this mock is configured to be faulty, an exception is thrown if the discovery is enforced or
+ * If this mock is configured to be faulty, an exception is thrown if the discovery is enforced or aborted.
- * aborted.
  *
  * @author Michael Grammling - Initial contribution
  * @author Thomas Höfer - Added representation
  */
+@NonNullByDefault
 public class DiscoveryServiceMock extends AbstractDiscoveryService {
 
     public static final int DEFAULT_TTL = 60;
 
-    ThingTypeUID thingType;
+    final ThingTypeUID thingType;
-    int timeout;
+    final boolean faulty;
-    boolean faulty;
 
     public DiscoveryServiceMock(ThingTypeUID thingType, int timeout) {
         this(thingType, timeout, false);

itests/org.openhab.core.config.discovery.tests/src/main/java/org/openhab/core/config/discovery/DiscoveryServiceMockOfBridge.java

@@ -14,12 +14,14 @@ package org.openhab.core.config.discovery;
 
 import java.util.Random;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.openhab.core.thing.ThingTypeUID;
 import org.openhab.core.thing.ThingUID;
 
 /**
  * @author Andre Fuechsel - Initial contribution
  */
+@NonNullByDefault
 public class DiscoveryServiceMockOfBridge extends DiscoveryServiceMock {
 
     final ThingUID bridgeUID;
@@ -31,7 +33,11 @@ public class DiscoveryServiceMockOfBridge extends DiscoveryServiceMock {
 
     @Override
     public void startScan() {
-        thingDiscovered(DiscoveryResultBuilder.create(new ThingUID(thingType, "test" + new Random().nextInt(999999999)))
+        if (faulty) {
+            throw new RuntimeException();
+        }
+        thingDiscovered(DiscoveryResultBuilder
+                .create(new ThingUID(thingType, bridgeUID, "test" + new Random().nextInt(999999999)))
                 .withBridge(bridgeUID).withTTL(DEFAULT_TTL).build());
     }
 

itests/org.openhab.core.config.discovery.tests/src/main/java/org/openhab/core/config/discovery/DiscoveryServiceRegistryOSGiTest.java

@@ -59,8 +59,8 @@ public class DiscoveryServiceRegistryOSGiTest extends JavaOSGiTest {
     private static final String ANY_BINDING_ID_3 = "any2BindingId3";
     private static final String ANY_THING_TYPE_3 = "any2ThingType3";
 
-    private static final ThingUID BRIDGE_UID_1 = new ThingUID("binding:bridge:1");
+    private static final ThingUID BRIDGE_UID_1 = new ThingUID(ANY_BINDING_ID_3, "bridge", "1");
-    private static final ThingUID BRIDGE_UID_2 = new ThingUID("binding:bridge:2");
+    private static final ThingUID BRIDGE_UID_2 = new ThingUID(ANY_BINDING_ID_3, "bridge", "2");
 
     private static final String FAULTY_BINDING_ID = "faulty2BindingId";
     private static final String FAULTY_THING_TYPE = "faulty2ThingType";
@@ -136,7 +136,6 @@ public class DiscoveryServiceRegistryOSGiTest extends JavaOSGiTest {
 
         serviceRegs.forEach(ServiceRegistration::unregister);
 
-        Inbox inbox = getService(Inbox.class);
         List<DiscoveryResult> discoveryResults = inbox.getAll();
         discoveryResults.forEach(res -> inbox.remove(res.getThingUID()));
         discoveryServiceRegistry.removeDiscoveryListener(mockDiscoveryListener);

itests/org.openhab.core.config.discovery.tests/src/main/java/org/openhab/core/config/discovery/internal/InboxOSGiTest.java

@@ -110,22 +110,25 @@ public class InboxOSGiTest extends JavaOSGiTest {
     private static final ThingTypeUID BRIDGE_THING_TYPE_UID = new ThingTypeUID("bindingId", "bridge");
 
     private static final ThingUID BRIDGE_THING_UID = new ThingUID(BRIDGE_THING_TYPE_UID, "bridgeId");
+    private static final ThingUID OTHER_BRIDGE_THING_UID = new ThingUID(THING_TYPE_UID, "id5");
 
     private static final DiscoveryResult BRIDGE = DiscoveryResultBuilder.create(BRIDGE_THING_UID)
             .withThingType(BRIDGE_THING_TYPE_UID).withRepresentationProperty("Bridge1").withLabel("bridge")
             .withTTL(DEFAULT_TTL).build();
     private static final DiscoveryResult THING1_WITH_BRIDGE = DiscoveryResultBuilder
-            .create(new ThingUID(THING_TYPE_UID, "id1")).withThingType(THING_TYPE_UID).withBridge(BRIDGE_THING_UID)
+            .create(new ThingUID(THING_TYPE_UID, BRIDGE_THING_UID, "id1")).withThingType(THING_TYPE_UID)
-            .withRepresentationProperty("Thing1").withLabel("thing1").withTTL(DEFAULT_TTL).build();
+            .withBridge(BRIDGE_THING_UID).withRepresentationProperty("Thing1").withLabel("thing1").withTTL(DEFAULT_TTL)
+            .build();
     private static final DiscoveryResult THING2_WITH_BRIDGE = DiscoveryResultBuilder
-            .create(new ThingUID(THING_TYPE_UID, "id2")).withThingType(THING_TYPE_UID).withBridge(BRIDGE_THING_UID)
+            .create(new ThingUID(THING_TYPE_UID, BRIDGE_THING_UID, "id2")).withThingType(THING_TYPE_UID)
-            .withRepresentationProperty("Thing2").withLabel("thing2").withTTL(DEFAULT_TTL).build();
+            .withBridge(BRIDGE_THING_UID).withRepresentationProperty("Thing2").withLabel("thing2").withTTL(DEFAULT_TTL)
+            .build();
     private static final DiscoveryResult THING_WITHOUT_BRIDGE = DiscoveryResultBuilder
             .create(new ThingUID(THING_TYPE_UID, "id3")).withThingType(THING_TYPE_UID)
             .withRepresentationProperty("Thing3").withLabel("thing3").withTTL(DEFAULT_TTL).build();
     private static final DiscoveryResult THING_WITH_OTHER_BRIDGE = DiscoveryResultBuilder
-            .create(new ThingUID(THING_TYPE_UID, "id4")).withThingType(THING_TYPE_UID)
+            .create(new ThingUID(THING_TYPE_UID, OTHER_BRIDGE_THING_UID, "id4")).withThingType(THING_TYPE_UID)
-            .withBridge(new ThingUID(THING_TYPE_UID, "id5")).withRepresentationProperty("Thing4").withLabel("thing4")
+            .withBridge(OTHER_BRIDGE_THING_UID).withRepresentationProperty("Thing4").withLabel("thing4")
             .withTTL(DEFAULT_TTL).build();
 
     private final URI testURI = createURI("http:dummy");