ChartServlet bug fixes and improvements (#2502)

* Improve exception handling * Add transparent themes * Add null annotations * Use java.time classes instead of Date and magic numbers * Upgrade XChart to 3.1.0 * Fix buggy legend position logic: Reinitialize counter to 0. So it does not work on legend position counter values of previously created charts. Use a local variable for the position counter instead of a field. This prevents issues when creating multiple charts simultanuously. For XChart release notes see: https://knowm.org/open-source/xchart/xchart-change-log/ On newer XChart versions there is an issue when using customized grid lines: knowm/XChart#628 Fixes #1183 Related to #2501 Supersedes #2415 Signed-off-by: Wouter Born <[email protected]>
openhab · Oct 9, 2021 · ac84206 · ac84206
1 parent fb8308a
commit ac84206
Show file tree

Hide file tree

Showing 14 changed files with 357 additions and 181 deletions.
diff --git a/bundles/org.openhab.core.io.rest/src/main/java/org/openhab/core/io/rest/RESTConstants.java b/bundles/org.openhab.core.io.rest/src/main/java/org/openhab/core/io/rest/RESTConstants.java
@@ -24,5 +24,5 @@ public class RESTConstants {
 
     public static final String JAX_RS_NAME = "openhab";
 
-    public static final String API_VERSION = "4";
+    public static final String API_VERSION = "5";
 }
diff --git a/bundles/org.openhab.core.ui/pom.xml b/bundles/org.openhab.core.ui/pom.xml
@@ -18,7 +18,8 @@
     <dependency>
       <groupId>org.knowm.xchart</groupId>
       <artifactId>xchart</artifactId>
-      <version>2.6.1</version>
+      <!-- Newer versions have issues with customized grid lines, see: https://github.com/knowm/XChart/issues/628 -->
+      <version>3.1.0</version>
     </dependency>
     <dependency>
       <groupId>org.openhab.core.bundles</groupId>

diff --git a/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/chart/ChartProvider.java b/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/chart/ChartProvider.java
@@ -13,8 +13,10 @@
 package org.openhab.core.ui.chart;
 
 import java.awt.image.BufferedImage;
-import java.util.Date;
+import java.time.ZonedDateTime;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.items.ItemNotFoundException;
 
 /**
@@ -25,6 +27,7 @@
  * @author Chris Jackson - Initial contribution
  * @author Holger Reichert - Support for themes, DPI, legend hiding
  */
+@NonNullByDefault
 public interface ChartProvider {
     /**
      * Gets the name of this chart provider.
@@ -56,8 +59,9 @@ public interface ChartProvider {
      * @throws ItemNotFoundException if an item or group is not found
      * @throws IllegalArgumentException if an invalid argument is passed
      */
-    BufferedImage createChart(String service, String theme, Date startTime, Date endTime, int height, int width,
-            String items, String groups, Integer dpi, Boolean legend) throws ItemNotFoundException;
+    BufferedImage createChart(@Nullable String service, @Nullable String theme, ZonedDateTime startTime,
+            ZonedDateTime endTime, int height, int width, @Nullable String items, @Nullable String groups,
+            @Nullable Integer dpi, @Nullable Boolean legend) throws ItemNotFoundException;
 
     /**
      * Gets the type of data that will be written by the chart.

diff --git a/...es/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/chart/ChartServlet.java b/...es/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/chart/ChartServlet.java
@@ -15,24 +15,27 @@
 import static java.util.Map.entry;
 
 import java.awt.image.BufferedImage;
-import java.io.EOFException;
 import java.io.IOException;
-import java.text.ParseException;
-import java.text.SimpleDateFormat;
-import java.util.Date;
+import java.time.Duration;
+import java.time.LocalDateTime;
+import java.time.ZonedDateTime;
+import java.time.format.DateTimeFormatter;
+import java.time.format.DateTimeParseException;
 import java.util.Map;
 import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
 
-import javax.imageio.IIOException;
 import javax.imageio.ImageIO;
 import javax.imageio.stream.ImageOutputStream;
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.config.core.ConfigurableService;
+import org.openhab.core.i18n.TimeZoneProvider;
 import org.openhab.core.io.http.servlet.OpenHABServlet;
 import org.openhab.core.items.ItemNotFoundException;
 import org.openhab.core.ui.chart.ChartProvider;
@@ -65,6 +68,7 @@
  * @author Chris Jackson - Initial contribution
  * @author Holger Reichert - Support for themes, DPI, legend hiding
  */
+@NonNullByDefault
 @Component(immediate = true, service = ChartServlet.class, configurationPid = "org.openhab.chart", //
         property = Constants.SERVICE_PID + "=org.openhab.chart")
 @ConfigurableService(category = "system", label = "Charts", description_uri = ChartServlet.CONFIG_URI)
@@ -76,6 +80,9 @@ public class ChartServlet extends OpenHABServlet {
     private static final int CHART_HEIGHT = 240;
     private static final int CHART_WIDTH = 480;
     private static final String DATE_FORMAT = "yyyyMMddHHmm";
+    private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern(DATE_FORMAT);
+
+    private final TimeZoneProvider timeZoneProvider;
 
     private String providerName = "default";
     private int defaultHeight = CHART_HEIGHT;
@@ -86,21 +93,25 @@ public class ChartServlet extends OpenHABServlet {
     // The URI of this servlet
     public static final String SERVLET_NAME = "/chart";
 
-    protected static final Map<String, Long> PERIODS = Map.ofEntries( //
-            entry("h", 3600000L), entry("4h", 14400000L), //
-            entry("8h", 28800000L), entry("12h", 43200000L), //
-            entry("D", 86400000L), entry("2D", 172800000L), //
-            entry("3D", 259200000L), entry("W", 604800000L), //
-            entry("2W", 1209600000L), entry("M", 2592000000L), //
-            entry("2M", 5184000000L), entry("4M", 10368000000L), //
-            entry("Y", 31536000000L)//
+    private static final Duration DEFAULT_PERIOD = Duration.ofDays(1);
+
+    private static final Map<String, Duration> PERIODS = Map.ofEntries( //
+            entry("h", Duration.ofHours(1)), entry("4h", Duration.ofHours(4)), //
+            entry("8h", Duration.ofHours(8)), entry("12h", Duration.ofHours(12)), //
+            entry("D", Duration.ofDays(1)), entry("2D", Duration.ofDays(2)), //
+            entry("3D", Duration.ofDays(3)), entry("W", Duration.ofDays(7)), //
+            entry("2W", Duration.ofDays(14)), entry("M", Duration.ofDays(30)), //
+            entry("2M", Duration.ofDays(60)), entry("4M", Duration.ofDays(120)), //
+            entry("Y", Duration.ofDays(365))//
     );
 
     protected static final Map<String, ChartProvider> CHART_PROVIDERS = new ConcurrentHashMap<>();
 
     @Activate
-    public ChartServlet(final @Reference HttpService httpService, final @Reference HttpContext httpContext) {
+    public ChartServlet(final @Reference HttpService httpService, final @Reference HttpContext httpContext,
+            final @Reference TimeZoneProvider timeZoneProvider) {
         super(httpService, httpContext);
+        this.timeZoneProvider = timeZoneProvider;
     }
 
     @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
@@ -128,7 +139,7 @@ protected void deactivate() {
     }
 
     @Modified
-    protected void modified(Map<String, Object> config) {
+    protected void modified(@Nullable Map<String, Object> config) {
         applyConfig(config);
     }
 
@@ -137,7 +148,7 @@ protected void modified(Map<String, Object> config) {
      *
      * @param config the configuration
      */
-    private void applyConfig(Map<String, Object> config) {
+    private void applyConfig(@Nullable Map<String, Object> config) {
         if (config == null) {
             return;
         }
@@ -224,22 +235,14 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) throws Ser
         }
 
         // Read out the parameter period, begin and end and save them.
-        Long period = null;
-        Date timeBegin = null;
-        Date timeEnd = null;
-
-        if (periodParam != null) {
-            period = PERIODS.get(periodParam);
-        }
-        if (period == null) {
-            // use a day as the default period
-            period = PERIODS.get("D");
-        }
+        Duration period = periodParam == null ? DEFAULT_PERIOD : PERIODS.getOrDefault(periodParam, DEFAULT_PERIOD);
+        ZonedDateTime timeBegin = null;
+        ZonedDateTime timeEnd = null;
 
         if (timeBeginParam != null) {
             try {
-                timeBegin = new SimpleDateFormat(DATE_FORMAT).parse(timeBeginParam);
-            } catch (ParseException e) {
+                timeBegin = LocalDateTime.parse(timeBeginParam, FORMATTER).atZone(timeZoneProvider.getTimeZone());
+            } catch (DateTimeParseException e) {
                 res.sendError(HttpServletResponse.SC_BAD_REQUEST,
                         "Begin and end must have this format: " + DATE_FORMAT + ".");
                 return;
@@ -248,8 +251,8 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) throws Ser
 
         if (timeEndParam != null) {
             try {
-                timeEnd = new SimpleDateFormat(DATE_FORMAT).parse(timeEndParam);
-            } catch (ParseException e) {
+                timeEnd = LocalDateTime.parse(timeEndParam, FORMATTER).atZone(timeZoneProvider.getTimeZone());
+            } catch (DateTimeParseException e) {
                 res.sendError(HttpServletResponse.SC_BAD_REQUEST,
                         "Begin and end must have this format: " + DATE_FORMAT + ".");
                 return;
@@ -258,17 +261,18 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) throws Ser
 
         // Set begin and end time and check legality.
         if (timeBegin == null && timeEnd == null) {
-            timeEnd = new Date();
-            timeBegin = new Date(timeEnd.getTime() - period);
+            timeEnd = ZonedDateTime.now(timeZoneProvider.getTimeZone());
+            timeBegin = timeEnd.minus(period);
             logger.debug("No begin or end is specified, use now as end and now-period as begin.");
         } else if (timeEnd == null) {
-            timeEnd = new Date(timeBegin.getTime() + period);
+            timeEnd = timeBegin.plus(period);
             logger.debug("No end is specified, use begin + period as end.");
         } else if (timeBegin == null) {
-            timeBegin = new Date(timeEnd.getTime() - period);
+            timeBegin = timeEnd.minus(period);
             logger.debug("No begin is specified, use end-period as begin");
-        } else if (timeEnd.before(timeBegin)) {
-            throw new ServletException("The end is before the begin.");
+        } else if (timeEnd.isBefore(timeBegin)) {
+            res.sendError(HttpServletResponse.SC_BAD_REQUEST, "The end is before the begin.");
+            return;
         }
 
         // If a persistence service is specified, find the provider
@@ -311,46 +315,43 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) throws Ser
             width = maxWidth;
         }
 
-        // Set the content type to that provided by the chart provider
-        res.setContentType("image/" + provider.getChartType());
         logger.debug("chart building with width {} height {} dpi {}", width, height, dpi);
-        try (ImageOutputStream imageOutputStream = ImageIO.createImageOutputStream(res.getOutputStream())) {
+        try {
             BufferedImage chart = provider.createChart(serviceName, req.getParameter("theme"), timeBegin, timeEnd,
                     height, width, req.getParameter("items"), req.getParameter("groups"), dpi, legend);
-            ImageIO.write(chart, provider.getChartType().toString(), imageOutputStream);
-            logger.debug("Chart successfully generated and written to the response.");
+            // Set the content type to that provided by the chart provider
+            res.setContentType("image/" + provider.getChartType());
+            try (ImageOutputStream imageOutputStream = ImageIO.createImageOutputStream(res.getOutputStream())) {
+                ImageIO.write(chart, provider.getChartType().toString(), imageOutputStream);
+                logger.debug("Chart successfully generated and written to the response.");
+            }
         } catch (ItemNotFoundException e) {
             logger.debug("{}", e.getMessage());
             res.sendError(HttpServletResponse.SC_BAD_REQUEST, e.getMessage());
         } catch (IllegalArgumentException e) {
             logger.warn("Illegal argument in chart: {}", e.getMessage());
             res.sendError(HttpServletResponse.SC_BAD_REQUEST, "Illegal argument in chart: " + e.getMessage());
-        } catch (IIOException | EOFException e) {
+        } catch (IOException e) {
             // this can happen if the request is terminated while the image is streamed, see
             // https://github.com/openhab/openhab-distro/issues/684
             logger.debug("Failed writing image to response stream", e);
         } catch (RuntimeException e) {
-            if (logger.isDebugEnabled()) {
-                // we also attach the stack trace
-                logger.warn("Chart generation failed: {}", e.getMessage(), e);
-            } else {
-                logger.warn("Chart generation failed: {}", e.getMessage());
-            }
+            logger.warn("Chart generation failed: {}", e.getMessage(), logger.isDebugEnabled() ? e : null);
             res.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, e.getMessage());
         }
     }
 
     @Override
-    public void init(ServletConfig config) throws ServletException {
+    public void init(@Nullable ServletConfig config) throws ServletException {
     }
 
     @Override
-    public ServletConfig getServletConfig() {
+    public @Nullable ServletConfig getServletConfig() {
         return null;
     }
 
     @Override
-    public String getServletInfo() {
+    public @Nullable String getServletInfo() {
         return null;
     }
 

diff --git a/....ui/src/main/java/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartTheme.java b/....ui/src/main/java/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartTheme.java
@@ -15,11 +15,14 @@
 import java.awt.Color;
 import java.awt.Font;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
 /**
  * Chart styling theme for the {@link DefaultChartProvider}.
  *
  * @author Holger Reichert - Initial contribution
  */
+@NonNullByDefault
 public interface ChartTheme {
 
     /**

diff --git a/...rc/main/java/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartThemeBlack.java b/...rc/main/java/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartThemeBlack.java
@@ -15,11 +15,14 @@
 import java.awt.Color;
 import java.awt.Font;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
 /**
  * Implementation of the black {@link ChartTheme chart theme}.
  *
  * @author Holger Reichert - Initial contribution
  */
+@NonNullByDefault
 public class ChartThemeBlack implements ChartTheme {
 
     private static final String THEME_NAME = "black";

diff --git a/...a/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartThemeBlackTransparent.java b/...a/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartThemeBlackTransparent.java
@@ -0,0 +1,43 @@
+/**
+ * Copyright (c) 2010-2021 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.ui.internal.chart.defaultchartprovider;
+
+import java.awt.Color;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
+/**
+ * Implementation of the black {@link ChartTheme chart theme} with transparent background.
+ *
+ * @author Wouter Born - Initial contribution
+ */
+@NonNullByDefault
+public class ChartThemeBlackTransparent extends ChartThemeBlack {
+
+    private static final String THEME_NAME = "black_transparent";
+
+    @Override
+    public String getThemeName() {
+        return THEME_NAME;
+    }
+
+    @Override
+    public Color getPlotBackgroundColor() {
+        return new Color(0, 0, 0, 0);
+    }
+
+    @Override
+    public Color getChartBackgroundColor() {
+        return new Color(0, 0, 0, 0);
+    }
+}
diff --git a/...c/main/java/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartThemeBright.java b/...c/main/java/org/openhab/core/ui/internal/chart/defaultchartprovider/ChartThemeBright.java
@@ -15,11 +15,14 @@
 import java.awt.Color;
 import java.awt.Font;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
 /**
  * Implementation of the default bright {@link ChartTheme chart theme}.
  *
  * @author Holger Reichert - Initial contribution
  */
+@NonNullByDefault
 public class ChartThemeBright implements ChartTheme {
 
     private static final String THEME_NAME = "bright";