Change mDNS add-on discovery syntax

[mherwege]

Depends on openhab/openhab-core#3924

This PR modifies all mDNS discovery configurations in all addon.xml files that define mDNS discovery to an adapted format.

For this PR to build, the modified addon.xml schema file needs to be uploaded first.

[andrewfg]

LGTM

[kaikreuzer]

Thanks!

[morph166955]

Is the new scheme a requirement for existing bindings or is this just a future proofing? Will anything break having it in the old configuration? I have marketplace users who are on 4.0 who are complaining about errors at startup now with this in place. Changes like this, while I do understand moving forward, make it very hard for those of us who maintain marketplace builds to do so.

[mherwege]

This should not affect marketplace bindings.

This functionality is for proposing add-ons to be installed. Marketplace bindings are not included in that search at all. Unless you have added add-on disocery information in the marketplace add-on it does not change anything.

[morph166955]

I think you're misunderstanding. There are many bindings which maintain a marketplace presence for both beta features as well as older releases. AndroidTV for example does both. In general I compile against the most recent main branch and add on the new features. Unfortunately, since we have people on slightly older versions of OH including OH4.0 they are now getting an error because of this. It makes it very hard for those of us who try to maintain support for the part of the community that isn't looking to be on snapshot releases for breaking changes to be introduced unnecessarily. While I understand the reason for this PR, if there wasn't a specific need for the old/existing bindings to be changed, then IMHO they should have been left alone. Users on 4.0 now get this at startup:

2024-01-21 17:11:06.788 [WARN ] [re.xml.osgi.XmlDocumentBundleTracker] - The XML document '/OH-INF/addon/addon.xml' in module 'org.openhab.binding.androidtv' could not be parsed: 
---- Debugging information ----
cause-exception     : com.thoughtworks.xstream.mapper.CannotResolveClassException
cause-message       : discovery-methods
class               : java.util.ArrayList
required-type       : java.util.ArrayList
converter-type      : com.thoughtworks.xstream.converters.collections.CollectionConverter
path                : /addon/discovery-methods
line number         : 11
class[1]            : org.openhab.core.addon.internal.xml.AddonInfoXmlResult
required-type[1]    : org.openhab.core.addon.internal.xml.AddonInfoXmlResult
converter-type[1]   : org.openhab.core.addon.internal.xml.AddonInfoConverter
version             : 1.4.20
-------------------------------
com.thoughtworks.xstream.converters.ConversionException: 
---- Debugging information ----
cause-exception     : com.thoughtworks.xstream.mapper.CannotResolveClassException
cause-message       : discovery-methods
class               : java.util.ArrayList
required-type       : java.util.ArrayList
converter-type      : com.thoughtworks.xstream.converters.collections.CollectionConverter
path                : /addon/discovery-methods
line number         : 11
class[1]            : org.openhab.core.addon.internal.xml.AddonInfoXmlResult
required-type[1]    : org.openhab.core.addon.internal.xml.AddonInfoXmlResult
converter-type[1]   : org.openhab.core.addon.internal.xml.AddonInfoConverter
version             : 1.4.20
-------------------------------

[mherwege]

I don’t think this is related to this PR. The functionality concerned by this PR was introduced in a commit on Dec 7th. This commit changed the syntax on Dec 16th, one week later. There were only snapshots in between.
And indeed, this is an enhancement that is required to make new functionality possible. An old version will not be able to parse this, but without it, the new functionality about suggesting add-ons is not possible. So I you want to make a version available for an older version, you will have to take this part out of the xml. You cannot assume the new version just works for an older core.

[mlobstein]

I think you're misunderstanding. There are many bindings which maintain a marketplace presence for both beta features as well as older releases. AndroidTV for example does both. In general I compile against the most recent main branch and add on the new features. Unfortunately, since we have people on slightly older versions of OH including OH4.0 they are now getting an error because of this.

Marketplace jars should have the <discovery-methods> removed from addon.xml if they support older versions. And since Marketplace addons do not get searched for suggestions, the presence of <discovery-methods> in a Marketplace jar does not have any effect.

[andrewfg]

The existing previously defined elements in the xml schema have not changed. So they should work as before. The discovery methods are new elements added to the xml schema. So I would assume that older code that did not know about such elements would simply ignore them. Or?? => Can you give an example of the errors that you see?

[morph166955]

@andrewfg see above error that a 4.0 release is throwing on this.

So again, while I don't disagree with change and progress, my problem is when that change and progress introduces breaking compatibility changes unnecessarily. For example, if a "discovery.xml" was created for this purpose it would have accomplished the exact same goal, allowed for the new functionality, and not caused this issue. If there is an intent to have a marketplace then it needs to be understood that the marketplace has many bindings that span releases.

[mlobstein]

The existing previously defined elements in the xml schema have not changed. So they should work as before. The discovery methods are new elements added to the xml schema. So I would assume that older code that did not know about such elements would simply ignore them. Or?? => Can you give an example of the errors that you see?

But those elements did not exist in 4.0.0. So if they are present in a marketplace jar, the addon.xml will fail to parse if the user is using 4.0.0 I believe @morph166955's message above shows what that error looks like. Also prior to 4.0.0, addon.xml did not even exist, so a marketplace jar that supports 3.X.0 will need to include binding.xml alongside of addons.xml.

[mlobstein]

So again, while I don't disagree with change and progress, my problem is when that change and progress introduces breaking compatibility changes unnecessarily.

Just remove the entire <discovery-methods> section from addons.xml and all is good. Think of it as just one more step in the convoluted process of creating a Marketplace jar that is cross version compatible.

[morph166955]

3.X.0 will need to include binding.xml alongside of addons.xml.

This is a PERFECT point. There was no harm done when moving binding.xml to addons.xml. For those which provide backwards support we simply just add the extra files and the others are ignored. In the case of this feature, modifying the xml with a breaking change caused an issue. As I stated above, this could have been done with a discovery.xml just as easily and not been breaking to prior releases.

[mherwege]

I still don’t see the point. I understand the issue if you use a 4.1 binding on a 4.0 or before core. But you do not have to put that new section in your addons.xml. It will work without in 4.1. If it is to support multiple versions in the marketplace, you can just leave the section out. It has no value for marketplace bindings anyway.

[morph166955]

The point is that this was an unnecessary breaking change to the xml schema which causes issues with backwards compatibility. Breaking changes should always be avoided if there is a non breaking alternative. This should be changed to move this information to a new xml file (discovery.xml or something similar) and removed from addons.xml. Additionally, that change should be cherry picked back in to the next 4.1.x release.

[mherwege]

I understand your concern. We might have made other choices if we had foreseen this. I don't see an easy way to solve it now though.

This PR is just the top of a series of PR's, done by myself, mainly @andrewfg and others in this repo, the core repo and the distro repo. This has been reviewed extensively by multiple people and approved.

To change this now would require:

1. Separate discovery.xml from addons.xml in the addons repo.
2. Change the distro to aggregate discovery.xml instead of addons.xml. Note: this will break the availability of extra add-on info (such as country, local vs cloud...) that allows better selection and filtering in the UI. Without this, the information is not available before installing the add-on. This relies on this aggregation.
3. Change all of the core logic to create separate classes for add-on discovery and extract it from the addonInfo class. Create separate readers for this. The AddonInfo registry cannot be used, a new and specific one needs to be created.
4. Change all add-on finders to use this modified class and the original class as well (info in there also needed).

Doing all this, while possible, is a major effort and may not make it to even 4.2 before everything is developed, tested and approved. And I think the solution is more complex and makes maintaining this harder.
I still think the logical place for this information is the addon.xml file. This is meant to carry all relevant add-on information. Adding a new xml file with the same scope (the add-on) every time we introduce new functionality doesn't feel right. Maybe we need to look into the code reading the xml file for it to be more forgiving when reading something that is not supported in the version of core and ignoring it (with a warning at best) to avoid this same situation with any schema enhancement in the future.

Now, this is my personal opinion on the matter and if anyone wants to step up and do all this to create a backfix to allow current version bindings to run on old core versions, please do so. I will not do the investment, especially as there are easy work-arounds.

[andrewfg]

^
This change is indeed regrettable. It is indeed a backwards compatibility breaking change. However IMHO it is not worse than many other backwards compatibility breaking changes .. for example the many recent changes in addons to Use Java 17 features that will not compile for OH v3.x or earlier..

[Skinah]

Agree with @mherwege
We should change the core to fail gracefully, to log the issue and then ignore and carry on working for unrecognized stuff.
If it's only a matter of moving from 4 to 4.1 I do not think that's a big ask of a user and letting them know we chabged the core to handle it better next time should be enough. Would be nice to. Make this known in the forum so people know to go a fetch older versions and not grind wheels if they hit this.