openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
931 stars 429 forks source link

PersistenceExtensions ignores item persistence aliases #4359

Open jpg0 opened 3 months ago

jpg0 commented 3 months ago

Expected Behavior

PersistenceExtensions.lastUpdate(item) should work for any supplied item, even if aliased

Current Behavior

As code directly reaches into persistence services using a query filter, it bypasses any alias configuration, so if the item has an alias then the filter misses it as it uses the item name directly.

Possible Solution

Apply alias to the filter prior to executing it.

mherwege commented 3 months ago

What persistence service are you using?

Does it work for any of the persistence extensions, or any of the mechanisms reading from the database (e.g. restoreOnStartup)?

As far as I understand, the alias is there to allow defining a different name for an item to be used in the database of the persistence service. That can be e.g. for adjusting names to comply with DB constraints. As such, it should be transparant to the user. Persistence extensions are a user mechanims used in rules and should be agnostic of the alias. But I don't see a consistent mechanism in the code to map an item name back to an alias. I think many persistence services actually don't do anything with the alias and just ignore it.

To make this work consistently, there may be need for some architectural discussion:

Honestly, I think the whole alias concept, while good in principle, is not working at all at the moment for the various reasons listed above. It needs more than a quick fix in the persistence extensions to solve that. As nobody has kept all persistence code to properly support this alias I also don't know how much it is used, and where it is critical. I am just wondering if it worth investing the time to fix it.

jpg0 commented 3 months ago

I've just dug a little deeper and I think you're right. The alias is used when saving state, however not when reading it. Restoring is done in PersistenceManagerImpl$PersistenceServiceContainer#restoreItemStateIfPossible and tbh it would be fairly straightforward to add the mapping there (pass it in, as the caller knows it). Right now I can't see how it's anything other than completely broken without this.

As for larger architectural concerns, I'm not sure that I'm qualified to answer, but here goes:

I do find the whole persistence aliasing configuration extremely unwieldy and it appears poorly specified to me. For example it's syntactically valid to specify:

Items {
    MyItem1,MyItem2 -> "MyReusedAlias" : strategy = ...
    * -> "EveryThingAlias" : strategy = ...
}

Which just seems entirely broken to me.

I could certainly add the ability to restore via alias (which would make it work minus the extensions), but I don't think I'd be keen on tackling the larger problems here.

mherwege commented 3 months ago

So, many things to happen here to make this work throughout:

This will also nicely provide a mechanism to keep persistence working if you change an item name. You can add an alias and it will remain the same in the database. The change will be transparent.

While doable, question is how much aliases are used in practice. Therefore, is it worth the effort, or should we just remove support. Anything less is not a solution as the concept as currently implemented is flawed from the start.

If we would remove it, individual persistence services could still offer a configuration option to do a mapping, to be implemented when the constraints on item naming in OH can create problems in the persistence service. But a persistence service may also have other means to cope with this and mapping would not be the only possible solution. The responsability would be with the persistence service in that case.

jpg0 commented 3 months ago

I wonder if another approach would be to add an extensibility point to allow transformation, similar to existing state transformation services. Scrap the configuration entirely and just allow adding a transform (could just be via JS). This would both be significantly simpler to implement, plus more powerful for users. It would require more technical expertise, however I'm guessing that only the tech literate would actually use it anyway.

Agree with the question over whether it will actually be used. My use case is to map items onto states written into influxdb by another system. I'm aware this is not the intended use-case, and I am also having to periodically refresh the state from the influxdb.

I am thinking that maybe a solution for my situation is to write a custom persistence service which maps and calls through to the original one.

jpg0 commented 3 months ago

FYI just also noticed that querying is broken (at least in influxdb) even when explicitly passing in an alias using the interface method. Whilst the data is correctly loaded, the item is then loaded from the registry to determine the type to use to coerce the data into a particular state. The alias is used here, it's not found in the registry, so it falls back to StringType. So I'd agree with you the aliasing seems completely broken, and I'd be amazed if anyone is actually using it.

mherwege commented 3 months ago

So I would think the best thing to do for know is completely remove it as a first step, and start working on an alternative approach.

jpg0 commented 3 months ago

Completely agree. The more I work with it, the more that I think that interface is wrong. Persistence services have no business needing to lookup items from registry, or even dealing with items at all. They should work with keys (item names), types (item state types), datetimes and filters.

I tried aliasing via a custom mapping persistence service (which delegates to a real whilst mapping item names), however whilst that technically works, it throws up warnings in the persistence services as it attempts to load mapped items from the registry directly.

mherwege commented 3 months ago

I tried aliasing via a custom mapping persistence service (which delegates to a real whilst mapping item names), however whilst that technically works, it throws up warnings in the persistence services as it attempts to load mapped items from the registry directly.

Indeed. One way forward without forcing a rewrite of persistence services (and one I am experimenting with), is to add an extra query(ItemFilter, String) method to the PersistenceService interfaces, whereby the second argument is the alias, defaulting to a null String. That way, when persistence services support aliases, they should override this method. Many don't so these will not have to be changed. When querying from any other method, the second form including the alias should be used, so if an alias exists, it gets passed on. Core can be extended to always include the alias if there is one.

I think removing the need for using the item registry in the addons would indeed be better (and using simple key value pairs), but it would break all persistence services straigth away. It is needed for proper unit handling.

I have some code doing this (and also redefining the way to define aliases). I need to test to see if it works well enough.