[addonservices] allow uninstalling of removed addons and fix other issues

[J-N-K]

Fixes #2576
Fixes #2581
Fixes #2673
Fixes #2687

It stores a String representation of the addon in the storage service on installation and removes it when the addon is uninstalled. When compiling the list of available addons the information in the storage takes precedence over the information received from the remote so the displayed information (including version or links) stays correct for the installed addon.

@wborn WDYT? I can extend that for the other Addon-Service very quickly, so we can ship that with 3.2.0, which would be very beneficial for users.

Signed-off-by: Jan N. Klug jan.n.klug@rub.de

[J-N-K]

I rebased after the nullness annotations have merged and also added all necessary parts for the Community Marketplace Addonservice.

[J-N-K]

Jenkins failed unrelated in persistence tests.

[splatch]

While I see the goal, I wanted to note that there is a way to have multiple sources for config admin configuration files (see multiple fileinstall configurations). Current design of openhab distro does not promote that forcing people to use userdata and likely json as storage for everything, even if its suboptimal. This is exclusively because config admin files are being rolled over with system or package update and ConfigDispatcher, as far I can understand its mechanism, is working only from services.cfg to config admin, but not vice versa.
Why not adding another config source under userdata and letting config admin/fileinstall manage that?

I could miss something here.

[J-N-K]

I am not sure I understand your point. We need to store the full representation of the Addon to restore it if needed (i.e. the add-on is still installed but no longer present in the remote source). Therefore we need to serialize that data and the JSON representation is very easy and can be extended/adapted by a custom TypeAdapter if we need extensions. What is wrong with using the StorageService for that and what would be the benefit of using another service?

[splatch]

My point is rather basic, why pulling in json and whole storage stuff, which adds just layers of complexity, instead of using config admin for that?
As far I understand addons installed via marketplace form rather basic structure which is fairly similar to what openhab used to have in addons.cfg.

[J-N-K]

No, we need a full set of information (id, title, description, version, doc link, image, etc.) to show the add-on in the market place. If it was a single information (like the name), I would agree. But as I said before: we need all (or nearly all) the information in the Addon class and that requires some sort of serialization. Of course we could use custom serialization/deserialization code, but IMO using standard libraries is better here.

The problem is that this information is only available on the remote/cloud/whatever-you-call-it side. If the information is removed there (or not available), the add-on cannot be displayed in the UI, even if it is installed (and not uninstalled because of that). Therefore we need to store the information set for installed add-ons locally. This is different from the distribution add-ons, where this information can be derived from the Karaf feature.

[splatch]

The problem is that this information is only available on the remote/cloud/whatever-you-call-it side. If the information is removed there (or not available), the add-on cannot be displayed in the UI, even if it is installed (and not uninstalled because of that). Therefore we need to store the information set for installed add-ons locally. This is different from the distribution add-ons, where this information can be derived from the Karaf feature.

I see your point, however this makes your implementation more difficult to maintain, especially when remote contents change in more fields than you actually use for identification.

[J-N-K]

No. It just requires a custom TypeAdapter for de-/serialization.

Edit: Maybe I misread your comment. The identification is done by the source:id (e.g. marketplace:123456) only. The stored information stays the same as long as the add-on is installed. This is correct, because it's the valid information for the installed version of the add-on. It is removed when the add-on is uninstalled, so with re-installation the updated information from the remote is used.

For storage the openHAB representation of the add-on is used. This is independent of the source. The fields filled in that class depends on the source, though.

[J-N-K]

I'll refactor common functionality to an abstract super class and add unit tests for that.

[J-N-K]

@openhab/core-maintainers If the overall design is fine with you, I'll add the test promised in #2633.

[J-N-K]

IMO this adds a pretty good coverage for the common part of the add-on services, including install/uninstall, remote/local only logic and storage.

[J-N-K]

Is there anything blocking progress here?

[cweitkamp]

In order to have small PR to fix bugs I submitted #2718 to fix #2687

[lolodomo]

@J-N-K : #2687 was finally fixed by another PR. You will have to update this PR.

[J-N-K]

This is definitely the last time I rebase this PR.

[cweitkamp]

LGTM. Thanks for your work.