Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public DiscoveryResultBuilder withProperties(@Nullable Map<String, Object> prope
* @return the updated builder
*/
public DiscoveryResultBuilder withProperty(String key, Object value) {
this.properties.put(key, value);
properties.put(key, value);
return this;
}

Expand All @@ -110,6 +110,7 @@ public DiscoveryResultBuilder withRepresentationProperty(@Nullable String repres
* @return the updated builder
*/
public DiscoveryResultBuilder withBridge(@Nullable ThingUID bridgeUID) {
validateThingUID(bridgeUID);
this.bridgeUID = bridgeUID;
return this;
}
Expand Down Expand Up @@ -146,4 +147,12 @@ public DiscoveryResult build() {
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 + "'");
}
}
}
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test ignored?

Copy link
Contributor Author

@cweitkamp cweitkamp Jul 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To streamline our builders (following eclipse-archived/smarthome#5929) I first changed the behavior of the withProperties(Map<String, Object>) method to replace existing properties map by the one set. This has been reverted after #1481 (comment) and thus the test is failing now. Will come up with a follow-up PR if the change should be applied at all to separate it from this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a look here for the changes I am talking about.

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())));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,25 @@
import java.util.Collections;
import java.util.Random;

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
* org.openhab.core.config.discovery.DiscoveryService} which can simulate a working and faulty
* The {@link DiscoveryServiceMock} is a mock for a {@link 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
* aborted.
* If this mock is configured to be faulty, an exception is thrown if the discovery is enforced or 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;
int timeout;
boolean faulty;
final ThingTypeUID thingType;
final boolean faulty;

public DiscoveryServiceMock(ThingTypeUID thingType, int timeout) {
this(thingType, timeout, false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@

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;
Expand All @@ -31,7 +33,11 @@ public DiscoveryServiceMockOfBridge(ThingTypeUID thingType, int timeout, ThingUI

@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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,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_2 = new ThingUID("binding:bridge:2");
private static final ThingUID BRIDGE_UID_1 = new ThingUID(ANY_BINDING_ID_3, "bridge", "1");
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";
Expand Down Expand Up @@ -131,7 +131,6 @@ public void cleanUp() {

serviceRegs.forEach(ServiceRegistration::unregister);

Inbox inbox = getService(Inbox.class);
List<DiscoveryResult> discoveryResults = inbox.getAll();
discoveryResults.forEach(res -> inbox.remove(res.getThingUID()));
discoveryServiceRegistry.removeDiscoveryListener(mockDiscoveryListener);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,22 +111,25 @@ protected void startScan() {
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)
.withRepresentationProperty("Thing1").withLabel("thing1").withTTL(DEFAULT_TTL).build();
.create(new ThingUID(THING_TYPE_UID, BRIDGE_THING_UID, "id1")).withThingType(THING_TYPE_UID)
.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)
.withRepresentationProperty("Thing2").withLabel("thing2").withTTL(DEFAULT_TTL).build();
.create(new ThingUID(THING_TYPE_UID, BRIDGE_THING_UID, "id2")).withThingType(THING_TYPE_UID)
.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)
.withBridge(new ThingUID(THING_TYPE_UID, "id5")).withRepresentationProperty("Thing4").withLabel("thing4")
.create(new ThingUID(THING_TYPE_UID, OTHER_BRIDGE_THING_UID, "id4")).withThingType(THING_TYPE_UID)
.withBridge(OTHER_BRIDGE_THING_UID).withRepresentationProperty("Thing4").withLabel("thing4")
.withTTL(DEFAULT_TTL).build();

private final URI testURI = createURI("http:dummy");
Expand Down