Return units in persistence extension commands and support future persisted states

[mherwege]

Closes #3732
Closes #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 Untill 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, evolutionRateUntill and evolutionRateBetween, bringing it in line with the other commands. This change is required to allow one date signatures to differentiate between Since and Untill. evolutionRate is deprecated and kept for backward compatibility, and interpreted as evolutionRateSince.
• added removeAllStates Since, Untill 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 mark.herwege@telenet.be

[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 #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]

adjust the documentation

Documentation openhab/openhab-docs#2289 is already created. I will do a breaking changes notice.

[florian-h05]

FYI openHAB JavaScript library PR to adjust to this PR is already created: openhab/openhab-js#331
Blockly needs to be updated next ...

[lolodomo]

@mherwege : does it require a note in the release notes to mention a breaking change ?

[J-N-K]

Already done:openhab/openhab-distro#1652

[florian-h05]

Can someone please trigger a distro build?
The WebUI-Job already triggered one but that one failed due, seems like there was a problem with JFrog artifactory.

[jimtng]

core build seems to be on a daily basis

[J-N-K]

Done

[jimtng]

openhab-core build is done now. Next, we need the distro build to kick in

[J-N-K]

Distro-build should not be required to build webui

[jimtng]

I've just noticed that the "between" variant of the old evolutionRate() was removed in this PR, instead of deprecated. Should we add it back for the sake of compatibility?

[mherwege]

I've just noticed that the "between" variant of the old evolutionRate() was removed in this PR, instead of deprecated. Should we add it back for the sake of compatibility?

Yes, makes sense. I missed that one.

[jimtng]

Another change I've just noticed: Previously I could call any methods with a null serviceid to indicate that the default persistence service is to be used.

Now, due to @NonNullByDefault on the PersistenceExtensions class, and the absence of @Nullable in front of serviceId, this is no longer possible. I think this now fails because of the implementation/code specifically not allowing null serviceid to mean "default persistence service".

This change has caused some errors in existing scripts. Should we restore the previous behaviour?

[jimtng]

@mherwege I edited/updated the post above, in case you've only read the original message.

[mherwege]

Another change I've just noticed: Previously I could call any methods with a null serviceid to indicate that the default persistence service is to be used.

@jimtng It has never been documented it could be used that way. Also, quickly looking through the old code again, it doesn't look like it worked for all actions, and it even gave wrong result for some (part of the query working and another part returning null and defaulting to a wrong default value, so the combination being wrong).
While it is certainly doable to introduce this, I am wondering if it is worth doing this to fix wrong usage of the actions in the first place.

[mherwege]

@jimtng PR is created to allow null serviceId.

[jimtng]

@jimtng PR is created to allow null serviceId.

Thanks! I was pondering about your previous response. I agree that it wasn't documented in the javadocs. I've started working on modifications in the jruby helper library to avoid using a null serviceId. However, I think it's a handy thing to support but either way is fine.