Modified finder discovery schema to make future finders easier to create #3924

mherwege · 2023-12-14T09:11:53Z

In the add-on schema, we currently have a special field for mDNS discovery: mdns-service-type.

As noted in #3920, new add-on finders may also require special parameters. Rather than extending the schema each time, this PR changes the schema to include a parameter section and adapts the mDNS finder to read the mdnsServiceType from a parameter.

While not strictly necessary to make this change for the 4.1 release, it would be good to do it now to avoid future changes in the schema.

The proposed schema section then becomes:

<discovery-method>
 	<service-type>mdns</service-type>
 	<discovery-parameters>
 	 	<discovery-parameter>
 	 	 	<name>mdnsServiceType</name>
 	 	 	<value>...</value>
 	 	</discovery-parameter>
  	</discovery-parameters>
 	<match-properties>
		<match-property>
 			<name>...</name>
 			<regex>...</regex>
 		</match-property>
 	</match-properties>
</discovery-method>

A follow-up PR will be created to change the addon.xml files in the addons repository.

This PR is code complete but requires a full build for testing.

EDIT: Full build done and tested. All is still behaving as expected. Tests run through fine.

Signed-off-by: Mark Herwege <[email protected]>

mherwege · 2023-12-14T09:13:33Z

@andrewfg @J-N-K @kaikreuzer Could you please have a look at this?

Signed-off-by: Mark Herwege <[email protected]>

andrewfg

Generally it looks good.

But I think the string "mdnsServiceType" should be a constant.

Also I have not actually tested yet on a full build.

andrewfg · 2023-12-14T11:25:31Z

...y.addon.mdns/src/main/java/org/openhab/core/config/discovery/addon/mdns/MDNSAddonFinder.java

@@ -148,6 +149,12 @@ && propertyMatches(matchProperties, APPLICATION, service.getApplication())
        return result;
    }

+    private String getMdnsServiceType(AddonDiscoveryMethod method) {
+        String param = method.getParameters().stream().filter(p -> "mdnsServiceType".equals(p.getName()))


"mdnsServiceType" should be a constant.

andrewfg · 2023-12-14T11:26:55Z

...s/src/test/java/org/openhab/core/config/discovery/addon/mdns/tests/MDNSAddonFinderTests.java

@@ -103,12 +104,12 @@ private void setupAddonInfos() {
        AddonDiscoveryMethod hp = new AddonDiscoveryMethod().setServiceType(AddonFinderConstants.SERVICE_TYPE_MDNS)
                .setMatchProperties(
                        List.of(new AddonMatchProperty("rp", ".*"), new AddonMatchProperty("ty", "hp (.*)")))
-                .setMdnsServiceType("_printer._tcp.local.");
+                .setParameters(List.of(new AddonParameter("mdnsServiceType", "_printer._tcp.local.")));


andrewfg · 2023-12-14T11:27:07Z

...s/src/test/java/org/openhab/core/config/discovery/addon/mdns/tests/MDNSAddonFinderTests.java

        addonInfos.add(AddonInfo.builder("hpprinter", "binding").withName("HP").withDescription("HP Printer")
                .withDiscoveryMethods(List.of(hp)).build());

        AddonDiscoveryMethod hue = new AddonDiscoveryMethod().setServiceType(AddonFinderConstants.SERVICE_TYPE_MDNS)
-                .setMdnsServiceType("_hue._tcp.local.");
+                .setParameters(List.of(new AddonParameter("mdnsServiceType", "_hue._tcp.local.")));


andrewfg · 2023-12-14T11:27:18Z

...src/test/java/org/openhab/core/config/discovery/addon/tests/AddonSuggestionServiceTests.java

@@ -125,13 +126,13 @@ private void setupMockAddonInfoProvider() {
        AddonDiscoveryMethod hp = new AddonDiscoveryMethod().setServiceType(AddonFinderConstants.SERVICE_TYPE_MDNS)
                .setMatchProperties(
                        List.of(new AddonMatchProperty("rp", ".*"), new AddonMatchProperty("ty", "hp (.*)")))
-                .setMdnsServiceType("_printer._tcp.local.");
+                .setParameters(List.of(new AddonParameter("mdnsServiceType", "_printer._tcp.local.")));


Done, but with local constant. Otherwise an import of MDNSAddonFinder is required. This would create a circular import.

andrewfg · 2023-12-14T11:27:28Z

...src/test/java/org/openhab/core/config/discovery/addon/tests/AddonSuggestionServiceTests.java


        AddonDiscoveryMethod hue1 = new AddonDiscoveryMethod().setServiceType(AddonFinderConstants.SERVICE_TYPE_UPNP)
                .setMatchProperties(List.of(new AddonMatchProperty("modelName", "Philips hue bridge")));

        AddonDiscoveryMethod hue2 = new AddonDiscoveryMethod().setServiceType(AddonFinderConstants.SERVICE_TYPE_MDNS)
-                .setMdnsServiceType("_hue._tcp.local.");
+                .setParameters(List.of(new AddonParameter("mdnsServiceType", "_hue._tcp.local.")));


Done, but with local constant. Otherwise an import of MDNSAddonFinder is required. This would create a circular import.

andrewfg · 2023-12-14T11:29:13Z

...les/org.openhab.core.addon/src/test/java/org/openhab/core/addon/AddonInfoListReaderTest.java

+        assertEquals(1, parameters.size());
+        AddonParameter parameter = parameters.get(0);
+        assertNotNull(parameter);
+        assertEquals("mdnsServiceType", parameter.getName());


"mdnsServiceType" should be a constant

Left as is, as this would also require a local constant and the exact name of the parameter is not relevant for the test.

andrewfg · 2023-12-14T11:29:46Z

.../org.openhab.core.addon/src/test/java/org/openhab/core/addon/AddonInfoRegistryMergeTest.java

@@ -67,7 +60,7 @@ private AddonInfoProvider createAddonInfoProvider0() {

    private AddonInfoProvider createAddonInfoProvider1() {
        AddonDiscoveryMethod discoveryMethod = new AddonDiscoveryMethod().setServiceType("mdns")
-                .setMdnsServiceType("_hue._tcp.local.");
+                .setParameters(List.of(new AddonParameter("mdnsServiceType", "_hue._tcp.local.")));


"mdnsServiceType" should be a constant

Left as is, as this would also require a local constant and the exact name of the parameter is not relevant for the test.

Signed-off-by: Mark Herwege <[email protected]>

andrewfg

LGTM

J-N-K · 2023-12-14T17:31:50Z

I have just one question: would it make sense to also have a more generic expression than regex?

andrewfg · 2023-12-14T18:21:10Z

a more generic expression

@J-N-K what is your use case? what do you mean?

J-N-K · 2023-12-14T18:51:51Z

I have nothing special in mind. I was just wondering if some new service could use something different than a regex (e.g. glob-like matching) and pattern would be a more generic fit.

andrewfg · 2023-12-14T19:12:38Z

wondering if some new service could use something different than a regex

IMHO a regex would cover all potential use cases.

mherwege · 2023-12-14T21:23:53Z

I have nothing special in mind. I was just wondering if some new service could use something different than a regex (e.g. glob-like matching) and pattern would be a more generic fit.

Regex is self explaining. Calling it pattern will require external documentation and I don’t see anything that cannot be covered by a regex. So I vote for keeping it as is.

mherwege · 2023-12-15T16:49:01Z

I tested this locally with a full build (including this PR and the add-on PR). I receive the same add-on suggestions as before, so this is working as expected.

Signed-off-by: Holger Friedrich <[email protected]>

Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer · 2023-12-16T10:10:15Z

FTR: xsd has been updated on the website with openhab/website@f3d354d.

Signed-off-by: Holger Friedrich <[email protected]>

mherwege added 2 commits December 14, 2023 09:27

change discovery method schema

300cd4e

Signed-off-by: Mark Herwege <[email protected]>

fix tests

f078ada

Signed-off-by: Mark Herwege <[email protected]>

mherwege requested a review from a team as a code owner December 14, 2023 09:11

rename parameter to avoid name conflict

011644b

Signed-off-by: Mark Herwege <[email protected]>

mherwege mentioned this pull request Dec 14, 2023

Change mDNS add-on discovery syntax openhab/openhab-addons#16060

Merged

fix xml parsing

8577681

Signed-off-by: Mark Herwege <[email protected]>

andrewfg suggested changes Dec 14, 2023

View reviewed changes

andrewfg mentioned this pull request Dec 14, 2023

Add-on suggestion finder revised schema openhab/openhab-docs#2174

Merged

reviewer feedback

9a08aba

Signed-off-by: Mark Herwege <[email protected]>

mherwege changed the title ~~[WIP] Modified finder discovery schema to make future finders easier to create~~ Modified finder discovery schema to make future finders easier to create Dec 14, 2023

AddonInfoReader changes

2fab660

Signed-off-by: Mark Herwege <[email protected]>

andrewfg approved these changes Dec 14, 2023

View reviewed changes

holgerfriedrich mentioned this pull request Dec 15, 2023

Service to suggest addons based on running processes #3904

Merged

holgerfriedrich added a commit to holgerfriedrich/openhab-addons that referenced this pull request Dec 16, 2023

adapt to openhab/openhab-core#3924

a0ae876

Signed-off-by: Holger Friedrich <[email protected]>

holgerfriedrich added a commit to holgerfriedrich/openhab-core that referenced this pull request Dec 16, 2023

adapt to openhab#3924

b63db40

Signed-off-by: Holger Friedrich <[email protected]>

holgerfriedrich mentioned this pull request Dec 16, 2023

Service to suggest addons via generic IP scan #3920

Merged

kaikreuzer approved these changes Dec 16, 2023

View reviewed changes

kaikreuzer merged commit fe242f8 into openhab:main Dec 16, 2023
3 checks passed

kaikreuzer added a commit to openhab/website that referenced this pull request Dec 16, 2023

Update addons.xsd for openhab/openhab-core#3924

f3d354d

Signed-off-by: Kai Kreuzer <[email protected]>

holgerfriedrich added a commit to holgerfriedrich/openhab-core that referenced this pull request Dec 16, 2023

adapt to openhab#3924

ded7540

Signed-off-by: Holger Friedrich <[email protected]>

mherwege deleted the finder_discovery branch December 16, 2023 11:22

holgerfriedrich added a commit to holgerfriedrich/openhab-addons that referenced this pull request Dec 17, 2023

adapt to openhab/openhab-core#3924

2b03093

Signed-off-by: Holger Friedrich <[email protected]>

holgerfriedrich added this to the 4.1 milestone May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified finder discovery schema to make future finders easier to create #3924

Modified finder discovery schema to make future finders easier to create #3924

mherwege commented Dec 14, 2023 •

edited

Loading

mherwege commented Dec 14, 2023

andrewfg left a comment

andrewfg Dec 14, 2023

mherwege Dec 14, 2023

andrewfg Dec 14, 2023

mherwege Dec 14, 2023

andrewfg Dec 14, 2023

mherwege Dec 14, 2023

andrewfg Dec 14, 2023

mherwege Dec 14, 2023

andrewfg Dec 14, 2023

mherwege Dec 14, 2023 •

edited

Loading

andrewfg Dec 14, 2023

mherwege Dec 14, 2023

andrewfg Dec 14, 2023

mherwege Dec 14, 2023

andrewfg left a comment

J-N-K commented Dec 14, 2023

andrewfg commented Dec 14, 2023

J-N-K commented Dec 14, 2023

andrewfg commented Dec 14, 2023

mherwege commented Dec 14, 2023

mherwege commented Dec 15, 2023

kaikreuzer commented Dec 16, 2023

Modified finder discovery schema to make future finders easier to create #3924

Modified finder discovery schema to make future finders easier to create #3924

Conversation

mherwege commented Dec 14, 2023 • edited Loading

mherwege commented Dec 14, 2023

andrewfg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mherwege Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewfg left a comment

Choose a reason for hiding this comment

J-N-K commented Dec 14, 2023

andrewfg commented Dec 14, 2023

J-N-K commented Dec 14, 2023

andrewfg commented Dec 14, 2023

mherwege commented Dec 14, 2023

mherwege commented Dec 15, 2023

kaikreuzer commented Dec 16, 2023

mherwege commented Dec 14, 2023 •

edited

Loading

mherwege Dec 14, 2023 •

edited

Loading