Extend PersistenceExtensions for working with future states

[florian-h05]

With support for future states in persistence added in #3597, we need to provide access to future states stored on persistence from scripts.

I therefore propose to extend org.openhab.core.persistence.extensions.PersistenceExtensions with the following methods:

• Add ****Till equivalents to ****Since, e.g. averageTill equivalent to averageSince (effectively those methods just calll ****Between with now and the future timestamp)
• historicState should probably be renamed to persistedState (guessing it is able to retrieve future states as well as historic ones)
• Add nextUpdate equivalent to lastUpdate
• Add nextState equivalent to previousState
• Add method overloads to persist(Item): persist(Item, ZonedDateTime, State) and persist(Item, TimeSeries)
• Add a removePersistedStates(Item, FilterCriteria) method

Also rename HistoricItem to PersistedItem. I guess this shouldn't be such a breaking changes as renaming some methods above, since most rules won't care about the classname of the return value.

As discussed in #3736, this will be implemented for openHAB 4.2 in PR #3736.

[mherwege]

If anyone starts working on this, also consider #3736. If that PR is accepted, we should make sure this new functionality works with units in the same way.

[rkoshak]

Changing the names would be a breaking change. Would that be allowed? Maybe if we keep the old names with deprecation warnings in the log?

[shikousa]

As InMemoryPersistenceService is only available in RAM, it makes sense to have also following methods available in rules:

.store(ZonedDateTime,State)
.remove(FilterCriteria)
.removePersistenceData() (removes complete persisted data)
...

Most of this methods should already be available in InMemoryPersistenceService.java

This persisted data in RAM could be used for charts, especially to improve performance.

[rkoshak]

.store(ZonedDateTime,State)

We already have a persist method so it makes sense to use that name for this, or change both to store for consistency.

[florian-h05]

If anyone starts working on this, also consider #3736.

And this proposal should then be implemented in the same release to avoid having breaking changes two times. Better to bundle them.

Changing the names would be a breaking change. Would that be allowed?

No one is a fan of breaking changes, but I guess as they are paired with new functionality, they will be accepted. It is up to the core maintainers to decide that.

Maybe if we keep the old names with deprecation warnings in the log?

An option, but I'd do that inside the JavaScript helper library and also ask the Ruby helper library maintainers the same, because IMHO we should keep the PersistenceExtensions in code "clean".

I have updated my proposal above to incorporate some comments.

.store(ZonedDateTime,State)

I would implement that as method overload for persist. Those methods do the same, they only take different arguments and therefore should be named the same.

.remove(FilterCriteria)
.removePersistenceData() (removes complete persisted data)

Not sure if we should provide such a destructive method as the last one ...

[shikousa]

.remove(FilterCriteria)
.removePersistenceData() (removes complete persisted data)

Not sure if we should provide such a destructive method as the last one ...

Maybe we should limit it to the InMemoryPersistenceService. As it is in RAM after a reboot it will be empty anyway. It makes sense to have a .removePersistenceData() as then we have the possitilty at certain times like midnight to create InMemory persistence items basing on certain values of e.g. a JDBC persistence item.
This we you could use for charts. If you want to have a yearly energy chart you actually need only 13 energy readings using diff_first/diff_last for a complete year (12 months + 1). At the moment Openhab is always getting all data to create the charts; having energy readings e.g. every minute this takes some time at the moment! With the InMemoryPersistenceService we could workaround this issue.

What would also be good if you have an extended/overloaded .persistedState() where you can define the search order. At the moment we are always searching Ordering.DESCENDING

The implementation could look like this (also see current implementation historicSate() )

PersistedState persistedState(Item item, ZonedDateTime timestamp, String serviceId, Boolean ordering) 
{
...
  if (ordering == true) {
     filter.setOrdering(Ordering.ASCENDING);
  } else {
     filter.setOrdering(Ordering.DESCENDING);
  }
...
}

[rkoshak]

I'd do that inside the JavaScript helper library and also ask the Ruby helper library maintainers

But Rules DSL users will simply have their rules break. Groovy too I suspect. Both directly use the Java classes directly. Blockly will also require an opening and resaving of the rules I suspect (maybe the upgrade tool will work in that case?).

I worry about stirring up a hornet's nest in a point release in OH.

[florian-h05]

Maybe we should limit it to the InMemoryPersistenceService. As it is in RAM after a reboot it will be empty anyway. It makes sense to have a .removePersistenceData() as then we have the possitilty at certain times like midnight to create InMemory persistence items basing on certain values of e.g. a JDBC persistence item.

if I understand that right, you want to delete that data and then store a new set of data. IMO it's better to then send a new TimeSeries with REPLACE policy, this way all existing entries in the range of the new time-series are replaced by the new time-series.

But Rules DSL users will simply have their rules break. Groovy too I suspect. Both directly use the Java classes directly. Blockly will also require an opening and resaving of the rules I suspect (maybe the upgrade tool will work in that case?).

Rules DSL and Groovy will experience a breaking change, that's correct though.
Blockly cannot use the update tool, because the Blockly code is generated by the UI, but some of the breaking changes can be avoided by keeping a compatibility layer inside the JS library. For the other ones, the user has to reopen and resave rules. We could provide a search query for the developer sidebar to find all affected rules, so the user at least does not have to open and save all.

[shikousa]

Maybe we should limit it to the InMemoryPersistenceService. As it is in RAM after a reboot it will be empty anyway. It makes sense to have a .removePersistenceData() as then we have the possitilty at certain times like midnight to create InMemory persistence items basing on certain values of e.g. a JDBC persistence item.

if I understand that right, you want to delete that data and then store a new set of data. IMO it's better to then send a new TimeSeries with REPLACE policy, this way all existing entries in the range of the new time-series are replaced by the new time-series.

Actually not. Another example. I have the oirignal timeseries of about 44600 values (a complete months of energy readings every minute). From this timeseries I want to have a new in RAM timeseries with only 32 values (first value of each day). I would have done this by searching the original timeseries with persistedState(ZonedDateTime) and writing this value into the new timeseries with persist(ZonedDateTime, State). But persistedState(ZonedDateTime) only gives you back the found value without timestamp. Hmm, this is a problem; not sure if we could use byref to get both value and timestamp. @florian-h05 How would you do this?

[florian-h05]

That still doesn't explain why you need a removeAllPersistedData.
In case you really want to remove from persistence, a method removePersistedStates(Item, FilterCriteria) should be sufficient. In case you want to delete all data, just use a begin timestamp that is enough in the past.

But persistedState(ZonedDateTime) only gives you back the found value without timestamp. Hmm, this is a problem; not sure if we could use byref to get both value and timestamp.

persistedState is a renamed historicState in my proposal and historicState returns a HistoricItem, that contains the state and the timestamp. So you should be able to get the timestamp.

[shikousa]

That still doesn't explain why you need a removeAllPersistedData. In case you really want to remove from persistence, a method removePersistedStates(Item, FilterCriteria) should be sufficient. In case you want to delete all data, just use a begin timestamp that is enough in the past.

Yes, removePersistedStates(Item, FilterCriteria) is sufficient. Thank you!

[mherwege]

Also rename HistoricItem to PersistedItem. I guess this shouldn't be such a breaking changes as renaming some methods above, since most rules won't care about the classname of the return value.

@florian-h05 I can do this, but it potentially impacts rules DSL, Groovy and the Java rule extensions as well. Is this worth it, or should we just keep the name? I understand you can solve a lot in the helper libraries for js and ruby, but there is another world out there. An alternative would be to have PersistedItem implemented as a class extending HistoricItem to keep that backward compatibility.

[florian-h05]

It would be „cleaner“ to rename it, but in case it causes too big user-facing breaking changes, I’d vote on not chancing it. Let’s check first whether it actually causes user-facing breaking changes:

I’ve never had a look at Groovy and Java rule, but at least when looking at the rules DSL docs, it seems only the API matters, not the class name.

@wborn As the Groovy maintainer, could you tell us whether this is a user-facing breaking change for Groovy?
@J-N-K @seaside1 Since you both work on Java rule automation add-ons, can you please tell us?

[wborn]

As the Groovy maintainer, could you tell us whether this is a user-facing breaking change for Groovy?

It has no helper library so users would directly use PersistenceExtensions. Any breaking API change on that class would break their rules.

[florian-h05]

Any breaking API change on that class would break their rules.

Is renaming the class a breaking API change for them? Or only when methods (signatures) change?

[wborn]

Is renaming the class a breaking API change for them? Or only when methods (signatures) change?

Both will be breaking changes.

[J-N-K]

PersistedItem is not a good naming, we already use org.openhab.core.items.ManagedItemProvider$PersistedItem for storing items in the JSON database.

Renaming classes is of course breaking.

[florian-h05]

In that case it’s probably best to leave it as it is.