Skip to content
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.

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

PersistenceExtensions ignores item persistence aliases #4359

Open
jpg0 opened this issue Aug 21, 2024 · 8 comments
Open

PersistenceExtensions ignores item persistence aliases #4359

jpg0 opened this issue Aug 21, 2024 · 8 comments
Labels
bug An unexpected problem or unintended behavior of the Core

Comments

@jpg0
Copy link
Contributor

jpg0 commented Aug 21, 2024

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.

@jpg0 jpg0 added the bug An unexpected problem or unintended behavior of the Core label Aug 21, 2024
@mherwege
Copy link
Contributor

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:

  • Where should the alias mapping be kept? At the moment it is in the item persistence configuration.
  • Having the mapping in the persistence configuration is fine for automatic writing through a persistence strategy, but what for reading back? Should we make it the responsability of the specific persistence service to map back, so it can keep track of aliases used in the past (if you would change the persistence configuration). If so, can it store a mapping table, real name to list of aliases?
  • You can use the persistence extension persist to persist an item state. This can also be done for an item that has no persistence configuration at all, so no alias defined. What should happen? Again, I don't think it would be wise to define the alias in the call, making it the responsability of the user to know the underlying alias.

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
Copy link
Contributor Author

jpg0 commented Aug 21, 2024

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'm not against it staying in the persistence configuration
  • Personally I would expect a pattern such that there is an abstraction of persistence services that supports both reading and writing, and operates at an item level. I'd expect this to be the place where the mapping occurs. I would not expect specific services to retain the mapping, although I have no problem with them being wrapped where the wrapper handles it.
  • I see no problem with items missing a persistence configuration; there is no requirement for them to have one anyway (I agree that it's not for the caller to decide)

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
Copy link
Contributor

mherwege commented Aug 21, 2024

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

  • Adjust persistence configuration syntax to only allow an alias on an item by item basis. It may have to become a separate section as you would also want to use these aliases when using group or all persistence configurations.
  • Keep the alias with item by persistence service in a separate structure and look for a mapping when calling store(item).
  • Change REST API to support configuration for aliases.
  • Change the persistence configuration UI for this new concept. Note, aliases are currently not supported in the UI configuration.
  • For restore, lookup the mapping, if one exists, use that instead of the item name.
  • For querying, lookup the mapping, if one exists, use that instead of the item name.
  • I think all alias handling can then be removed rom the individual persistence services, and will work the same way for all. Also services that do not current have alias handling will then have.
  • Deprecate store(item, alias). The alias should always be retrieved when calling store(item). So there is no need anymore for a separate method to be implemented in the persistence services.

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
Copy link
Contributor Author

jpg0 commented Aug 21, 2024

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
Copy link
Contributor Author

jpg0 commented Aug 23, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

jpg0 commented Aug 25, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core
Projects
None yet
Development

No branches or pull requests

2 participants