From 84cfec2925cedc2e030282fabcdc090278523cf3 Mon Sep 17 00:00:00 2001 From: Nadahar Date: Tue, 9 Dec 2025 22:18:14 +0100 Subject: [PATCH] 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 --- .../mdns/internal/MDNSDiscoveryService.java | 4 +--- .../discovery/sddp/SddpDiscoveryService.java | 8 ++----- .../upnp/internal/UpnpDiscoveryService.java | 8 ++----- .../internal/UsbSerialDiscoveryService.java | 4 +++- .../discovery/AbstractDiscoveryService.java | 22 ++++++++++++------- .../discovery/DiscoveryResultBuilder.java | 14 ++++++++++++ .../discovery/DiscoveryResultBuilderTest.java | 12 ++++++++++ 7 files changed, 48 insertions(+), 24 deletions(-) diff --git a/bundles/org.openhab.core.config.discovery.mdns/src/main/java/org/openhab/core/config/discovery/mdns/internal/MDNSDiscoveryService.java b/bundles/org.openhab.core.config.discovery.mdns/src/main/java/org/openhab/core/config/discovery/mdns/internal/MDNSDiscoveryService.java index aa8c0b1c1..9e553a009 100644 --- a/bundles/org.openhab.core.config.discovery.mdns/src/main/java/org/openhab/core/config/discovery/mdns/internal/MDNSDiscoveryService.java +++ b/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); diff --git a/bundles/org.openhab.core.config.discovery.sddp/src/main/java/org/openhab/core/config/discovery/sddp/SddpDiscoveryService.java b/bundles/org.openhab.core.config.discovery.sddp/src/main/java/org/openhab/core/config/discovery/sddp/SddpDiscoveryService.java index d41a5e57b..b182f40d0 100644 --- a/bundles/org.openhab.core.config.discovery.sddp/src/main/java/org/openhab/core/config/discovery/sddp/SddpDiscoveryService.java +++ b/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)); diff --git a/bundles/org.openhab.core.config.discovery.upnp/src/main/java/org/openhab/core/config/discovery/upnp/internal/UpnpDiscoveryService.java b/bundles/org.openhab.core.config.discovery.upnp/src/main/java/org/openhab/core/config/discovery/upnp/internal/UpnpDiscoveryService.java index 893297d0f..21e708b0f 100644 --- a/bundles/org.openhab.core.config.discovery.upnp/src/main/java/org/openhab/core/config/discovery/upnp/internal/UpnpDiscoveryService.java +++ b/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); diff --git a/bundles/org.openhab.core.config.discovery.usbserial/src/main/java/org/openhab/core/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java b/bundles/org.openhab.core.config.discovery.usbserial/src/main/java/org/openhab/core/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java index b3e85c405..3b67a4394 100644 --- a/bundles/org.openhab.core.config.discovery.usbserial/src/main/java/org/openhab/core/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java +++ b/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())); } } } diff --git a/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/AbstractDiscoveryService.java b/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/AbstractDiscoveryService.java index f30fbbc45..d6b130711 100644 --- a/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/AbstractDiscoveryService.java +++ b/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; diff --git a/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/DiscoveryResultBuilder.java b/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/DiscoveryResultBuilder.java index 1ab1acd96..bca0efbfa 100644 --- a/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/DiscoveryResultBuilder.java +++ b/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. * diff --git a/bundles/org.openhab.core.config.discovery/src/test/java/org/openhab/core/config/discovery/DiscoveryResultBuilderTest.java b/bundles/org.openhab.core.config.discovery/src/test/java/org/openhab/core/config/discovery/DiscoveryResultBuilderTest.java index 50a678341..0233f9a4b 100644 --- a/bundles/org.openhab.core.config.discovery/src/test/java/org/openhab/core/config/discovery/DiscoveryResultBuilderTest.java +++ b/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();