From c6cece9dc113cd6717814c451cc31f08f913c279 Mon Sep 17 00:00:00 2001 From: "Jan N. Klug" Date: Sun, 27 Mar 2022 20:26:37 +0200 Subject: [PATCH] further work Signed-off-by: Jan N. Klug --- .../core/persistence/PersistenceManager.java | 3 +- .../PersistenceServiceConfigurationDTO.java | 4 +- .../internal/PersistenceManager.java | 120 ++++++++++-------- ...fiablePersistenceServiceConfiguration.java | 14 +- .../strategy/PersistenceStrategy.java | 2 +- .../internal/PersistenceManagerTest.java | 16 +-- 6 files changed, 81 insertions(+), 78 deletions(-) diff --git a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceManager.java b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceManager.java index 65d2ed62f83..7355e28d5ad 100644 --- a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceManager.java +++ b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceManager.java @@ -13,13 +13,12 @@ package org.openhab.core.persistence; import org.eclipse.jdt.annotation.NonNullByDefault; -import org.openhab.core.persistence.registry.PersistenceServiceConfigurationProvider; /** * A persistence manager service which could be used to start event handling or supply configuration for * persistence services. * - * @deprecated implement a {@link PersistenceServiceConfigurationProvider} instead + * @deprecated implement a {@link org.openhab.core.persistence.registry.PersistenceServiceConfigurationProvider} instead * * @author Markus Rathgeb - Initial contribution */ diff --git a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/dto/PersistenceServiceConfigurationDTO.java b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/dto/PersistenceServiceConfigurationDTO.java index ba831a2d218..84147c9dc82 100644 --- a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/dto/PersistenceServiceConfigurationDTO.java +++ b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/dto/PersistenceServiceConfigurationDTO.java @@ -19,7 +19,7 @@ import org.eclipse.jdt.annotation.Nullable; /** - * The {@link PersistenceServiceConfigurationDTO} is a + * The {@link PersistenceServiceConfigurationDTO} is used for transferring persistence service configurations * * @author Jan N. Klug - Initial contribution */ @@ -33,7 +33,7 @@ public class PersistenceServiceConfigurationDTO { public static class PersistenceItemConfigurationDTO { public Collection items = List.of(); - public @Nullable Collection strategies; + public Collection strategies = List.of(); public @Nullable String alias; } } diff --git a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManager.java b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManager.java index 8b197f71599..bd85b3fbc9e 100644 --- a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManager.java +++ b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManager.java @@ -28,6 +28,7 @@ import java.util.stream.Stream; import org.eclipse.jdt.annotation.NonNullByDefault; +import org.eclipse.jdt.annotation.Nullable; import org.openhab.core.common.NamedThreadFactory; import org.openhab.core.common.SafeCaller; import org.openhab.core.items.GenericItem; @@ -117,7 +118,7 @@ protected void deactivate() { persistenceServiceConfigurationRegistry.removeRegistryChangeListener(this); started = false; - persistenceServiceContainers.values().forEach(PersistenceServiceContainer::clearCronJobs); + persistenceServiceContainers.values().forEach(PersistenceServiceContainer::cancelPersistJobs); // remove item state change listeners itemRegistry.stream().filter(GenericItem.class::isInstance) @@ -128,15 +129,14 @@ protected void deactivate() { protected void addPersistenceService(PersistenceService persistenceService) { String serviceId = persistenceService.getId(); logger.debug("Initializing {} persistence service.", serviceId); - IdentifiablePersistenceServiceConfiguration serviceConfiguration = Objects.requireNonNullElse( - persistenceServiceConfigurationRegistry.get(serviceId), getDefaultConfig(persistenceService)); PersistenceServiceContainer container = new PersistenceServiceContainer(persistenceService, - serviceConfiguration); + persistenceServiceConfigurationRegistry.get(serviceId)); PersistenceServiceContainer oldContainer = persistenceServiceContainers.put(serviceId, container); - if (oldContainer != null) { - oldContainer.clearCronJobs(); + if (oldContainer != null) { // cancel all jobs if the persistence service is set and an old configuration is + // already present + oldContainer.cancelPersistJobs(); } if (started) { @@ -147,7 +147,7 @@ protected void addPersistenceService(PersistenceService persistenceService) { protected void removePersistenceService(PersistenceService persistenceService) { PersistenceServiceContainer container = persistenceServiceContainers.remove(persistenceService.getId()); if (container != null) { - container.clearCronJobs(); + container.cancelPersistJobs(); } } @@ -170,26 +170,23 @@ private void handleStateEvent(Item item, boolean changed) { /** * Checks if a given persistence configuration entry is relevant for an item * - * @param config the persistence configuration entry + * @param itemConfig the persistence configuration entry * @param item to check if the configuration applies to * @return true, if the configuration applies to the item */ - private boolean appliesToItem(PersistenceItemConfiguration config, Item item) { - for (PersistenceConfig itemCfg : config.getItems()) { + private boolean appliesToItem(PersistenceItemConfiguration itemConfig, Item item) { + for (PersistenceConfig itemCfg : itemConfig.getItems()) { if (itemCfg instanceof PersistenceAllConfig) { return true; } else if (itemCfg instanceof PersistenceItemConfig) { - PersistenceItemConfig singleItemConfig = (PersistenceItemConfig) itemCfg; - if (item.getName().equals(singleItemConfig.getItem())) { + if (item.getName().equals(((PersistenceItemConfig) itemCfg).getItem())) { return true; } } else if (itemCfg instanceof PersistenceGroupConfig) { - PersistenceGroupConfig groupItemConfig = (PersistenceGroupConfig) itemCfg; try { - Item gItem = itemRegistry.getItem(groupItemConfig.getGroup()); + Item gItem = itemRegistry.getItem(((PersistenceGroupConfig) itemCfg).getGroup()); if (gItem instanceof GroupItem) { - GroupItem groupItem = (GroupItem) gItem; - return groupItem.getAllMembers().contains(item); + return ((GroupItem) gItem).getAllMembers().contains(item); } } catch (ItemNotFoundException e) { // do nothing @@ -246,7 +243,6 @@ private Iterable getAllItems(PersistenceItemConfiguration config) { * * @param item the item to restore the state for */ - @SuppressWarnings("null") private void restoreItemStateIfNeeded(Item item) { // get the last persisted state from the persistence service if no state is yet set if (UnDefType.NULL.equals(item.getState()) && item instanceof GenericItem) { @@ -260,27 +256,24 @@ private void restoreItemStateIfNeeded(Item item) { QueryablePersistenceService queryService = (QueryablePersistenceService) container .getPersistenceService(); FilterCriteria filter = new FilterCriteria().setItemName(item.getName()).setPageSize(1); - Iterable result = safeCaller.create(queryService, QueryablePersistenceService.class) + Iterator result = safeCaller.create(queryService, QueryablePersistenceService.class) .onTimeout(() -> logger.warn("Querying persistence service '{}' takes more than {}ms.", queryService.getId(), SafeCaller.DEFAULT_TIMEOUT)) .onException(e -> logger.error("Exception occurred while querying persistence service '{}': {}", queryService.getId(), e.getMessage(), e)) - .build().query(filter); - if (result != null) { - Iterator it = result.iterator(); - if (it.hasNext()) { - HistoricItem historicItem = it.next(); - GenericItem genericItem = (GenericItem) item; - genericItem.removeStateChangeListener(this); - genericItem.setState(historicItem.getState()); - genericItem.addStateChangeListener(this); - if (logger.isDebugEnabled()) { - logger.debug("Restored item state from '{}' for item '{}' -> '{}'", - DateTimeFormatter.ISO_ZONED_DATE_TIME.format(historicItem.getTimestamp()), - item.getName(), historicItem.getState()); - } - return; + .build().query(filter).iterator(); + if (result.hasNext()) { + HistoricItem historicItem = result.next(); + GenericItem genericItem = (GenericItem) item; + genericItem.removeStateChangeListener(this); + genericItem.setState(historicItem.getState()); + genericItem.addStateChangeListener(this); + if (logger.isDebugEnabled()) { + logger.debug("Restored item state from '{}' for item '{}' -> '{}'", + DateTimeFormatter.ISO_ZONED_DATE_TIME.format(historicItem.getTimestamp()), + item.getName(), historicItem.getState()); } + return; } } } @@ -289,15 +282,7 @@ private void restoreItemStateIfNeeded(Item item) { private void startEventHandling(PersistenceServiceContainer serviceContainer) { serviceContainer.getMatchingConfigurations(PersistenceStrategy.Globals.RESTORE) .forEach(itemConfig -> getAllItems(itemConfig).forEach(this::restoreItemStateIfNeeded)); - serviceContainer.startCronJobs(); - } - - private IdentifiablePersistenceServiceConfiguration getDefaultConfig(PersistenceService persistenceService) { - List strategies = persistenceService.getDefaultStrategies(); - List configs = List - .of(new PersistenceItemConfiguration(List.of(new PersistenceAllConfig()), null, strategies, null)); - return new IdentifiablePersistenceServiceConfiguration(persistenceService.getId(), configs, strategies, - strategies); + serviceContainer.schedulePersistJobs(); } // ItemStateChangeListener methods @@ -362,7 +347,6 @@ public void onReadyMarkerRemoved(ReadyMarker readyMarker) { public void added(IdentifiablePersistenceServiceConfiguration element) { PersistenceServiceContainer container = persistenceServiceContainers.get(element.getUID()); if (container != null) { - container.clearCronJobs(); container.setConfiguration(element); if (started) { startEventHandling(container); @@ -374,8 +358,7 @@ public void added(IdentifiablePersistenceServiceConfiguration element) { public void removed(IdentifiablePersistenceServiceConfiguration element) { PersistenceServiceContainer container = persistenceServiceContainers.get(element.getUID()); if (container != null) { - container.clearCronJobs(); - container.setConfiguration(getDefaultConfig(container.persistenceService)); + container.setConfiguration(null); if (started) { startEventHandling(container); } @@ -396,19 +379,32 @@ private class PersistenceServiceContainer { private IdentifiablePersistenceServiceConfiguration configuration; public PersistenceServiceContainer(PersistenceService persistenceService, - IdentifiablePersistenceServiceConfiguration configuration) { + @Nullable IdentifiablePersistenceServiceConfiguration configuration) { this.persistenceService = persistenceService; - this.configuration = configuration; + this.configuration = Objects.requireNonNullElseGet(configuration, this::getDefaultConfig); } public PersistenceService getPersistenceService() { return persistenceService; } - public IdentifiablePersistenceServiceConfiguration getConfiguration() { - return configuration; + /** + * Set a new configuration for this persistence service (also cancels all cron jobs) + * + * @param configuration the new {@link IdentifiablePersistenceServiceConfiguration}, if {@code null} the default + * configuration of the service is used + */ + public void setConfiguration(@Nullable IdentifiablePersistenceServiceConfiguration configuration) { + cancelPersistJobs(); + this.configuration = Objects.requireNonNullElseGet(configuration, this::getDefaultConfig); } + /** + * Get all item configurations from this service that match a certain strategy + * + * @param strategy the {@link PersistenceStrategy} to look for + * @return a @link Stream} of the result + */ public Stream getMatchingConfigurations(PersistenceStrategy strategy) { boolean matchesDefaultStrategies = configuration.getDefaults().contains(strategy); return configuration.getConfigs().stream() @@ -416,11 +412,18 @@ public Stream getMatchingConfigurations(Persistenc || (itemConfig.getStrategies().isEmpty() && matchesDefaultStrategies)); } - public void setConfiguration(IdentifiablePersistenceServiceConfiguration configuration) { - this.configuration = configuration; + private IdentifiablePersistenceServiceConfiguration getDefaultConfig() { + List strategies = persistenceService.getDefaultStrategies(); + List configs = List + .of(new PersistenceItemConfiguration(List.of(new PersistenceAllConfig()), null, strategies, null)); + return new IdentifiablePersistenceServiceConfiguration(persistenceService.getId(), configs, strategies, + strategies); } - public void clearCronJobs() { + /** + * Cancel all scheduled cron jobs / strategies for this service + */ + public void cancelPersistJobs() { synchronized (jobs) { jobs.forEach(job -> job.cancel(true)); jobs.clear(); @@ -428,12 +431,17 @@ public void clearCronJobs() { logger.debug("Removed scheduled cron job for persistence service '{}'", configuration.getUID()); } - public void startCronJobs() { + /** + * Schedule all necessary cron jobs / strategies for this service + */ + public void schedulePersistJobs() { configuration.getStrategies().stream().filter(PersistenceCronStrategy.class::isInstance) .forEach(strategy -> { PersistenceCronStrategy cronStrategy = (PersistenceCronStrategy) strategy; String cronExpression = cronStrategy.getCronExpression(); - jobs.add(scheduler.schedule(() -> persistJob(cronStrategy), cronExpression)); + List itemConfigs = getMatchingConfigurations(strategy) + .collect(Collectors.toList()); + jobs.add(scheduler.schedule(() -> persistJob(itemConfigs), cronExpression)); logger.debug("Scheduled strategy {} with cron expression {} for service {}", cronStrategy.getName(), cronExpression, configuration.getUID()); @@ -441,8 +449,8 @@ public void startCronJobs() { }); } - private void persistJob(PersistenceStrategy strategy) { - getMatchingConfigurations(strategy).forEach(itemConfig -> { + private void persistJob(List itemConfigs) { + itemConfigs.forEach(itemConfig -> { for (Item item : getAllItems(itemConfig)) { long startTime = System.nanoTime(); persistenceService.store(item, itemConfig.getAlias()); diff --git a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/registry/IdentifiablePersistenceServiceConfiguration.java b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/registry/IdentifiablePersistenceServiceConfiguration.java index 9cb60acd22a..998be3c4af5 100644 --- a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/registry/IdentifiablePersistenceServiceConfiguration.java +++ b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/registry/IdentifiablePersistenceServiceConfiguration.java @@ -67,10 +67,8 @@ public PersistenceServiceConfigurationDTO toDTO() { PersistenceServiceConfigurationDTO.PersistenceItemConfigurationDTO itemDto = new PersistenceServiceConfigurationDTO.PersistenceItemConfigurationDTO(); itemDto.items = config.getItems().stream().map(this::persistenceConfigToString) .collect(Collectors.toList()); - Collection strategies = config.getStrategies(); - if (strategies != null) { - itemDto.strategies = strategies.stream().map(PersistenceStrategy::getName).collect(Collectors.toList()); - } + itemDto.strategies = config.getStrategies().stream().map(PersistenceStrategy::getName) + .collect(Collectors.toList()); itemDto.alias = config.getAlias(); return itemDto; }).collect(Collectors.toList()); @@ -106,14 +104,14 @@ public static IdentifiablePersistenceServiceConfiguration fromDTO(PersistenceSer List defaults = dto.defaults.stream() .map(str -> stringToPersistenceStrategy(str, strategyMap, dto.serviceId)).collect(Collectors.toList()); - List configs = dto.configs.stream().map(c -> { - List items = c.items.stream() + List configs = dto.configs.stream().map(config -> { + List items = config.items.stream() .map(IdentifiablePersistenceServiceConfiguration::stringToPersistenceConfig) .collect(Collectors.toList()); - List strategies = c.strategies.stream() + List strategies = config.strategies.stream() .map(str -> stringToPersistenceStrategy(str, strategyMap, dto.serviceId)) .collect(Collectors.toList()); - return new PersistenceItemConfiguration(items, c.alias, strategies, List.of()); + return new PersistenceItemConfiguration(items, config.alias, strategies, List.of()); }).collect(Collectors.toList()); return new IdentifiablePersistenceServiceConfiguration(dto.serviceId, configs, defaults, strategyMap.values()); diff --git a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/strategy/PersistenceStrategy.java b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/strategy/PersistenceStrategy.java index 86652d3be97..8bd0b16bb5e 100644 --- a/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/strategy/PersistenceStrategy.java +++ b/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/strategy/PersistenceStrategy.java @@ -48,7 +48,7 @@ public String getName() { public int hashCode() { final int prime = 31; int result = 1; - result = prime * result + ((name == null) ? 0 : name.hashCode()); + result = prime * result + name.hashCode(); return result; } diff --git a/bundles/org.openhab.core.persistence/src/test/java/org/openhab/core/persistence/internal/PersistenceManagerTest.java b/bundles/org.openhab.core.persistence/src/test/java/org/openhab/core/persistence/internal/PersistenceManagerTest.java index 06f83b9670c..3f6554147d0 100644 --- a/bundles/org.openhab.core.persistence/src/test/java/org/openhab/core/persistence/internal/PersistenceManagerTest.java +++ b/bundles/org.openhab.core.persistence/src/test/java/org/openhab/core/persistence/internal/PersistenceManagerTest.java @@ -19,7 +19,6 @@ import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -131,8 +130,7 @@ public void setUp() throws ItemNotFoundException { manager.addPersistenceService(persistenceServiceMock); manager.addPersistenceService(queryablePersistenceServiceMock); - clearInvocations(persistenceServiceMock); - clearInvocations(queryablePersistenceServiceMock); + clearInvocations(persistenceServiceMock, queryablePersistenceServiceMock); } @Test @@ -153,7 +151,7 @@ public void doesNotApplyToItemWithItemConfig() { manager.stateUpdated(TEST_ITEM2, TEST_STATE); - verifyNoInteractions(persistenceServiceMock); + verifyNoMoreInteractions(persistenceServiceMock); } @Test @@ -186,7 +184,7 @@ public void doesNotApplyToItemWithGroupConfig() { manager.stateUpdated(TEST_ITEM2, TEST_STATE); manager.stateUpdated(TEST_GROUP_ITEM, TEST_STATE); - verifyNoInteractions(persistenceServiceMock); + verifyNoMoreInteractions(persistenceServiceMock); } @Test @@ -209,7 +207,7 @@ public void updatedStateDoesNotPersistWithChangeStrategy() { addConfiguration(TEST_PERSISTENCE_SERVICE_ID, new PersistenceAllConfig(), PersistenceStrategy.Globals.CHANGE); manager.stateUpdated(TEST_ITEM, TEST_STATE); - verifyNoInteractions(persistenceServiceMock); + verifyNoMoreInteractions(persistenceServiceMock); } @Test @@ -228,7 +226,7 @@ public void changedStateDoesNotPersistWithUpdateStrategy() { manager.stateChanged(TEST_ITEM, UnDefType.UNDEF, TEST_STATE); - verifyNoInteractions(persistenceServiceMock); + verifyNoMoreInteractions(persistenceServiceMock); } @Test @@ -245,7 +243,7 @@ public void restoreOnStartupWhenItemNull() { verify(queryablePersistenceServiceMock, times(3)).query(any()); verifyNoMoreInteractions(queryablePersistenceServiceMock); - verifyNoInteractions(persistenceServiceMock); + verifyNoMoreInteractions(persistenceServiceMock); } @Test @@ -266,7 +264,7 @@ public void noRestoreOnStartupWhenItemNotNull() { verify(queryablePersistenceServiceMock, times(2)).query(any()); verifyNoMoreInteractions(queryablePersistenceServiceMock); - verifyNoInteractions(persistenceServiceMock); + verifyNoMoreInteractions(persistenceServiceMock); } @Test