Eliminate double localization of discovery results (#5151)

* Remove double localization of discovery results

Localization of discovery results requires a bundle from which to acquire the translations. When bindings implement AbstractDiscoveryService themselves, the bundle is resolved using the class of the implementation. For "sub discovery services" where bindings are "participants", this doesn't work, because the  implementing class is the "sub discovery service" itself, not the binding, which means that the resolved bundle doesn't have any translations. This has been solved by doing an extra round of localization in the "sub discovery service" (using the correct bundle) before passing the DiscoveryResult on to AbstractDiscoveryService, which will attempt localization again, with the wrong bundle, but since no translations are found in this bundle, it has no other consequences than being wasteful.

This solves the problem by adding a second thingDiscovered() method to AbstractDiscoveryService, which accepts a bundle as a second argument. That way, "sub discovery services" can resolve the correct bundle using the participant class, and pass on the correct bundle, avoiding the "double localization".

In addition, a new factory method is added to DiscoveryResultBuilder that allows initializing a new builder from an existing DiscoveryResult instance, so that this doesn't have to be done in the localization method itself.

Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>

bundles/org.openhab.core.config.discovery.mdns/src/main/java/org/openhab/core/config/discovery/mdns/internal/MDNSDiscoveryService.java

@@ -222,9 +222,7 @@ public class MDNSDiscoveryService extends AbstractDiscoveryService implements Se
             DiscoveryResult result = participant.createResult(serviceInfo);
             if (result != null) {
                 cancelRemovalTask(serviceInfo);
-                final DiscoveryResult resultNew = getLocalizedDiscoveryResult(result,
-                        FrameworkUtil.getBundle(participant.getClass()));
-                thingDiscovered(resultNew);
+                thingDiscovered(result, FrameworkUtil.getBundle(participant.getClass()));
             }
         } catch (Exception e) {
             logger.error("Participant '{}' threw an exception", participant.getClass().getName(), e);

bundles/org.openhab.core.config.discovery.sddp/src/main/java/org/openhab/core/config/discovery/sddp/SddpDiscoveryService.java

@@ -140,9 +140,7 @@ public class SddpDiscoveryService extends AbstractDiscoveryService
         foundDevicesCache.stream().filter(d -> !d.isExpired()).forEach(d -> {
             DiscoveryResult result = participant.createResult(d);
             if (result != null) {
-                DiscoveryResult localizedResult = getLocalizedDiscoveryResult(result,
-                        FrameworkUtil.getBundle(participant.getClass()));
-                thingDiscovered(localizedResult);
+                thingDiscovered(result, FrameworkUtil.getBundle(participant.getClass()));
             }
         });
     }
@@ -380,9 +378,7 @@ public class SddpDiscoveryService extends AbstractDiscoveryService
                 discoveryParticipants.forEach(p -> {
                     DiscoveryResult discoveryResult = p.createResult(device);
                     if (discoveryResult != null) {
-                        DiscoveryResult localizedResult = getLocalizedDiscoveryResult(discoveryResult,
-                                FrameworkUtil.getBundle(p.getClass()));
-                        thingDiscovered(localizedResult);
+                        thingDiscovered(discoveryResult, FrameworkUtil.getBundle(p.getClass()));
                     }
                 });
                 deviceParticipants.forEach(f -> f.deviceAdded(device));

bundles/org.openhab.core.config.discovery.upnp/src/main/java/org/openhab/core/config/discovery/upnp/internal/UpnpDiscoveryService.java

@@ -118,9 +118,7 @@ public class UpnpDiscoveryService extends AbstractDiscoveryService
             }
             DiscoveryResult result = participant.createResult(device);
             if (result != null) {
-                final DiscoveryResult resultNew = getLocalizedDiscoveryResult(result,
-                        FrameworkUtil.getBundle(participant.getClass()));
-                thingDiscovered(resultNew);
+                thingDiscovered(result, FrameworkUtil.getBundle(participant.getClass()));
             }
         }
     }
@@ -183,9 +181,7 @@ public class UpnpDiscoveryService extends AbstractDiscoveryService
                     if (participant.getRemovalGracePeriodSeconds(device) > 0) {
                         cancelRemovalTask(device.getIdentity().getUdn());
                     }
-                    final DiscoveryResult resultNew = getLocalizedDiscoveryResult(result,
-                            FrameworkUtil.getBundle(participant.getClass()));
-                    thingDiscovered(resultNew);
+                    thingDiscovered(result, FrameworkUtil.getBundle(participant.getClass()));
                 }
             } catch (Exception e) {
                 logger.error("Participant '{}' threw an exception", participant.getClass().getName(), e);

bundles/org.openhab.core.config.discovery.usbserial/src/main/java/org/openhab/core/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

@@ -31,6 +31,7 @@ import org.openhab.core.config.discovery.usbserial.UsbSerialDiscoveryListener;
 import org.openhab.core.config.discovery.usbserial.UsbSerialDiscoveryParticipant;
 import org.openhab.core.thing.ThingTypeUID;
 import org.openhab.core.thing.ThingUID;
+import org.osgi.framework.FrameworkUtil;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.Deactivate;
@@ -101,7 +102,8 @@ public class UsbSerialDiscoveryService extends AbstractDiscoveryService implemen
         for (UsbSerialDeviceInformation usbSerialDeviceInformation : previouslyDiscovered) {
             DiscoveryResult result = participant.createResult(usbSerialDeviceInformation);
             if (result != null) {
-                thingDiscovered(createDiscoveryResultWithUsbProperties(result, usbSerialDeviceInformation));
+                thingDiscovered(createDiscoveryResultWithUsbProperties(result, usbSerialDeviceInformation),
+                        FrameworkUtil.getBundle(participant.getClass()));
             }
         }
     }

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

@@ -354,11 +354,21 @@ public abstract class AbstractDiscoveryService implements DiscoveryService {
     /**
      * Notifies the registered {@link DiscoveryListener}s about a discovered device.
      *
-     * @param discoveryResult Holds the information needed to identify the discovered device.
+     * @param discoveryResult the {@link DiscoveryResult} to send to listeners.
      */
     protected void thingDiscovered(final DiscoveryResult discoveryResult) {
-        final DiscoveryResult discoveryResultNew = getLocalizedDiscoveryResult(discoveryResult,
-                FrameworkUtil.getBundle(this.getClass()));
+        thingDiscovered(discoveryResult, FrameworkUtil.getBundle(this.getClass()));
+    }
+
+    /**
+     * Notifies the registered {@link DiscoveryListener}s about a discovered device, localizing the results
+     * using the specified {@link Bundle}.
+     *
+     * @param discoveryResult the {@link DiscoveryResult} to send to listeners.
+     * @param bundle the {@link Bundle} to use when looking up translations.
+     */
+    protected void thingDiscovered(final DiscoveryResult discoveryResult, @Nullable Bundle bundle) {
+        final DiscoveryResult discoveryResultNew = getLocalizedDiscoveryResult(discoveryResult, bundle);
         for (DiscoveryListener discoveryListener : discoveryListeners) {
             scheduler.execute(() -> {
                 try {
@@ -563,11 +573,7 @@ public abstract class AbstractDiscoveryService implements DiscoveryService {
             if (currentLabel.equals(label)) {
                 return discoveryResult;
             } else {
-                return DiscoveryResultBuilder.create(discoveryResult.getThingUID())
-                        .withThingType(discoveryResult.getThingTypeUID()).withBridge(discoveryResult.getBridgeUID())
-                        .withProperties(discoveryResult.getProperties())
-                        .withRepresentationProperty(discoveryResult.getRepresentationProperty()).withLabel(label)
-                        .withTTL(discoveryResult.getTimeToLive()).build();
+                return DiscoveryResultBuilder.create(discoveryResult).withLabel(label).build();
             }
         } else {
             return discoveryResult;

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

@@ -62,6 +62,20 @@ public class DiscoveryResultBuilder {
         return new DiscoveryResultBuilder(thingUID);
     }
 
+    /**
+     * Creates a new builder initialized with the values from the specified {@link DiscoveryResult}.
+     *
+     * @param discoveryResult the {@link DiscoveryResult} to use for initialization.
+     * @return The new {@link DiscoveryResultBuilder}.
+     */
+    public static DiscoveryResultBuilder create(DiscoveryResult discoveryResult) {
+        return new DiscoveryResultBuilder(discoveryResult.getThingUID()).withBridge(discoveryResult.getBridgeUID())
+                .withProperties(discoveryResult.getProperties())
+                .withRepresentationProperty(discoveryResult.getRepresentationProperty())
+                .withLabel(discoveryResult.getLabel()).withTTL(discoveryResult.getTimeToLive())
+                .withThingType(discoveryResult.getThingTypeUID());
+    }
+
     /**
      * Explicitly sets the thing type.
      *

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

@@ -90,6 +90,18 @@ public class DiscoveryResultBuilderTest {
         assertThat(discoveryResult.getTimeToLive(), is(DiscoveryResult.TTL_UNLIMITED));
     }
 
+    @Test
+    public void testDiscoveryResultBuilderCopy() {
+        DiscoveryResult r = DiscoveryResultBuilder.create(discoveryResult).build();
+        assertThat(r.getThingUID(), is(discoveryResult.getThingUID()));
+        assertThat(r.getThingTypeUID(), is(discoveryResult.getThingTypeUID()));
+        assertThat(r.getBindingId(), is(discoveryResult.getBindingId()));
+        assertThat(r.getLabel(), is(discoveryResult.getLabel()));
+        assertThat(r.getProperties(), is(discoveryResult.getProperties()));
+        assertThat(r.getRepresentationProperty(), is(discoveryResult.getRepresentationProperty()));
+        assertThat(r.getTimeToLive(), is(discoveryResult.getTimeToLive()));
+    }
+
     @Test
     public void testDiscoveryResultBuilderWithTTL() {
         DiscoveryResult otherDiscoveryResult = builder.withTTL(100L).build();