[inbox] Approval with newThingId: only one segment allowed (#2338)

* [inbox] Approval with newThingId: only one segment allowed Build of the new thing UID corrected in case newThingId is provided Fix #2331 Signed-off-by: Laurent Garnier <[email protected]>
openhab · May 4, 2021 · 6eee846 · 6eee846
1 parent 141e73c
commit 6eee846
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 4 deletions.
diff --git a/...g.discovery/src/main/java/org/openhab/core/config/discovery/internal/PersistentInbox.java b/...g.discovery/src/main/java/org/openhab/core/config/discovery/internal/PersistentInbox.java
@@ -36,6 +36,7 @@
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.openhab.core.common.AbstractUID;
 import org.openhab.core.common.ThreadPoolManager;
 import org.openhab.core.config.core.ConfigDescription;
 import org.openhab.core.config.core.ConfigDescriptionParameter;
@@ -183,6 +184,9 @@ protected void deactivate() {
         if (results.isEmpty()) {
             throw new IllegalArgumentException("No Thing with UID " + thingUID.getAsString() + " in inbox");
         }
+        if (newThingId != null && newThingId.contains(AbstractUID.SEPARATOR)) {
+            throw new IllegalArgumentException("New Thing ID " + newThingId + " must not contain multiple segments");
+        }
         DiscoveryResult result = results.get(0);
         final Map<String, String> properties = new HashMap<>();
         final Map<String, Object> configParams = new HashMap<>();
@@ -191,11 +195,12 @@ protected void deactivate() {
         ThingTypeUID thingTypeUID = result.getThingTypeUID();
         ThingUID newThingUID = thingUID;
         if (newThingId != null) {
+            String newUID = thingUID.getAsString().substring(0,
+                    thingUID.getAsString().lastIndexOf(AbstractUID.SEPARATOR) + 1) + newThingId;
             try {
-                newThingUID = new ThingUID(thingTypeUID, newThingId);
+                newThingUID = new ThingUID(newUID);
             } catch (IllegalArgumentException e) {
-                logger.warn("Cannot create thing: {}", e.getMessage());
-                return null;
+                throw new IllegalArgumentException("Invalid thing UID " + newUID, e);
             }
         }
         Thing newThing = ThingFactory.createThing(newThingUID, config, properties, result.getBridgeUID(), thingTypeUID,

diff --git a/...scovery/src/test/java/org/openhab/core/config/discovery/internal/PersistentInboxTest.java b/...scovery/src/test/java/org/openhab/core/config/discovery/internal/PersistentInboxTest.java
@@ -172,7 +172,15 @@ public void testApproveWithInvalidThingId() throws URISyntaxException {
         when(storage.getValues()).thenReturn(List.of(result));
 
         inbox.activate();
-        assertNull(inbox.approve(THING_UID, "Test", "invalid:id"));
+        Exception exception = assertThrows(IllegalArgumentException.class, () -> {
+            inbox.approve(THING_UID, "Test", "invalid:id");
+        });
+        assertEquals("New Thing ID invalid:id must not contain multiple segments", exception.getMessage());
+
+        exception = assertThrows(IllegalArgumentException.class, () -> {
+            inbox.approve(THING_UID, "Test", "invalid$id");
+        });
+        assertEquals("Invalid thing UID test:test:invalid$id", exception.getMessage());
     }
 
     @Test