Skip to content

Pass timestamps from Timeseries to ModifiablePersistenceServices #5733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mjagdis wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Pass timestamps from Timeseries to ModifiablePersistenceServices #5733

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cb3918
Pass timestamps from Timeseries to ModifiablePersistenceServices
mjagdis Jul 26, 2026
28a287f
Use new 4 arg store wherever possible
mjagdis Jul 27, 2026
b543379
Get "now" as close to getState as possible for persist jobs
mjagdis Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 0 additions & 42 deletions ....persistence/src/main/java/org/openhab/core/persistence/ModifiablePersistenceService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,8 @@
*/
package org.openhab.core.persistence;

import java.time.ZonedDateTime;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.core.items.Item;
import org.openhab.core.types.State;

/**
* This class provides an interface to the a {@link PersistenceService} to allow data to be stored
Expand All @@ -29,44 +25,6 @@
*/
@NonNullByDefault
public interface ModifiablePersistenceService extends QueryablePersistenceService {
/**
* <p>
* Stores the historic item value. This allows the item, time and value to be specified.
*
* <p>
* Adding data with the same time as an existing record should update the current record value rather than adding a
* new record.
*
* <p>
* Implementors should keep in mind that all registered {@link PersistenceService}s are called synchronously. Hence
* long running operations should be processed asynchronously. E.g. <code>store</code> adds things to a queue which
* is processed by some asynchronous workers (Quartz Job, Thread, etc.).
*
* @param item the data to be stored
* @param date the date of the record
* @param state the state to be recorded
*/
void store(Item item, ZonedDateTime date, State state);

/**
* <p>
* Stores the historic item value under a specified alias. This allows the item, time and value to be specified.
*
* <p>
* Adding data with the same time as an existing record should update the current record value rather than adding a
* new record.
*
* <p>
* Implementors should keep in mind that all registered {@link PersistenceService}s are called synchronously. Hence
* long running operations should be processed asynchronously. E.g. <code>store</code> adds things to a queue which
* is processed by some asynchronous workers (Quartz Job, Thread, etc.).
*
* @param item the data to be stored
* @param date the date of the record
* @param state the state to be recorded
*/
void store(Item item, ZonedDateTime date, State state, @Nullable String alias);

/**
* Removes data associated with an item from a persistence service.
* If all data is removed for the specified item, the persistence service should free any resources associated with
Expand Down
67 changes: 65 additions & 2 deletions ...enhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
*/
package org.openhab.core.persistence;

import java.time.ZonedDateTime;
import java.util.List;
import java.util.Locale;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.core.items.Item;
import org.openhab.core.persistence.strategy.PersistenceStrategy;
import org.openhab.core.types.State;

/**
* A persistence service which can be used to store data from openHAB.
Expand Down Expand Up @@ -49,21 +51,79 @@ public interface PersistenceService {
*/
String getLabel(@Nullable Locale locale);

/**
* <p>
* Stores the historic item value. This allows the item, time and value to be specified.
*
* <p>
* Adding data with the same time as an existing record should update the current record value rather than adding a
* new record.
*
* <p>
* Implementors SHOULD NOT rely on the default. It is provided solely for compatibility with existing
* implementations which do not provide a store method which allows date and state to be specified.
*
* <p>
* Implementors should keep in mind that all registered {@link PersistenceService}s are called synchronously. Hence
* long running operations should be processed asynchronously. E.g. <code>store</code> adds things to a queue which
* is processed by some asynchronous workers (Quartz Job, Thread, etc.).
*
* @param item the data to be stored
* @param date the date of the record
* @param state the state to be recorded
*/
default void store(Item item, ZonedDateTime date, State state) {
store(item);
}

/**
* <p>
* Stores the historic item value under a specified alias. This allows the item, time and value to be specified.
*
* <p>
* Adding data with the same time as an existing record should update the current record value rather than adding a
* new record.
*
* <p>
* Implementors SHOULD NOT rely on the default. It is provided solely for compatibility with existing
* implementations which do not provide a store method which allows date and state to be specified.
*
* <p>
* Implementors should keep in mind that all registered {@link PersistenceService}s are called synchronously. Hence
* long running operations should be processed asynchronously. E.g. <code>store</code> adds things to a queue which
* is processed by some asynchronous workers (Quartz Job, Thread, etc.).
*
* @param item the data to be stored
* @param date the date of the record
* @param state the state to be recorded
*/
Comment on lines +96 to +99
default void store(Item item, ZonedDateTime date, State state, @Nullable String alias) {
store(item, alias);
}

/**
* Stores the current value of the given item.
*
* This method has been deprecated and {@link #store(Item, ZonedDateTime, State, String)} MUST be used instead.
*
* <p>
* Implementors should keep in mind that all registered {@link PersistenceService}s are called synchronously. Hence
* long running operations should be processed asynchronously. E.g. <code>store</code> adds things to a queue which
* is processed by some asynchronous workers (Quartz Job, Thread, etc.).
*
* @param item the item which state should be persisted.
*/
void store(Item item);
@Deprecated
default void store(Item item) {
store(item, ZonedDateTime.now(), item.getState(), null);

@mherwege mherwege Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these NOOP, or you end up in a circular reference if nothing is implemented.

}

/**
* <p>
* Stores the current value of the given item under a specified alias.
*
* This method has been deprecated and {@link #store(Item, ZonedDateTime, State, String)} MUST be used instead.
*
* <p>
* Implementors should keep in mind that all registered {@link PersistenceService}s are called synchronously. Hence
* long running operations should be processed asynchronously. E.g. <code>store</code> adds things to a queue which
Expand All @@ -72,7 +132,10 @@ public interface PersistenceService {
* @param item the item which state should be persisted.
* @param alias the alias under which the item should be persisted.
*/
void store(Item item, @Nullable String alias);
@Deprecated
default void store(Item item, @Nullable String alias) {

@wborn wborn Aug 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PersistenceExtensions.internalPersist(Item, ...) still calls this overload. Since this PR deprecates it in favor of the timestamp/state-aware method, can we migrate that core caller as part of this change as well?

This becomes particularly important if these deprecated defaults are made no-ops to resolve the recursion discussed above: a persistence service implementing only the new API would otherwise silently ignore persist(item) calls. Using the new overload there with ZonedDateTime.now() and item.getState() should preserve the existing semantics while keeping the compatibility delegation one-directional.

store(item, ZonedDateTime.now(), item.getState(), alias);
}
Comment on lines +135 to +138

/**
* Provides default persistence strategies that are used for all items if no user defined configuration is found.
Expand Down
10 changes: 8 additions & 2 deletions ...rsistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,15 @@ private void handleStateEvent(Item item, boolean changed) {
}

private void storeItem(PersistenceServiceContainer container, Item item, PersistenceStrategy changeStrategy) {
ZonedDateTime now = ZonedDateTime.now();

container.getMatchingConfigurations(changeStrategy).filter(itemConfig -> appliesToItem(itemConfig, item))
.filter(itemConfig -> itemConfig.filters().stream().allMatch(filter -> filter.apply(item)))
.forEach(itemConfig -> {
itemConfig.filters().forEach(filter -> filter.persisted(item));
container.getPersistenceService().store(item, container.getAlias(item));
PersistenceService persistenceService = container.getPersistenceService();
persistenceService.store(item, Objects.requireNonNullElse(item.getLastStateUpdate(), now),

@mherwege mherwege Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastStateUpdate is not guaranteed to be updated when this is called. It will likely be as it runs in a separate thread, but applyState in GenericItem explicitly only updates it after the notifyListeners call. So I am not sure this is always valid.

item.getState(), container.getAlias(item));
});
}

Expand Down Expand Up @@ -817,12 +821,14 @@ public String getName() {
}

private void persistJob(List<PersistenceItemConfiguration> itemConfigs) {
ZonedDateTime now = ZonedDateTime.now();

itemConfigs.forEach(itemConfig -> {
for (Item item : getAllItems(itemConfig)) {
if (itemConfig.filters().stream().allMatch(filter -> filter.apply(item))) {
long startTime = System.nanoTime();
itemConfig.filters().forEach(filter -> filter.persisted(item));
persistenceService.store(item, getAlias(item));
persistenceService.store(item, now, item.getState(), getAlias(item));

@mherwege mherwege Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an item's state changes while iterating over the items, the date will be wrong. You should only apply this fix when restoring forecasts.

@mjagdis mjagdis Jul 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try this then. There always is a risk that the state can change under us because GenericItem doesn't seem to have any atomicity guarantees but this should be no worse than previously. Possibly slightly better since now the core ensures now and getState are always next to each other whereas before it depended how the service implemented store.

logger.trace("Storing item '{}' with persistence service '{}' took {}ms", item.getName(),
configuration.getUID(), TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime));
}
Expand Down
42 changes: 23 additions & 19 deletions ...rsistence/src/test/java/org/openhab/core/persistence/internal/PersistenceManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ public void appliesToItemWithItemConfig() {

manager.stateUpdated(TEST_ITEM, TEST_STATE);

verify(persistenceServiceMock).store(TEST_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));
verifyNoMoreInteractions(persistenceServiceMock);
}

Expand All @@ -235,7 +235,7 @@ public void appliesToGroupItemWithItemConfig() {

manager.stateUpdated(TEST_GROUP_ITEM, TEST_STATE);

verify(persistenceServiceMock).store(TEST_GROUP_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_GROUP_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));
verifyNoMoreInteractions(persistenceServiceMock);
}

Expand All @@ -246,7 +246,7 @@ public void appliesToItemWithGroupConfig() {

manager.stateUpdated(TEST_ITEM, TEST_STATE);

verify(persistenceServiceMock).store(TEST_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));
verifyNoMoreInteractions(persistenceServiceMock);
}

Expand All @@ -270,9 +270,9 @@ public void appliesToItemWithAllConfig() {
manager.stateUpdated(TEST_ITEM2, TEST_STATE);
manager.stateUpdated(TEST_GROUP_ITEM, TEST_STATE);

verify(persistenceServiceMock).store(TEST_ITEM, null);
verify(persistenceServiceMock).store(TEST_ITEM2, null);
verify(persistenceServiceMock).store(TEST_GROUP_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));
verify(persistenceServiceMock).store(eq(TEST_ITEM2), any(ZonedDateTime.class), any(State.class), eq(null));
verify(persistenceServiceMock).store(eq(TEST_GROUP_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));

verifyNoMoreInteractions(persistenceServiceMock);
}
Expand All @@ -297,8 +297,8 @@ public void doesNotApplyToItemWithAllConfigAndItemExclusion() {
manager.stateUpdated(TEST_ITEM2, TEST_STATE);
manager.stateUpdated(TEST_GROUP_ITEM, TEST_STATE);

verify(persistenceServiceMock).store(TEST_ITEM2, null);
verify(persistenceServiceMock).store(TEST_GROUP_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM2), any(ZonedDateTime.class), any(State.class), eq(null));
verify(persistenceServiceMock).store(eq(TEST_GROUP_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));

verifyNoMoreInteractions(persistenceServiceMock);
}
Expand All @@ -313,8 +313,8 @@ public void doesNotApplyToItemWithAllConfigAndGroupExclusion() {
manager.stateUpdated(TEST_ITEM2, TEST_STATE);
manager.stateUpdated(TEST_GROUP_ITEM, TEST_STATE);

verify(persistenceServiceMock).store(TEST_ITEM2, null);
verify(persistenceServiceMock).store(TEST_GROUP_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM2), any(ZonedDateTime.class), any(State.class), eq(null));
verify(persistenceServiceMock).store(eq(TEST_GROUP_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));

verifyNoMoreInteractions(persistenceServiceMock);
}
Expand All @@ -331,8 +331,8 @@ public void doesNotApplyToNestedGroupItemWithAllConfigAndGroupExclusion() {
manager.stateUpdated(TEST_ITEM3, DecimalType.ZERO);
manager.stateUpdated(TEST_GROUP_ITEM2, DecimalType.ZERO);

verify(persistenceServiceMock).store(TEST_ITEM2, null);
verify(persistenceServiceMock).store(TEST_GROUP_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM2), any(ZonedDateTime.class), any(State.class), eq(null));
verify(persistenceServiceMock).store(eq(TEST_GROUP_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));

verifyNoMoreInteractions(persistenceServiceMock);
}
Expand All @@ -345,7 +345,8 @@ public void updatedStatePersistsEveryUpdate() {
manager.stateUpdated(TEST_ITEM, TEST_STATE);
manager.stateUpdated(TEST_ITEM, TEST_STATE);

verify(persistenceServiceMock, times(2)).store(TEST_ITEM, null);
verify(persistenceServiceMock, times(2)).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class),
eq(null));

verifyNoMoreInteractions(persistenceServiceMock);
}
Expand All @@ -366,7 +367,7 @@ public void changedStatePersistsWithChangeStrategy() {

manager.stateChanged(TEST_ITEM, UnDefType.UNDEF, TEST_STATE);

verify(persistenceServiceMock).store(TEST_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));
verifyNoMoreInteractions(persistenceServiceMock);
}

Expand Down Expand Up @@ -485,7 +486,7 @@ public void storeTimeSeriesAndForecastsScheduled() {
assertThat(lastStateChange.toInstant(), is(firstEntry.timestamp()));

// Check if other persistence services got updated
verify(persistenceServiceMock).store(TEST_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));
verify(persistenceServiceMock, atLeast(0)).getId();
verifyNoMoreInteractions(persistenceServiceMock);

Expand Down Expand Up @@ -539,7 +540,7 @@ public void externalPersistenceDataChangeIsHandled() {
manager.handleExternalPersistenceDataChange(queryablePersistenceServiceMock, TEST_ITEM);
verify(queryablePersistenceServiceMock).persistedItem(eq(TEST_ITEM_NAME), any());
assertEquals(TEST_STATE, TEST_ITEM.getState());
verify(persistenceServiceMock).store(TEST_ITEM, null);
verify(persistenceServiceMock).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class), eq(null));
}

@Test
Expand Down Expand Up @@ -568,9 +569,11 @@ public void cronStrategyIsScheduledAndCancelledAndPersistsValue() throws Excepti
verify(cronSchedulerMock, times(2)).schedule(any(), any());
verify(scheduledFutureMock, times(2)).cancel(true);
// no filter - persist everything
verify(persistenceServiceMock, times(2)).store(TEST_ITEM3, null);
verify(persistenceServiceMock, times(2)).store(eq(TEST_ITEM3), any(ZonedDateTime.class), any(State.class),
eq(null));
// filter - persist filtered value
verify(queryablePersistenceServiceMock, times(1)).store(TEST_ITEM3, null);
verify(queryablePersistenceServiceMock, times(1)).store(eq(TEST_ITEM3), any(ZonedDateTime.class),
any(State.class), eq(null));
}

@Test
Expand Down Expand Up @@ -600,7 +603,8 @@ public void filterAppliesOnStateUpdate() {
manager.stateUpdated(TEST_ITEM3, DecimalType.ZERO);
manager.stateUpdated(TEST_ITEM3, DecimalType.ZERO);

verify(persistenceServiceMock, times(1)).store(TEST_ITEM3, null);
verify(persistenceServiceMock, times(1)).store(eq(TEST_ITEM3), any(ZonedDateTime.class), any(State.class),
eq(null));

verifyNoMoreInteractions(persistenceServiceMock);
}
Expand Down