Fix discovery exception (#17669)

Fixes #17668

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/AbstractWeatherHandler.java

@@ -29,10 +29,11 @@ import javax.measure.Unit;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.jetty.client.HttpClient;
 import org.openhab.binding.fmiweather.internal.client.Client;
 import org.openhab.binding.fmiweather.internal.client.Data;
+import org.openhab.binding.fmiweather.internal.client.FMIRequest;
 import org.openhab.binding.fmiweather.internal.client.FMIResponse;
-import org.openhab.binding.fmiweather.internal.client.Request;
 import org.openhab.binding.fmiweather.internal.client.exception.FMIResponseException;
 import org.openhab.binding.fmiweather.internal.client.exception.FMIUnexpectedResponseException;
 import org.openhab.core.library.types.DateTimeType;
@@ -68,6 +69,7 @@ public abstract class AbstractWeatherHandler extends BaseThingHandler {
 
     protected static final int TIMEOUT_MILLIS = 10_000;
     private final Logger logger = LoggerFactory.getLogger(AbstractWeatherHandler.class);
+    private final HttpClient httpClient;
 
     protected volatile @NonNullByDefault({}) Client client;
     protected final AtomicReference<@Nullable ScheduledFuture<?>> futureRef = new AtomicReference<>();
@@ -77,8 +79,9 @@ public abstract class AbstractWeatherHandler extends BaseThingHandler {
     private volatile long lastRefreshMillis = 0;
     private final AtomicReference<@Nullable ScheduledFuture<?>> updateChannelsFutureRef = new AtomicReference<>();
 
-    public AbstractWeatherHandler(Thing thing) {
+    public AbstractWeatherHandler(final Thing thing, final HttpClient httpClient) {
         super(thing);
+        this.httpClient = httpClient;
     }
 
     @Override
@@ -111,7 +114,7 @@ public abstract class AbstractWeatherHandler extends BaseThingHandler {
 
     @Override
     public void initialize() {
-        client = new Client();
+        client = new Client(httpClient);
         updateStatus(ThingStatus.UNKNOWN);
         rescheduleUpdate(0, false);
     }
@@ -136,7 +139,7 @@ public abstract class AbstractWeatherHandler extends BaseThingHandler {
 
     protected abstract void updateChannels();
 
-    protected abstract Request getRequest();
+    protected abstract FMIRequest getRequest();
 
     protected void update(int retry) {
         if (retry < RETRIES) {
@@ -145,6 +148,9 @@ public abstract class AbstractWeatherHandler extends BaseThingHandler {
             } catch (FMIResponseException e) {
                 handleError(e, retry);
                 return;
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                return;
             }
         } else {
             logger.trace("Query failed. Retries exhausted, not trying again until next poll.");

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/ForecastWeatherHandler.java

@@ -30,12 +30,13 @@ import javax.measure.Unit;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.jetty.client.HttpClient;
 import org.openhab.binding.fmiweather.internal.client.Data;
+import org.openhab.binding.fmiweather.internal.client.FMIRequest;
 import org.openhab.binding.fmiweather.internal.client.FMIResponse;
 import org.openhab.binding.fmiweather.internal.client.ForecastRequest;
 import org.openhab.binding.fmiweather.internal.client.LatLon;
 import org.openhab.binding.fmiweather.internal.client.Location;
-import org.openhab.binding.fmiweather.internal.client.Request;
 import org.openhab.binding.fmiweather.internal.client.exception.FMIUnexpectedResponseException;
 import org.openhab.binding.fmiweather.internal.config.ForecastConfiguration;
 import org.openhab.core.thing.Channel;
@@ -85,8 +86,8 @@ public class ForecastWeatherHandler extends AbstractWeatherHandler {
     private @NonNullByDefault({}) LatLon location;
     private String query = "";
 
-    public ForecastWeatherHandler(Thing thing) {
+    public ForecastWeatherHandler(final Thing thing, final HttpClient httpClient) {
-        super(thing);
+        super(thing, httpClient);
         // Override poll interval to slower value
         pollIntervalSeconds = (int) TimeUnit.MINUTES.toSeconds(QUERY_RESOLUTION_MINUTES);
     }
@@ -119,7 +120,7 @@ public class ForecastWeatherHandler extends AbstractWeatherHandler {
     }
 
     @Override
-    protected Request getRequest() {
+    protected FMIRequest getRequest() {
         long now = Instant.now().getEpochSecond();
         return new ForecastRequest(location, query, floorToEvenMinutes(now, QUERY_RESOLUTION_MINUTES),
                 ceilToEvenMinutes(now + TimeUnit.HOURS.toSeconds(FORECAST_HORIZON_HOURS), QUERY_RESOLUTION_MINUTES),

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/HandlerFactory.java

@@ -18,12 +18,17 @@ import java.util.Set;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.jetty.client.HttpClient;
+import org.openhab.core.io.net.http.HttpClientFactory;
 import org.openhab.core.thing.Thing;
 import org.openhab.core.thing.ThingTypeUID;
 import org.openhab.core.thing.binding.BaseThingHandlerFactory;
 import org.openhab.core.thing.binding.ThingHandler;
 import org.openhab.core.thing.binding.ThingHandlerFactory;
+import org.osgi.service.component.ComponentContext;
+import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
 
 /**
  * The {@link HandlerFactory} is responsible for creating things and thing
@@ -38,6 +43,14 @@ public class HandlerFactory extends BaseThingHandlerFactory {
     private static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(THING_TYPE_OBSERVATION,
             THING_TYPE_FORECAST);
 
+    private final HttpClient httpClient;
+
+    @Activate
+    public HandlerFactory(final @Reference HttpClientFactory httpClientFactory, ComponentContext componentContext) {
+        super.activate(componentContext);
+        this.httpClient = httpClientFactory.getCommonHttpClient();
+    }
+
     @Override
     public boolean supportsThingType(ThingTypeUID thingTypeUID) {
         return SUPPORTED_THING_TYPES_UIDS.contains(thingTypeUID);
@@ -48,9 +61,9 @@ public class HandlerFactory extends BaseThingHandlerFactory {
         ThingTypeUID thingTypeUID = thing.getThingTypeUID();
 
         if (THING_TYPE_OBSERVATION.equals(thingTypeUID)) {
-            return new ObservationWeatherHandler(thing);
+            return new ObservationWeatherHandler(thing, httpClient);
         } else if (THING_TYPE_FORECAST.equals(thingTypeUID)) {
-            return new ForecastWeatherHandler(thing);
+            return new ForecastWeatherHandler(thing, httpClient);
         }
 
         return null;

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/ObservationWeatherHandler.java

@@ -32,12 +32,13 @@ import javax.measure.quantity.Length;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.jetty.client.HttpClient;
 import org.openhab.binding.fmiweather.internal.client.Data;
+import org.openhab.binding.fmiweather.internal.client.FMIRequest;
 import org.openhab.binding.fmiweather.internal.client.FMIResponse;
 import org.openhab.binding.fmiweather.internal.client.FMISID;
 import org.openhab.binding.fmiweather.internal.client.Location;
 import org.openhab.binding.fmiweather.internal.client.ObservationRequest;
-import org.openhab.binding.fmiweather.internal.client.Request;
 import org.openhab.binding.fmiweather.internal.client.exception.FMIUnexpectedResponseException;
 import org.openhab.core.library.unit.MetricPrefix;
 import org.openhab.core.thing.Channel;
@@ -107,8 +108,8 @@ public class ObservationWeatherHandler extends AbstractWeatherHandler {
 
     private @NonNullByDefault({}) String fmisid;
 
-    public ObservationWeatherHandler(Thing thing) {
+    public ObservationWeatherHandler(final Thing thing, final HttpClient httpClient) {
-        super(thing);
+        super(thing, httpClient);
         pollIntervalSeconds = POLL_INTERVAL_SECONDS;
     }
 
@@ -124,7 +125,7 @@ public class ObservationWeatherHandler extends AbstractWeatherHandler {
     }
 
     @Override
-    protected Request getRequest() {
+    protected FMIRequest getRequest() {
         long now = Instant.now().getEpochSecond();
         return new ObservationRequest(new FMISID(fmisid),
                 floorToEvenMinutes(now - OBSERVATION_LOOK_BACK_SECONDS, STEP_MINUTES),

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/client/Client.java

@@ -19,6 +19,9 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.stream.IntStream;
 
 import javax.xml.namespace.NamespaceContext;
@@ -33,11 +36,15 @@ import javax.xml.xpath.XPathFactory;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.api.ContentResponse;
+import org.eclipse.jetty.client.api.Request;
+import org.eclipse.jetty.http.HttpMethod;
+import org.eclipse.jetty.http.HttpStatus;
 import org.openhab.binding.fmiweather.internal.client.FMIResponse.Builder;
 import org.openhab.binding.fmiweather.internal.client.exception.FMIExceptionReportException;
 import org.openhab.binding.fmiweather.internal.client.exception.FMIIOException;
 import org.openhab.binding.fmiweather.internal.client.exception.FMIUnexpectedResponseException;
-import org.openhab.core.io.net.http.HttpUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.w3c.dom.Document;
@@ -79,6 +86,7 @@ public class Client {
             "ef", "http://inspire.ec.europa.eu/schemas/ef/4.0");
 
     private final Logger logger = LoggerFactory.getLogger(Client.class);
+    private final HttpClient httpClient;
 
     private static final NamespaceContext NAMESPACE_CONTEXT = new NamespaceContext() {
         @Override
@@ -101,7 +109,9 @@ public class Client {
     private DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
     private DocumentBuilder documentBuilder;
 
-    public Client() {
+    public Client(final HttpClient httpClient) {
+        this.httpClient = httpClient;
+
         documentBuilderFactory.setNamespaceAware(true);
         try {
             // see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
@@ -126,20 +136,30 @@ public class Client {
      * @throws FMIUnexpectedResponseException on all unexpected content errors
      * @throws FMIExceptionReportException on explicit error responses from the server
      */
-    public FMIResponse query(Request request, int timeoutMillis)
+    public FMIResponse query(FMIRequest fmiRequest, int timeoutMillis)
-            throws FMIExceptionReportException, FMIUnexpectedResponseException, FMIIOException {
+            throws FMIExceptionReportException, FMIUnexpectedResponseException, FMIIOException, InterruptedException {
         try {
-            String url = request.toUrl();
+            String url = fmiRequest.toUrl();
             logger.trace("GET request for {}", url);
-            String responseText = HttpUtil.executeUrl("GET", url, timeoutMillis);
+            Request request = httpClient.newRequest(url).timeout(timeoutMillis, TimeUnit.MILLISECONDS)
-            if (responseText == null) {
+                    .method(HttpMethod.GET);
-                throw new FMIIOException(String.format("HTTP error with %s", request.toUrl()));
+
+            ContentResponse response = request.send();
+
+            int status = response.getStatus();
+            if (!HttpStatus.isSuccess(status)) {
+                throw new FMIIOException("The request failed with HTTP error " + status);
             }
-            logger.trace("Response content: '{}'", responseText);
+
-            FMIResponse response = parseMultiPointCoverageXml(responseText);
+            String responseContent = response.getContentAsString();
-            logger.debug("Request {} translated to url {}. Response: {}", request, url, response);
+            if (responseContent == null) {
-            return response;
+                throw new FMIIOException(String.format("HTTP error with %s", url));
-        } catch (IOException e) {
+            }
+            logger.trace("Response content: '{}'", responseContent);
+            FMIResponse fmiResponse = parseMultiPointCoverageXml(responseContent);
+            logger.debug("Request {} translated to url {}. Response: {}", fmiRequest, url, response);
+            return fmiResponse;
+        } catch (IOException | TimeoutException | ExecutionException e) {
             throw new FMIIOException(e);
         } catch (SAXException | XPathExpressionException e) {
             throw new FMIUnexpectedResponseException(e);
@@ -156,14 +176,24 @@ public class Client {
      * @throws FMIExceptionReportException on explicit error responses from the server
      */
     public Set<Location> queryWeatherStations(int timeoutMillis)
-            throws FMIIOException, FMIUnexpectedResponseException, FMIExceptionReportException {
+            throws FMIIOException, FMIUnexpectedResponseException, FMIExceptionReportException, InterruptedException {
         try {
-            String response = HttpUtil.executeUrl("GET", WEATHER_STATIONS_URL, timeoutMillis);
+            Request request = httpClient.newRequest(WEATHER_STATIONS_URL).timeout(timeoutMillis, TimeUnit.MILLISECONDS)
-            if (response == null) {
+                    .method(HttpMethod.GET);
+
+            ContentResponse response = request.send();
+
+            int status = response.getStatus();
+            if (!HttpStatus.isSuccess(status)) {
+                throw new FMIIOException("The request failed with HTTP error " + status);
+            }
+
+            String responseContent = response.getContentAsString();
+            if (responseContent == null) {
                 throw new FMIIOException(String.format("HTTP error with %s", WEATHER_STATIONS_URL));
             }
-            return parseStations(response);
+            return parseStations(responseContent);
-        } catch (IOException e) {
+        } catch (IOException | TimeoutException | ExecutionException e) {
             throw new FMIIOException(e);
         } catch (XPathExpressionException | SAXException e) {
             throw new FMIUnexpectedResponseException(e);

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/client/FMIRequest.java

@@ -26,7 +26,7 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
  *
  */
 @NonNullByDefault
-public class Request {
+public class FMIRequest {
 
     public static final String FMI_WFS_URL = "https://opendata.fmi.fi/wfs";
 
@@ -40,13 +40,13 @@ public class Request {
     private static final ZoneId UTC = ZoneId.of("Z");
     private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss'Z'");
 
-    public Request(String storedQueryId, QueryParameter location, long startEpoch, long endEpoch,
+    public FMIRequest(String storedQueryId, QueryParameter location, long startEpoch, long endEpoch,
             long timestepMinutes) {
         this(storedQueryId, location, startEpoch, endEpoch, timestepMinutes, new String[0]);
     }
 
-    public Request(String storedQueryId, QueryParameter location, long startEpoch, long endEpoch, long timestepMinutes,
+    public FMIRequest(String storedQueryId, QueryParameter location, long startEpoch, long endEpoch,
-            String[] parameters) {
+            long timestepMinutes, String[] parameters) {
         this.storedQueryId = storedQueryId;
         this.location = location;
         this.startEpoch = startEpoch;

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/client/ForecastRequest.java

@@ -22,7 +22,7 @@ import org.openhab.binding.fmiweather.internal.config.ForecastConfiguration;
  *
  */
 @NonNullByDefault
-public class ForecastRequest extends Request {
+public class ForecastRequest extends FMIRequest {
 
     public static final String STORED_QUERY_ID_HARMONIE = "fmi::forecast::harmonie::surface::point::multipointcoverage";
     public static final String STORED_QUERY_ID_EDITED = "fmi::forecast::edited::weather::scandinavia::point::multipointcoverage";

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/client/ObservationRequest.java

@@ -21,7 +21,7 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
  *
  */
 @NonNullByDefault
-public class ObservationRequest extends Request {
+public class ObservationRequest extends FMIRequest {
 
     public static final String STORED_QUERY_ID = "fmi::observations::weather::multipointcoverage";
 

bundles/org.openhab.binding.fmiweather/src/main/java/org/openhab/binding/fmiweather/internal/discovery/FMIWeatherDiscoveryService.java

@@ -28,6 +28,7 @@ import java.util.stream.Stream;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.jetty.client.HttpClient;
 import org.openhab.binding.fmiweather.internal.BindingConstants;
 import org.openhab.binding.fmiweather.internal.client.Client;
 import org.openhab.binding.fmiweather.internal.client.Location;
@@ -39,10 +40,12 @@ import org.openhab.core.config.discovery.DiscoveryResult;
 import org.openhab.core.config.discovery.DiscoveryResultBuilder;
 import org.openhab.core.config.discovery.DiscoveryService;
 import org.openhab.core.i18n.LocationProvider;
+import org.openhab.core.io.net.http.HttpClientFactory;
 import org.openhab.core.library.types.DecimalType;
 import org.openhab.core.library.types.PointType;
 import org.openhab.core.thing.ThingTypeUID;
 import org.openhab.core.thing.ThingUID;
+import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.Reference;
 import org.slf4j.Logger;
@@ -65,35 +68,45 @@ public class FMIWeatherDiscoveryService extends AbstractDiscoveryService {
     private static final int LOCATION_CHANGED_CHECK_INTERVAL_SECONDS = 60;
     private static final int FIND_STATION_METERS = 80_000;
 
+    private final LocationProvider locationProvider;
+    private final HttpClient httpClient;
+    private final ExpiringCache<Set<Location>> stationsCache;
+
     private @Nullable ScheduledFuture<?> discoveryJob;
     private @Nullable PointType previousLocation;
-    private @NonNullByDefault({}) LocationProvider locationProvider;
-
-    private ExpiringCache<Set<Location>> stationsCache = new ExpiringCache<>(STATIONS_CACHE_MILLIS, () -> {
-        try {
-            return new Client().queryWeatherStations(STATIONS_TIMEOUT_MILLIS);
-        } catch (FMIUnexpectedResponseException e) {
-            logger.warn("Unexpected error with the response, potentially API format has changed. Printing out details",
-                    e);
-        } catch (FMIResponseException e) {
-            logger.warn("Error when querying stations, {}: {}", e.getClass().getSimpleName(), e.getMessage());
-        }
-        // Return empty set on errors
-        return Collections.emptySet();
-    });
 
     /**
      * Creates a {@link FMIWeatherDiscoveryService} with immediately enabled background discovery.
      */
-    public FMIWeatherDiscoveryService() {
+    @Activate
+    public FMIWeatherDiscoveryService(final @Reference LocationProvider locationProvider,
+            final @Reference HttpClientFactory httpClientFactory) {
         super(SUPPORTED_THING_TYPES, DISCOVER_TIMEOUT_SECONDS, true);
+        this.locationProvider = locationProvider;
+        httpClient = httpClientFactory.getCommonHttpClient();
+
+        stationsCache = new ExpiringCache<>(STATIONS_CACHE_MILLIS, () -> {
+            try {
+                return new Client(httpClient).queryWeatherStations(STATIONS_TIMEOUT_MILLIS);
+            } catch (FMIUnexpectedResponseException e) {
+                logger.warn(
+                        "Unexpected error with the response, potentially API format has changed. Printing out details",
+                        e);
+            } catch (FMIResponseException e) {
+                logger.warn("Error when querying stations, {}: {}", e.getClass().getSimpleName(), e.getMessage());
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                logger.debug("Interrupted");
+            }
+            // Return empty set on errors
+            return Collections.emptySet();
+        });
     }
 
     @Override
     protected void startScan() {
         PointType location = null;
         logger.debug("Starting FMI Weather discovery scan");
-        LocationProvider locationProvider = getLocationProvider();
         location = locationProvider.getLocation();
         if (location == null) {
             logger.debug("LocationProvider.getLocation() is not set -> Will discover all stations");
@@ -208,17 +221,4 @@ public class FMIWeatherDiscoveryService extends AbstractDiscoveryService {
             }
         }
     }
-
-    @Reference
-    protected void setLocationProvider(LocationProvider locationProvider) {
-        this.locationProvider = locationProvider;
-    }
-
-    protected void unsetLocationProvider(LocationProvider provider) {
-        this.locationProvider = null;
-    }
-
-    protected LocationProvider getLocationProvider() {
-        return locationProvider;
-    }
 }

bundles/org.openhab.binding.fmiweather/src/test/java/org/openhab/binding/fmiweather/internal/AbstractFMIResponseParsingTest.java

@@ -12,6 +12,8 @@
  */
 package org.openhab.binding.fmiweather.internal;
 
+import static org.mockito.Mockito.mock;
+
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
@@ -22,6 +24,7 @@ import javax.xml.xpath.XPathExpressionException;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.jetty.client.HttpClient;
 import org.hamcrest.Description;
 import org.hamcrest.Matcher;
 import org.hamcrest.TypeSafeMatcher;
@@ -122,6 +125,10 @@ public class AbstractFMIResponseParsingTest {
     }
 
     protected class ClientExposed extends Client {
+        public ClientExposed() {
+            super(mock(HttpClient.class));
+        }
+
         public FMIResponse parseMultiPointCoverageXml(String response) throws FMIUnexpectedResponseException,
                 FMIExceptionReportException, SAXException, IOException, XPathExpressionException {
             return super.parseMultiPointCoverageXml(response);