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

Persistence aliases #4363

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Aug 27, 2024

See discussion #4359

At its current state, defining and using aliases for persistence is largely broken.

  1. At the point of definining aliases, one can define an alias in the syntax for an item, item group or all items. There are several issues with this way of defining:
  • The last two options obviously don't make sense.
  • One has to define with all parameters (strategy, filters) for each item you want to define an alias for (otherwise the default will be used). It becomes impossible to use a group definition for the strategy and filters.
  • It is not possible to define an alias for an item without a strategy (once you define it, the default strategy will be applied). This can be useful if an item is persisted manually through a rule, and not through a strategy.
  1. There are methods to store items with aliases. However, on retrieval (query), it is assumed the itemName in the FilterCriteria is the alias. Many persistence services use this filter itemName 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:

  1. Redefine syntax to have a separate section to define aliases. This is a breaking change, but allows defining aliases in a proper and consistent way, and is easy enough to apply.
  2. Add interface methods (with default implemention) QueryablePersistenceService.query(FilterCriteria, String) and ModifiablePersistenceService.remove(FilterCriteria, String). The second argument defaults to null 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 the query and remove 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 and remove 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 with alias 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.

@mherwege mherwege requested a review from a team as a code owner August 27, 2024 08:01
@mherwege mherwege marked this pull request as draft August 27, 2024 08:06
@jpg0
Copy link
Contributor

jpg0 commented Aug 27, 2024

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.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege mherwege marked this pull request as ready for review August 29, 2024 12:29
@mherwege
Copy link
Contributor Author

This is now tested, and if the concept is accepted, it is ready from my perspective. This is of course open for further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants