Service to find suggested addons to install

[andrewfg]

Fixes #2645

This PR provides a service to find addons that the user is suggested to install.

This finder service depends on changes made in #3865 to read in a list of addon.xml information about all available addons. It then works through the list of addons, filters on those that have discovery-methods, and uses those methods to scan the user's LAN (using UPnP and MDNS) to detect if any of the candidate things or bridges were actively detected on the LAN. The finder service then produces a filtered list of Addons containing only those bindings where activity was found. This list can be queried via a REST API endpoint.

See Architecture.pdf

Depends on #3865
Depends on openhab/openhab-distro#1604

See Use Cases.pdf

Signed-off-by: Andrew Fiddian-Green [email protected]

[andrewfg]

@mherwege the build fails on OH core :: Features :: Karaf :: Core .. probably because a lack of including this new feature; => can you please advise how to fix that?

Also the itests also fail because (so far) the code is not fully working. The upnp discovery seems to work, but the mdns is currently broken..

[mherwege]

I think the build fails on the integration test you created. There is something wrong with your itest bnd file. It does not properly resolve. I am not an expert in this, and would need to try it out to find what’s wrong.

[mherwege]

You should mark this PR as WIP.

[andrewfg]

You should mark this PR as WIP.

Ok. Done.

There is something wrong with your itest bnd file.

Many thanks for the tip. I had copied the file from another module without changing the id's within it.
Also I think there is a maven command to update the runrequires dependencies in the bnd file. But I cannot remember the command line, and Google did not help. => @mherwege do you know the respective maven command line?

---

EDIT: it seems the build failure is due to my new module not being found in the features.xml file .. the error is below, but I don't know what I need to do to resolve it ;)

[WARNING] Feature resolution failed for [openhab-core-base/4.1.0.SNAPSHOT]
Message: Unable to resolve root: missing requirement [root] osgi.identity; osgi.identity=openhab-core-base; type=karaf.feature; version=4.1.0.SNAPSHOT; filter:="(&(osgi.identity=openhab-core-base)(type=karaf.feature)(version>=4.1.0.SNAPSHOT))" [caused by: Unable to resolve openhab-core-base/4.1.0.SNAPSHOT: missing requirement [openhab-core-base/4.1.0.SNAPSHOT] osgi.identity; osgi.identity=org.openhab.core.io.rest.core; type=osgi.bundle; version="[4.1.0.202309211155,4.1.0.202309211155]"; resolution:=mandatory [caused by: Unable to resolve org.openhab.core.io.rest.core/4.1.0.202309211155: missing requirement [org.openhab.core.io.rest.core/4.1.0.202309211155] osgi.wiring.package; filter:="(&(osgi.wiring.package=org.openhab.core.config.discovery.addon.finder)(version>=4.1.0)(!(version>=5.0.0)))"]]
Repositories: {
        file:G:\git\andrewfg\openhab-core\features\karaf\openhab-core\target/feature/feature.xml

---

EDIT 2: I played around with adding stuff to the core karaf feature.xml file, without much idea about what I was doing, and with little actual effect. I think it needs to resolve and load the AddonSuggestionFinderService plus its respective dependency UpnpService and MdnsClient modules. And make them available a) to the core in general, and b) to the REST API module in particular. I need help with this.

[andrewfg]

@kaikreuzer just pinging you on this PR since it was originally your idea, and you may have thoughts on it. ??

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/need-help-to-fix-feature-xml-problem-in-a-new-oh-core-module/149862/1

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/need-help-to-fix-feature-xml-problem-in-a-new-oh-core-module/149862/2

[mherwege]

@andrewfg I am afraid I am not much help on the dependency resolution. I would think you somehow need to define a new feature in https://github.com/openhab/openhab-core/blob/main/features/karaf/openhab-core/src/main/feature/feature.xml.

Apart from that, I have a few architectural questions/suggestions.

I somehow think we should try to reuse what is there in the AddonInfo class, and maybe extend that. The structure with AddonInfoProviders makes it possible to feed more information into it earlier, making it available to all AddonInfo consumers (through the AddonInfoRegistry) earlier than during the installation.

What about:

• Move your code to read an xml file to another implementation of an AddonInfoProvider that reads that xml file. This would then register to the AddonInfoRegistry and we could get all AddonInfo information from there. You should then add the required discovery fields to AddonInfo to take care of the specific search strings. The advantage is that other suggestions can easily be added, such as country suggestions (field is already there). It would also allow better filtering and providing more information for addons in the UI. Also that makes hooking in the REST API more easy, without cross dependencies (which I suspect are a probem at the moment).
• Make your AddonSuggestionFinder an interface with separate service implementations for MDNS and UPnP. It would make it easier to add others later if needed.

[mherwege]

I experimented a bit with generating an addons.xml file from all of the addon.xml files in the addons repository. Result of my experiments are here: https://github.com/mherwege/openhab-addons/tree/addoninfo.
When you run a build (mvn clean install) from the openhab-addons/info folder, it will generate an addons.xml file from that repository.

I am not sure if this should reside in the addons repository, or in the distro repository. From the addons repository, I wouldn't have ZWave and Zigbee included, although that is still of limited use.

Next step would be to get access to this generated file in the right place.

[andrewfg]

I am not sure if this should reside in the addons repository, or in the distro repository.

I suggest that the xml file should be written to the src/main/resources of the suggested addons service module, so that the xml data is incorporated as a resource within the suggested addons service module jar file. Obviously this requires that your code would build the xml file before the suggested addons service module is compiled into its jar. Advantage of this approach is that the code and data remain together in the same jar.

[andrewfg]

@mherwege after some thought, I think that there are issues with your proposed approach. Today none of the existing bindings have any metadata in them relating to potential disoverability for the suggested addons procedure. So (today) your proposed process of scanning those existing jar files to look for such non existent metadata does not make any sense. This means that (at least in the first step) the currently missing metadata must be supplied by an independent manual process. I guess this manual process would initially be done on a few bindings, where the binding developers are also engaged in core coding. Then, I think in a second step, you will need to formally changed the existing developer coding guidelines to include the new metadata, and introduce a "force upgrade round" which puts binding developers under pressure to modify their bindings. I think such a formal change in developer guidelines, and force upgrade round, is beyond the scope of a PR like this. Personally I suggest a two step approach whereby I can contribute to the first step. But I think the second larger step is beyond my authority, so you should perhaps resolve this first with other core developers. Or??

[mherwege]

Nothing is stopping us from providing a manual file right now. But I do believe the extra fields should go in the addon.xml files in the addon repository. Someone will need to provide the info anyway.
There is a precedent where we have added country and connection fields to the addon.xml file. Some of us went back and updated many bindings. We could do the same thing here. It doesn't have to be the original developer to do that. And even if we miss some in a first pass, it would just mean they are not discoverable at first, could be added in a later release.
The advantage of it would also be to actually make fields like country and connection useful, as they will be available before binding installation. Therefore an (enhanced) UI could filter on it.

What is more of a concern for me is that the repositories are very much treated independently. I don't see yet how to copy the generated addons.xml to the core repository. Maybe it should just be stored in a configuration folder created during the distribution build, that then picks it up from the addon repository. Distribution builds will depend on a finished addons build. I don't know how easy that is to do.
The feature.xml files do get picked up, but that is done by Karaf. We don't have that mechanism available.

Another idea I had was to actually create the AddonInfoProvider implementation in the the addon repository. The service would then be installed from there, and there is no need to move any file at all.

I do agree more people should get involved in this discussion. I would very much welcome other people's view and support on this.

[wborn]

Another idea I had was to actually create the AddonInfoProvider implementation in the the addon repository. The service would then be installed from there, and there is no need to move any file at all.

I think that would be the best approach so the Core remains unaware of all these add-on implementation details. It will also allow for such a mechanism to be implemented in other repos or by third parties.

[andrewfg]

the Core remains unaware of all these add-on implementation details

I am not sure if I understand you. Surely the core MUST be aware since it finally must present the suggested addons to the user, and resp. install them. I.e. the addon suggestion finder service must a) be above and beyond the individual addons, and b) tightly integrated with the core.

[wborn]

Yes it is aware but it should provide APIs to get the details for suggesting add-ons. It's nicer if all the details stay in the add-on repos. That way contributors also only have to deal with one repo when they create PRs for their add-on.

[mherwege]

Yes it is aware but it should provide APIs to get the details for suggesting add-ons. It's nicer if all the details stay in the add-on repos. That way contributors also only have to deal with one repo when they create PRs for their add-on.

There already is an AddonInfoRegistry and an AddonInfoProvider interface in core. I was thinking of leveraging that, getting the suggestion search criteria from the AddonInfoRegistry (filter on the ones with search criteria provided). The suggestion finder in core would then be limited to using MDNS, UPnP (or other tools/algorithms), get info from the AddonInfoRegistry and return the suggested addons. A new AddonInfoProvider using the interface coded in the addon repository could be providing addon info (including search criteria) before installation of the addon, and could be an installable OSGI bundle (potentially installed by default).
This mechanism would build on my proposed addon.xml aggregation, but rather than just providing an addons.xml file, it would use the content of that file in its service.

[wborn]

Before this PR was created I was also thinking about using addon.xml aggregation and creating a service to provide the info to the Core to solve the current issues. 🙂 It could also be further extended in the future to also be able to show localized add-on info without having to install the add-ons (openhab/openhab-webui#1213).

[andrewfg]

I was also thinking about using addon.xml aggregation and creating a service to provide the info to the Core to solve the current issues. 🙂 It could also be further extended in the future to also be able to show localized add-on info without having to install the add-ons

Ok. So it sounds that we are in fact in agreement:

• the suggested addon finder service is part of the core
• the finder service uses meta data criteria about the potential (not installed) bindings, and searches (at core level) using mdns or upnp to see if potential things that would match those criteria are actually on the user's lan
• in a first stage the extra meta data is provided manually via an xml file
• in a second stage we would embed the meta data in the addon resource files, and extract it into a common location during the OH overall build process

@mherwege / @wborn please confirm that the above is indeed the common understanding?

---

EDIT: so to be more precise, the suggested addons finder service reads addon info that are provided by other addon info providers from the central addon info registry, and for the subset of those addons which have metadata indicating that they could be candidates to be a suggested addon, it scans the network to find actual evidence of potential things that match those candidate criteria, and for that subset it sets a boolean flag 'isSuggested' on the respective addon info, whic it writes back to the central addon info registry. I.e. the only thing that this service is providing is to set the isSuggested flag on addon info provided by others.

[mherwege]

EDIT: so to be more precise, the suggested addons finder service reads addon info that are provided by other addon info providers from the central addon info registry, and for the subset of those addons which have metadata indicating that they could be candidates to be a suggested addon, it scans the network to find actual evidence of potential things that match those candidate criteria, and for that subset it sets a boolean flag 'isSuggested' on the respective addon info, whic it writes back to the central addon info registry. I.e. the only thing that this service is providing is to set the isSuggested flag on addon info provided by others.

Close, but not entirely: you would not be able to write back to the registry. The Registry gets its info from various addon info providers, but does not keep a map itself, just methods to query all registered AddonInfoProviders. Therefore you cannot update a isSuggested field. The suggested addon finder service should keep track of that list.
That list also needs to be updated when an AddonProvider is added/removed in the registry. Thinking about this a bit more, the suggested addons finder should actually be a registry in itself, replicating the AddonInfo registry, but with the added functionality of filtering them for suggestions. It is the only way to dynamically add/remove AddonInfoProviders. Basing it on the exisiting registry won't allow you to do it dynamically.

I have updated my work on the addons repository to make it an AddonInfoProvider. It is not tested yet, just coded up last night. None of the query filter fields have been added yet. I first want to make it work with the existing fields. And if that works, at least country and connections fields should always be available in the REST API without installation.
In parallel, you could include the extra filter fields in the AddonInfo class in core and in the addon.xml schema as part of your PR. I will then add them in the AddonInfoProvider in a second step.

[andrewfg]

the suggested addons finder should actually be a registry in itself, replicating the AddonInfo registry, but with the added functionality of filtering them for suggestions

Umm. That is what I did already. The finder service class also implements the AddonService interface through which it makes available to the core precisely such a filtered list of addons.

---

EDIT: so I wonder is you misunderstood my code; please look at its AddonService:getAddons() method: this returns a flat list of the results of all the other AddonService:getAddons() implementations, filtered to include only those addons which fulfil the requirement of either mdns or upnp having found a potential thing on the lan according to the given search metadata 1).

Note 1): currently the given search metadata is loaded hardcoded from a resource file; but in future the metadata can be loaded from your addon scanner (given that the respective new metadata fields will have been added).

So in short, my code supports AddonService (both as a consumer and as a producer); and I am still not convinced if, or how it should alternately support AddonInfoProvider; this latter seems unnecessary since my code is actually NOT providing AddonInfo, but rather simply filtering a list of AddonInfo provided by others.

[mherwege]

I think we are talking about 2 different things here.
I can see your service is an AddonService. But I am not sure it should be. The suggestion service should not have responsability to install or remove, the suggestion service is only making a recommendation to the UI that then should call the REST endpoint to install the recommended binding when the user accepts it. The only info needed for that is the binding uid and that can come from AddonInfo, doesn't need to come from Addon. Fields in the Addon object are populated from AddonInfo as it becomes available (jar files and bindings from karaf features only have this available at time of installation). And making the info available early enough is what I try to solve.
You also have a circular reference on AddonService. You do filter out itself, but I am not even sure it is possible to have a circular reference between classes like that in OSGI. Not making your class an AddonService would also avoid that.
For installation, it can just call the existing endpoint, no need to have a new AddonService for it. Having that may even lead to problems, because it becomes unclear which of the services is responsible for installation. Addons will be available from multiple services (the suggestion service and the original source). I see you have left the install method blank, but I am not sure that will work out.
I would argue it would be better to have an AddonSuggestionService which looks like the AddonInfoRegistry, so it can directly keep track of the AddonInfoProvider services and get the information for the addons from there, complemented with the necessary methods to provide the suggestions.
The different discovery methods should be in subclasses from one main class, so it can easily be extended.

Ultimately, all of this is my personal view on this. If it is possible to make it work the way you implemented it, and we can remove the need to read a local xml file, I am fine with it. I will keep on trying to get AddonInfo properly populated.

[andrewfg]

service is an AddonService. But I am not sure it should be

I see what you are saying. I can probably implement the getAddons method as it is, but actually without that method being an override of the AddonService interface method of the same name. But in any case I am still pretty sure that the suggestion service is not an AddonInfoProvider (but rather a filterer of data provided by other providers). @mherwege do you agree with my latter assertion?

[mherwege]

Yes, I agree it is not an AddonInfoProvider. It consumes the info that is provided by these. And as in the AddonInfoRegistry, it should keep a list of available AddonInfoProviders and query these. This is only needed because the AddonInfoRegistry does not filter addons (it is 1 or all). Otherwise it could simply use the AddonInfoRegistry. The easy way would be to get all from the registry with each call and just implement filtering in the suggestion service. But I am not sure about the performance. It would be simple though. And that of course relies on the AddonInfo objects containing all relevant info in a timely way.

[andrewfg]

The easy way would be to get all from the registry with each call and just implement filtering in the suggestion service. But I am not sure about the performance.

The existing REST UI code does GET the full list of all addons (via AddonService) in real time. And the performance seems to be OK. So GETting the same list with a stream filter applied, is trivial.

[andrewfg]

In parallel, you could include the extra filter fields in the AddonInfo class in core and in the addon.xml schema as part of your PR.

@mherwege yes I confirm that I will do that. I am currently on vacation so I will probably make a new commit next week.

[andrewfg]

@mherwege in today’s commit, I have implemented your suggestions. And fixed the main build issue.

[andrewfg]

there is no NOTICE file in bundles/org.openhab.core.config.discovery.addon.upnp

Ok. I will add it.

[andrewfg]

BTW I just finished a full system build (post #3865 merging) and I can confirm that it still works :)

[kaikreuzer]

Thanks @andrewfg and @mherwege, this code looks really great to me, thank you so much!
I just have very few comments - see below.

[andrewfg]

Oops. I lost the DCO check here somewhere. @kaikreuzer I suppose you can fix that when you merge?

[kaikreuzer]

Don't worry about the DCO.

[andrewfg]

JavaDoc issue ??

[kaikreuzer]

So all done, @andrewfg?

[andrewfg]

So all done, @andrewfg?

@kaikreuzer I think so :)

[kaikreuzer]

Hooray! Let's wait for the build to be green and we can merge.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-get-an-addon-to-show-up-in-the-new-addon-suggestion-finder-list/152127/8

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/unsuccess-openhab-updated-from-4-0-2-to-4-1-0-on-docker-openhab-unresponsive/152282/13