Return units in persistence extension commands and support future persisted states

[mherwege]

Closes https://github.com/openhab/openhab-core/issues/3732 Closes https://github.com/openhab/openhab-core/issues/3896

This PR implements unit support in all PersistenceExtension commands, returning the full value including unit for Number item types with a dimension. It therefore changes return types from DecimalType to a State type, whereby the state can include the unit. This has been tested to work with rules DSL, and it should be possible to extend other scripting languages to support this, making calculations with historic states of items with dimensions more straightforward. This may require adapting user rules and is therefore a breaking change.

Secondly, this PR makes sure persistence extensions work properly with persisted items in the future:

• all Between commands have been adapted to be able to properly work across the now boundary,
• a series of Till commands have been added, equivalent to the Since commands.
• nexUpdate and nexState commands have been added.
• the historicState command has been deprecated (but still available for backward compatibility) and replaced with persistedState also accepting future dates.
• the evolutionRate command has been renamed to evolutionRateSince, evolutionRateTill and evolutionRateBetween, bringing it in line with the other commands. This change is required to allow one date signatures to differentiate between Since and Till. evolutionRate is deprecated and kept for backward compatibility, and interpreted as evolutionRateSince.
• added removeAllStates Since, Till and Between commands.

The code now also has full null annotations and checks. In doing so, changes were required to avoid potential null issues. Many of these null issues where due to null being interpreted as now for end dates before, which is not possible when time can be in the future.

The test classes where extended and refactored to also test for all future commands.

Signed-off by: Mark Herwege [email protected]

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/historicstate-returns-uom-but-deltasince-doesnt/148713/2

[J-N-K]

I understand what you want to achieve, but why did you chose HistoricItem as return value? The timestamp has no meaning for averages or similar, a simple State would be fine (and could be a DecimalType or a QuantityType (depending on the item's unit).

[mherwege]

I understand what you want to achieve, but why did you chose HistoricItem as return value? The timestamp has no meaning for averages or similar, a simple State would be fine (and could be a DecimalType or a QuantityType (depending on the item's unit).

HistoricItem was already used for minimum, maximum and previous, so wanted to stick to that. But I agree the date returned in the HistoricItem is arbitrary. I will adjust.

[mherwege]

@J-N-K the methods now return State.

As this is API breaking anyway, I also aligned method names. EvolutionRate whas the only group of methods that did not have a distinction between Since and Between in the name. I believe this is a rarely used method anyway and I think it is cleaner to have them aligned. If you feel differently, I can revert this last commit.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/blockly-quantity-type-gets-lost-when-using-persistence/150470/4

[florian-h05]

Any chance to get this merged for openHAB 4.1 or is that something for a new major release because it's breaking?

[kaikreuzer]

@J-N-K As you had commented before, I hoped that you continue on this PR - any chance to do so?

This may require adapting user rules and is therefore a breaking change.

Breaking user rules is never nice. I'm just trying to get my head around how much this might break. Do you have a typical example where user's will have to adapt their rules? Are these rather exceptional cases or do most rules break by this? Is there any migration path that we could document as a "quick fix"? I am not 100% sure whether we should merge this now so shortly before the release. Waiting for a next major release isn't really an option either, that will still take a while...

[florian-h05]

If this gets merged for 4.1, we should try to implement a solution for #3896 for 4.1 as well to avoid having multiple sets of breaking changes.

[J-N-K]

I have a rule that calculates the mean temperature over the last three days. It uses averageSince and then adds the unit before posting the update to the item.

I still think this is a good addition, but I have no idea how to make it "less breaking".

[mherwege]

I have a rule that calculates the mean temperature over the last three days. It uses averageSince and then adds the unit before posting the update to the item.

Similar for me, and that was the reason to create this PR. It forces you to add the unit again after retrieving the average. That means you need to 'know' the unit in the rule when retrieving to put the right unit back and it goes against the principle of UOM where you don't care.

[mherwege]

If this gets merged for 4.1, we should try to implement a solution for #3896 for 4.1 as well to avoid having multiple sets of breaking changes.

@J-N-K @florian-h05 @kaikreuzer If we all agree we want to do this, I could extend this PR to include a solution for https://github.com/openhab/openhab-core/issues/3896. @florian-h05 Scripting languages may also be affected. Would any change be required in the javascript helper libraries? What about jruby (@jimtng) and habapp (@spacemanspiff2007) ?

[florian-h05]

If we all agree we want to do this, I could extend this PR to include a solution for #3896.

I agree, but I'm no core maintainer ;-) Thank you for the offer!

Scripting languages may also be affected. Would any change be required in the javascript helper libraries?

Yes, for renamed methods there would be changes required, however those are minor as the library is only wrapping PersistenceExtensions inside a JS class to return JS datatypes. I would also keep the old ones in the library and log a deprecation warning, so this is less breaking.

For the new functionalities, I have to think about how to integrate them in the library, but I already have an idea.

I can't speak for jRuby, but I guess HABApp is not affected because it is using the REST API to communicate with openHAB and not any Java calls.

[kaikreuzer]

If we all agree we want to do this

I agree that I want this change, I am just not sure when (before or after 4.1). I tend towards waiting after the release, so that we can get some feedback by snapshot and milestone users first and maybe come up with a good migration plan for 4.2.

[florian-h05]

Yeah I've already asked myself when this should be implemented.

On one hand, it would be nice to provide the right "tools" together with the new time-series support, but on the other hand 4.0 had several breaking changes (and so it would be "nice" to have less/no breaking changes in 4.1) and 1.5 weeks till the 4.1 code-freeze is also a bit short to coordinate the changes across all repos and test this.

[J-N-K]

Similar for me, and that was the reason to create this PR. It forces you to add the unit again after retrieving the average. That means you need to 'know' the unit in the rule when retrieving to put the right unit back and it goes against the principle of UOM where you don't care.

Full ack. But nevertheless all existing rules using persistence extensions need to be changed.

[spacemanspiff2007]

Similar for me, and that was the reason to create this PR. It forces you to add the unit again after retrieving the average.

But why? I thought when the unit is properly set you can just post a unit with an empty value and the unit from the metadata will be assumed? Imho you should just be able to post the unitless value and everything should work as expected. The whole point of explicitly providing the unit in the item definition is that there are no ambiguites and the unit can always properly be resolved.

I think HABApp is not affected (but I have to confirm this because currently there are no test cases) because when using the RestAPI the unit is properly assumed when not provided.

[mherwege]

But why? I thought when the unit is properly set you can just post a unit with an empty value and the unit from the metadata will be assumed? Imho you should just be able to post the unitless value and everything should work as expected. The whole point of explicitly providing the unit in the item definition is that there are no ambiguites and the unit can always properly be resolved.

That's true if you immediately post an update to an item that has the same unit in metadata as the unit for the item you retrieve from persistence. Sometimes, these are intermediate steps in calculations. When you retrieve something from persistence, you have to know what unit it is, and you need to consider that in each step in the calculation, convert the value if necessary yourself as it may be in the wrong unit, and then post an update to an item with the value in the unit the item expects. So the rule needs to know in each step what unit it is working in, but it is not explicit. I prefer to have the Quantities with units in the calculations all the way, to avoid all of this. That way the rule does not have to know the unit it retrieves from persistence and the unit of the item it writes to.

[mherwege]

I agree that I want this change, I am just not sure when (before or after 4.1). I tend towards waiting after the release, so that we can get some feedback by snapshot and milestone users first and maybe come up with a good migration plan for 4.2.

I will work on extending this with logic for future states with target 4.2 for the whole PR.

[spacemanspiff2007]

That's true if you immediately post an update to an item that has the same unit in metadata as the unit for the item you retrieve from persistence.

That's exactly the example @J-N-K made above and that's why I was confused because this should already work.

[J-N-K]

My example was just a rule that needs adjustment, not an example to justify this change :-)

[J-N-K]

What's the state here? Are the discussed extensions already included and we can review and merge? The sooner, the better: more time for testing.

[mherwege]

What's the state here? Are the discussed extensions already included and we can review and merge? The sooner, the better: more time for testing.

Sorry, not yet. And I won’t be able to work on it for the next few days.

[mherwege]

@J-N-K, @florian-h05 I completed the work on persistence extensions, so this is now ready for review. I messed up my local git history, so had to force push a new state with the full content of the file. I am sorry about that.

[jimtng]

Sorry to be so late to the party. I would suggest calling the methods Until instead of Till.

[mherwege]

Sorry to be so late to the party. I would suggest calling the methods Until instead of Till.

Fine for me, change done.

[jimtng]

Fine for me, change done.

Thanks so much! The OCD in me would love the "till" in the comments also updated to say "until" to be more consistent.

[mherwege]

I am going through the javadocs again. Many of them need updates or clarifications. I am working on that and will come back with refinements on them.

[mherwege]

I am going through the javadocs again. Many of them need updates or clarifications. I am working on that and will come back with refinements on them.

Done, and removed some commented code and solved an issue.

[jimtng]

Scripting languages may also be affected. Would any change be required in the javascript helper libraries? What about jruby

If the old evolutionRate() were to remain, perhaps with a deprecation warning like historicState, then it wouldn't break any jruby scripts, and perhaps also other scripting languages?

[mherwege]

If the old evolutionRate() were to remain, perhaps with a deprecation warning like historicState, then it wouldn't break any jruby scripts, and perhaps also other scripting languages?

Done that now. A warning similar to historicState will be logged. @florian-h05 is better placed to assess if there is anything breaking left for javascript. There shouldn't be for DSL now. Note there are slight changes in null vs 0 concerning corner cases. Null is typically an indication of errors in arguments now, while empty persistence in the timerange can return 0 where it makes sense (if no division by 0 would be required). This has been done because there were no null annotations before (and some of it went unnoticed), and time can now span from past to future (introducing a new class of argument errors). It was also not consistent. I don't expect this to have direct impact on rules, as these are corner cases and typical errors when returning null.