From 7eb2c9fb811251271a7bd2a32572c62c90b1e590 Mon Sep 17 00:00:00 2001 From: Jacob Laursen Date: Tue, 15 Nov 2022 08:44:12 +0100 Subject: [PATCH] [jdbc] Improve error handling (#13719) * Enable wrapped exceptions being thrown by Yank Fixes #13718 Signed-off-by: Jacob Laursen * Fix SAT warning about hashCode implementation Signed-off-by: Jacob Laursen Signed-off-by: Jacob Laursen --- bundles/org.openhab.persistence.jdbc/pom.xml | 2 +- .../jdbc/console/JdbcCommandExtension.java | 39 +++++++---- .../openhab/persistence/jdbc/dto/ItemsVO.java | 7 +- .../persistence/jdbc/internal/JdbcMapper.java | 68 +++++++++---------- .../jdbc/internal/JdbcPersistenceService.java | 66 +++++++++++------- 5 files changed, 104 insertions(+), 78 deletions(-) diff --git a/bundles/org.openhab.persistence.jdbc/pom.xml b/bundles/org.openhab.persistence.jdbc/pom.xml index 7ad8183c0..e26f67e61 100644 --- a/bundles/org.openhab.persistence.jdbc/pom.xml +++ b/bundles/org.openhab.persistence.jdbc/pom.xml @@ -22,7 +22,7 @@ UTF-8 2.4.7 1.6 - 3.2.0 + 3.4.0 10.14.2.0 diff --git a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/console/JdbcCommandExtension.java b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/console/JdbcCommandExtension.java index 0bcab36a2..6923430aa 100644 --- a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/console/JdbcCommandExtension.java +++ b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/console/JdbcCommandExtension.java @@ -19,6 +19,7 @@ import java.util.stream.Stream; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; +import org.knowm.yank.exceptions.YankSQLException; import org.openhab.core.io.console.Console; import org.openhab.core.io.console.ConsoleCommandCompleter; import org.openhab.core.io.console.StringsCompleter; @@ -70,22 +71,14 @@ public class JdbcCommandExtension extends AbstractConsoleCommandExtension implem if (persistenceService == null) { return; } - if (SUBCMD_TABLES_LIST.equalsIgnoreCase(args[1])) { - listTables(persistenceService, console, args.length == 3 && PARAMETER_ALL.equalsIgnoreCase(args[2])); - return; - } else if (SUBCMD_TABLES_CLEAN.equalsIgnoreCase(args[1])) { - if (args.length == 3) { - cleanupItem(persistenceService, console, args[2], false); - return; - } else if (args.length == 4 && PARAMETER_FORCE.equalsIgnoreCase(args[3])) { - cleanupItem(persistenceService, console, args[2], true); - return; - } else { - cleanupTables(persistenceService, console); + try { + if (!execute(persistenceService, args, console)) { + printUsage(console); return; } + } catch (YankSQLException e) { + console.println(e.toString()); } - printUsage(console); } private @Nullable JdbcPersistenceService getPersistenceService() { @@ -97,12 +90,32 @@ public class JdbcCommandExtension extends AbstractConsoleCommandExtension implem return null; } + private boolean execute(JdbcPersistenceService persistenceService, String[] args, Console console) { + if (SUBCMD_TABLES_LIST.equalsIgnoreCase(args[1])) { + listTables(persistenceService, console, args.length == 3 && PARAMETER_ALL.equalsIgnoreCase(args[2])); + return true; + } else if (SUBCMD_TABLES_CLEAN.equalsIgnoreCase(args[1])) { + if (args.length == 3) { + cleanupItem(persistenceService, console, args[2], false); + return true; + } else if (args.length == 4 && PARAMETER_FORCE.equalsIgnoreCase(args[3])) { + cleanupItem(persistenceService, console, args[2], true); + return true; + } else { + cleanupTables(persistenceService, console); + return true; + } + } + return false; + } + private void listTables(JdbcPersistenceService persistenceService, Console console, Boolean all) { List entries = persistenceService.getCheckedEntries(); if (!all) { entries.removeIf(t -> t.getStatus() == ItemTableCheckEntryStatus.VALID); } entries.sort(Comparator.comparing(ItemTableCheckEntry::getTableName)); + // FIXME: NoSuchElement when empty table - because of get() int itemNameMaxLength = Math .max(entries.stream().map(t -> t.getItemName().length()).max(Integer::compare).get(), 4); int tableNameMaxLength = Math diff --git a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/dto/ItemsVO.java b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/dto/ItemsVO.java index 0e203f598..9f8f7da3c 100644 --- a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/dto/ItemsVO.java +++ b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/dto/ItemsVO.java @@ -13,6 +13,7 @@ package org.openhab.persistence.jdbc.dto; import java.io.Serializable; +import java.util.Objects; /** * Represents the table naming data. @@ -96,11 +97,7 @@ public class ItemsVO implements Serializable { */ @Override public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + ((itemName == null) ? 0 : itemName.hashCode()); - result = prime * result + (itemId ^ (itemId >>> 32)); - return result; + return Objects.hash(itemName, itemId); } /* diff --git a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcMapper.java b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcMapper.java index c956e706e..cae0a5346 100644 --- a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcMapper.java +++ b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcMapper.java @@ -25,6 +25,7 @@ import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; import org.knowm.yank.Yank; +import org.knowm.yank.exceptions.YankSQLException; import org.openhab.core.i18n.TimeZoneProvider; import org.openhab.core.items.Item; import org.openhab.core.persistence.FilterCriteria; @@ -66,7 +67,7 @@ public class JdbcMapper { /***************** * MAPPER ITEMS * *****************/ - public boolean pingDB() { + private boolean pingDB() { logger.debug("JDBC::pingDB"); boolean ret = false; long timerStart = System.currentTimeMillis(); @@ -90,15 +91,7 @@ public class JdbcMapper { return ret; } - public String getDB() { - logger.debug("JDBC::getDB"); - long timerStart = System.currentTimeMillis(); - String res = conf.getDBDAO().doGetDB(); - logTime("getDB", timerStart, System.currentTimeMillis()); - return res != null ? res : ""; - } - - public boolean ifItemsTableExists() { + private boolean ifItemsTableExists() { logger.debug("JDBC::ifItemsTableExists"); long timerStart = System.currentTimeMillis(); boolean res = conf.getDBDAO().doIfTableExists(new ItemsVO()); @@ -106,7 +99,7 @@ public class JdbcMapper { return res; } - public boolean ifTableExists(String tableName) { + protected boolean ifTableExists(String tableName) { logger.debug("JDBC::ifTableExists"); long timerStart = System.currentTimeMillis(); boolean res = conf.getDBDAO().doIfTableExists(tableName); @@ -114,7 +107,7 @@ public class JdbcMapper { return res; } - public ItemsVO createNewEntryInItemsTable(ItemsVO vo) { + private ItemsVO createNewEntryInItemsTable(ItemsVO vo) { logger.debug("JDBC::createNewEntryInItemsTable"); long timerStart = System.currentTimeMillis(); Long i = conf.getDBDAO().doCreateNewEntryInItemsTable(vo); @@ -123,7 +116,7 @@ public class JdbcMapper { return vo; } - public boolean createItemsTableIfNot(ItemsVO vo) { + private boolean createItemsTableIfNot(ItemsVO vo) { logger.debug("JDBC::createItemsTableIfNot"); long timerStart = System.currentTimeMillis(); conf.getDBDAO().doCreateItemsTableIfNot(vo); @@ -131,7 +124,7 @@ public class JdbcMapper { return true; } - public boolean dropItemsTableIfExists(ItemsVO vo) { + private boolean dropItemsTableIfExists(ItemsVO vo) { logger.debug("JDBC::dropItemsTableIfExists"); long timerStart = System.currentTimeMillis(); conf.getDBDAO().doDropItemsTableIfExists(vo); @@ -139,14 +132,14 @@ public class JdbcMapper { return true; } - public void dropTable(String tableName) { + protected void dropTable(String tableName) { logger.debug("JDBC::dropTable"); long timerStart = System.currentTimeMillis(); conf.getDBDAO().doDropTable(tableName); logTime("doDropTable", timerStart, System.currentTimeMillis()); } - public ItemsVO deleteItemsEntry(ItemsVO vo) { + protected ItemsVO deleteItemsEntry(ItemsVO vo) { logger.debug("JDBC::deleteItemsEntry"); long timerStart = System.currentTimeMillis(); conf.getDBDAO().doDeleteItemsEntry(vo); @@ -154,7 +147,7 @@ public class JdbcMapper { return vo; } - public List getItemIDTableNames() { + private List getItemIDTableNames() { logger.debug("JDBC::getItemIDTableNames"); long timerStart = System.currentTimeMillis(); List vo = conf.getDBDAO().doGetItemIDTableNames(new ItemsVO()); @@ -162,7 +155,7 @@ public class JdbcMapper { return vo; } - public List getItemTables() { + protected List getItemTables() { logger.debug("JDBC::getItemTables"); long timerStart = System.currentTimeMillis(); ItemsVO vo = new ItemsVO(); @@ -175,14 +168,14 @@ public class JdbcMapper { /**************** * MAPPERS ITEM * ****************/ - public void updateItemTableNames(List vol) { + private void updateItemTableNames(List vol) { logger.debug("JDBC::updateItemTableNames"); long timerStart = System.currentTimeMillis(); conf.getDBDAO().doUpdateItemTableNames(vol); logTime("updateItemTableNames", timerStart, System.currentTimeMillis()); } - public ItemVO createItemTable(ItemVO vo) { + private ItemVO createItemTable(ItemVO vo) { logger.debug("JDBC::createItemTable"); long timerStart = System.currentTimeMillis(); conf.getDBDAO().doCreateItemTable(vo); @@ -190,7 +183,7 @@ public class JdbcMapper { return vo; } - public Item storeItemValue(Item item, State itemState, @Nullable ZonedDateTime date) { + protected Item storeItemValue(Item item, State itemState, @Nullable ZonedDateTime date) { logger.debug("JDBC::storeItemValue: item={} state={} date={}", item, itemState, date); String tableName = getTable(item); long timerStart = System.currentTimeMillis(); @@ -208,7 +201,7 @@ public class JdbcMapper { return conf.getDBDAO().doGetRowCount(tableName); } - public List getHistItemFilterQuery(FilterCriteria filter, int numberDecimalcount, String table, + protected List getHistItemFilterQuery(FilterCriteria filter, int numberDecimalcount, String table, Item item) { logger.debug( "JDBC::getHistItemFilterQuery filter='{}' numberDecimalcount='{}' table='{}' item='{}' itemName='{}'", @@ -221,13 +214,12 @@ public class JdbcMapper { return result; } - public boolean deleteItemValues(FilterCriteria filter, String table) { + protected void deleteItemValues(FilterCriteria filter, String table) { logger.debug("JDBC::deleteItemValues filter='{}' table='{}' itemName='{}'", true, table, filter.getItemName()); long timerStart = System.currentTimeMillis(); conf.getDBDAO().doDeleteItemValues(filter, table, timeZoneProvider.getTimeZone()); logTime("deleteItemValues", timerStart, System.currentTimeMillis()); errCnt = 0; - return true; } /*********************** @@ -239,6 +231,7 @@ public class JdbcMapper { logger.info("JDBC::openConnection: Driver is available::Yank setupDataSource"); try { Yank.setupDefaultConnectionPool(conf.getHikariConfiguration()); + Yank.setThrowWrappedExceptions(true); conf.setDbConnected(true); return true; } catch (PoolInitializationException e) { @@ -271,16 +264,21 @@ public class JdbcMapper { if (initialized) { return true; } - // first - boolean p = pingDB(); - if (p) { - logger.debug("JDBC::checkDBAcessability, first try connection: {}", p); - return (p && !(conf.getErrReconnectThreshold() > 0 && errCnt <= conf.getErrReconnectThreshold())); - } else { - // second - p = pingDB(); - logger.debug("JDBC::checkDBAcessability, second try connection: {}", p); - return (p && !(conf.getErrReconnectThreshold() > 0 && errCnt <= conf.getErrReconnectThreshold())); + try { + // first + boolean p = pingDB(); + if (p) { + logger.debug("JDBC::checkDBAcessability, first try connection: {}", p); + return (p && !(conf.getErrReconnectThreshold() > 0 && errCnt <= conf.getErrReconnectThreshold())); + } else { + // second + p = pingDB(); + logger.debug("JDBC::checkDBAcessability, second try connection: {}", p); + return (p && !(conf.getErrReconnectThreshold() > 0 && errCnt <= conf.getErrReconnectThreshold())); + } + } catch (YankSQLException e) { + logger.warn("Unable to ping database", e); + return false; } } @@ -412,7 +410,7 @@ public class JdbcMapper { initialized = tmpinit; } - public Set getItems() { + protected Set getItems() { // TODO: in general it would be possible to query the count, earliest and latest values for each item too but it // would be a very costly operation return itemNameToTableNameMap.keySet().stream().map(itemName -> new JdbcPersistenceItemInfo(itemName)) diff --git a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcPersistenceService.java b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcPersistenceService.java index d267ad097..e0fbf1155 100644 --- a/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcPersistenceService.java +++ b/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcPersistenceService.java @@ -25,6 +25,7 @@ import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; +import org.knowm.yank.exceptions.YankSQLException; import org.openhab.core.config.core.ConfigurableService; import org.openhab.core.i18n.TimeZoneProvider; import org.openhab.core.items.GroupItem; @@ -155,11 +156,15 @@ public class JdbcPersistenceService extends JdbcMapper implements ModifiablePers state, item, errCnt, conf.getErrReconnectThreshold()); return; } - long timerStart = System.currentTimeMillis(); - storeItemValue(item, state, date); - if (logger.isDebugEnabled()) { - logger.debug("JDBC: Stored item '{}' as '{}' in SQL database at {} in {} ms.", item.getName(), state, - new Date(), System.currentTimeMillis() - timerStart); + try { + long timerStart = System.currentTimeMillis(); + storeItemValue(item, state, date); + if (logger.isDebugEnabled()) { + logger.debug("JDBC: Stored item '{}' as '{}' in SQL database at {} in {} ms.", item.getName(), state, + new Date(), System.currentTimeMillis() - timerStart); + } + } catch (YankSQLException e) { + logger.warn("JDBC::store: Unable to store item", e); } } @@ -215,16 +220,20 @@ public class JdbcPersistenceService extends JdbcMapper implements ModifiablePers return List.of(); } - long timerStart = System.currentTimeMillis(); - List items = getHistItemFilterQuery(filter, conf.getNumberDecimalcount(), table, item); - if (logger.isDebugEnabled()) { - logger.debug("JDBC: Query for item '{}' returned {} rows in {} ms", itemName, items.size(), - System.currentTimeMillis() - timerStart); + try { + long timerStart = System.currentTimeMillis(); + List items = getHistItemFilterQuery(filter, conf.getNumberDecimalcount(), table, item); + if (logger.isDebugEnabled()) { + logger.debug("JDBC: Query for item '{}' returned {} rows in {} ms", itemName, items.size(), + System.currentTimeMillis() - timerStart); + } + // Success + errCnt = 0; + return items; + } catch (YankSQLException e) { + logger.warn("JDBC::query: Unable to query item", e); + return List.of(); } - - // Success - errCnt = 0; - return items; } public void updateConfig(Map configuration) { @@ -233,9 +242,14 @@ public class JdbcPersistenceService extends JdbcMapper implements ModifiablePers conf = new JdbcConfiguration(configuration); if (conf.valid && checkDBAccessability()) { namingStrategy = new NamingStrategy(conf); - checkDBSchema(); - // connection has been established ... initialization completed! - initialized = true; + try { + checkDBSchema(); + // connection has been established ... initialization completed! + initialized = true; + } catch (YankSQLException e) { + logger.error("Failed to check database schema", e); + initialized = false; + } } else { initialized = false; } @@ -269,14 +283,18 @@ public class JdbcPersistenceService extends JdbcMapper implements ModifiablePers return false; } - long timerStart = System.currentTimeMillis(); - boolean result = deleteItemValues(filter, table); - if (logger.isDebugEnabled()) { - logger.debug("JDBC: Deleted values for item '{}' in SQL database at {} in {} ms.", itemName, new Date(), - System.currentTimeMillis() - timerStart); + try { + long timerStart = System.currentTimeMillis(); + deleteItemValues(filter, table); + if (logger.isDebugEnabled()) { + logger.debug("JDBC: Deleted values for item '{}' in SQL database at {} in {} ms.", itemName, new Date(), + System.currentTimeMillis() - timerStart); + } + return true; + } catch (YankSQLException e) { + logger.debug("JDBC::remove: Unable to remove values for item", e); + return false; } - - return result; } /**