Open mherwege opened 3 months ago
Thanks for the PR. I think it addresses the current problems and introduces the least possible change. The code looks fine from a quick look.
This is now tested, and if the concept is accepted, it is ready from my perspective. This is of course open for further discussion.
See discussion https://github.com/openhab/openhab-core/issues/4359
At its current state, defining and using aliases for persistence is largely broken.
query
), it is assumed theitemName
in theFilterCriteria
is the alias. Many persistence services use this filteritemName
to look up the item in the item registry (e.g. to find the unit). This does not work if an alias is passed in the filter criteria.This PR does 2 things:
QueryablePersistenceService.query(FilterCriteria, String)
andModifiablePersistenceService.remove(FilterCriteria, String)
. The second argument defaults tonull
in its default implementation. Persistence services implementing support for aliases and using item registry lookups, should implement these new methods to make querying with aliases work. However, as there is a default implementation, if the persistence service is not changed, it will behave as it does now (failing if the persistence service tries to do a lookup in the item registry). All core calls to thequery
andremove
methods are also changed to add the alias if one exists. I expect some classes in the ui repo (and maybe some scripting addons) will need limited changes as well to properly support aliases. (I have not done this, but the methods without alias could be annotated@Deprecated
to make it clear to the consumer they should use the methods with alias arguments).For 2, the better approach would probably be to make the individual persistence services completely independent of the item registry. The interface should then have
store
,query
andremove
methods that are only concerned about the storage key and values, whereby the storage key is the item name or alias. The core persistence layer would be responsible for using item name or alias when calling these methods, and then also for all actions currently done in the individual persistence services where the lookup in the item registry is required (e.g. unit lookup and conversion). Also ChartServlets would be impacted by this. As this is obviously a major API change, I have refrained from doing that. With the current proposal, the interaction with persistence services should keep on working as before, and persistence services that support aliases (that is a limited number) can easily be enhanced to properly support them (instead of the broken support in the current implementation without applying required changes).Another option would be to completely remove support for aliases from openHAB core, and leave it to individual persistence services if they want to implement something, marking the
store
methods withalias
parameters as deprecated. I also think that would be a step back and is not needed. But it may simplify code and, as it stands, I don't have the impression aliases are used a lot (if they where, I would expect to see many more issues being created as they don't work).This PR is offered as a starting point and open for discussion. That's why I am still marking this as draft. My design premises have been to try to fix it while not breaking the interface with persistence addons. I would very much appreciate feedback if this approach makes sense, and would be accepted before I invest further time in it.
@jpg0 FYI and evaluation.
EDIT: changed the default query method implementations to do an alias lookup, so the returned results are correct as long as the implementing persistence service does not do an item registry lookup. This again limits impact on persistence service implementations.