Initial implementation of ConfigOptionProvider interface #495

cdjackson · 2015-10-18T12:33:07Z

As described here.

This PR allows a binding to provide custom options in a config description. This information is provided within the CD request through a GET thing/{thingUID}/config REST interface.

Signed-off-by: Chris Jackson chris@cd-jackson.com

dnobel · 2015-10-18T15:19:08Z

Is this really what was discussed in the forum? I thought that the REST interface and the ConfigDescriptionRegistry must not be changed. As far as I remember the idea was to just request a regular config description and internally it adds the dynamic values from the ConfigOptionProvider.

cdjackson · 2015-10-18T15:24:17Z

Yes - I referenced the discussion above and explained why we have to change the REST interface. The REST has to change unless you make a GET /thingtype/{thingUID}/config call - passing the thingUID into a thingtype API seems to me like a bad idea? Or do you think there's a better way?

Regarding the Registry - Kai said he wanted to keep the same registry (ie not add another one) but if we don't change something, how can we add a ConfigOptionProvider? Can you suggest how better to change this?

dnobel · 2015-10-18T15:29:38Z

Sorry, next time I will read the forum first :). I will comment it later or tomorrow. Did you close it accidentally?

cdjackson · 2015-10-18T15:32:00Z

Did you close it accidentally?

Ooops - yes - I hate the placement of those buttons...

Thanks.

maggu2810 · 2015-10-19T16:43:23Z

...e.config.core/src/main/java/org/eclipse/smarthome/config/core/ConfigDescriptionRegistry.java

+     * the binding to ensure that multiple sources (eg static XML and dynamic binding data) do not contain overlapping
+     * information.
+     *
+     * @param uri


javadoc parameter does not fit

Agreed - this PR needs a tidy (and tests). It’s provided early for comment to see if it even fits the concept as I don’t want to waste time if it’s not something that’s going to be merged.

kaikreuzer · 2015-10-19T20:01:33Z

@dnobel, please also see and comment on https://www.eclipse.org/forums/index.php?t=msg&th=1066227&goto=1711802&#msg_1711802 before further reviewing this PR.

dnobel · 2015-10-19T21:09:32Z

Yes, I have read it. It seems as we all have a different understanding of the agreed solution.

@maggu2810 Let´s wait with the detailed review until we finally agreed on the solution. Feel free to join the discussion.

kaikreuzer · 2015-10-19T21:10:25Z

It seems as we all have a different understanding of the agreed solution.

Where do you see the two of us differ?

dnobel · 2015-10-19T21:14:33Z

Where do you see the two of us differ?

If you fully agree on my last forum entry, I think there is no difference any more.

kaikreuzer · 2015-10-19T21:17:16Z

If you fully agree on my last forum entry, I think there is no difference any more.

The second possibility is that if I fully agree on your last entry, I didn't read it carefully enough ;-)

kaikreuzer · 2015-10-27T05:02:13Z

@cdjackson Will you update this PR according to the latest discussion at https://www.eclipse.org/forums/index.php?t=msg&th=1066227&goto=1712198&#msg_1712198?

cdjackson · 2015-10-27T06:56:41Z

Hi Ksi
Yes. Will try and take a look in the next week. Have been tied up with different things and traveling over the past few days.
Chris

cdjackson · 2015-11-07T18:30:11Z

As presented on the forum...

This now adds a ThingConfigDescriptionProvider. This is a proxy for things - it gets the config information from the thing-type, and if the thingHandler implements the ConfigOptionProvider interface, it will call the getParameterOptions() method to get a list of options which will be merged with the static options.

The following code shows an example implementation of the getParameterOptions method for zwave where we might want to add options for a certain subset of options (this is just an example implementation - the final will be more dynamic depending on the actual thing).

A new REST resource (GET config-description/URI) is implemented to get the configuration.

    @Override
    public Collection<ParameterOption> getParameterOptions(String param, Locale locale) {
        // Is it an association group?
        if (!param.startsWith("group_")) {
            return null;
        }

        List<ParameterOption> options = new ArrayList<ParameterOption>();
        options.add(new ParameterOption("node1", "Node 1"));
        options.add(new ParameterOption("node1", "Node 3"));

        return options;
    }

Any comments appreciated... As per the last message on the forum, there is another implementation option which makes the ConfigOptionProvider a service, but I think this is simplest given that the ThingConfigDescriptionProvider knows all about things...

kaikreuzer · 2015-11-09T16:38:01Z

...ome.io.rest.core/src/main/java/org/eclipse/smarthome/io/rest/core/config/ConfigResource.java

@@ -0,0 +1,118 @@
+package org.eclipse.smarthome.io.rest.core.config;


pls add license header

cdjackson · 2015-11-14T10:02:48Z

Updated and also taken account of comments.

dnobel · 2015-11-14T10:11:40Z

I just realized that this PR also contains a ConfigDescription REST resource. I have implemented more or less the same thing in #517 . Sorry for that. My PR contains a ConfigDescriptionDTO, a mapper class and also takes the language into account, including Swagger annotations.

Maybe we could remove the REST resource here. The REST resource in the other PR should smoothly work together with this one. But if you want, you can also copy my changes into this PR.

maggu2810 · 2015-12-02T10:20:33Z

I have no such detailed knowledge about that topic as @dnobel, @kaikreuzer and @cdjackson but for me this looks good.

maggu2810 · 2015-12-02T10:24:18Z

Please fix the "getConfigDescriptions" function of the "ThingConfigDescriptionProvider" class.

See note last week: https://github.com/eclipse/smarthome/pull/495/files#r45886784

cdjackson · 2015-12-02T11:55:21Z

On 2 Dec 2015, at 11:24, Markus Rathgeb notifications@github.com wrote:

Please fix the "getConfigDescriptions" function of the "ThingConfigDescriptionProvider" class.

I understood that this was not needed - as per the discussion. I don’t think it’s necessary - what is the use case?

maggu2810 · 2015-12-02T12:04:04Z

I understood that this was not needed - as per the discussion.

Why is this not needed? Could you point me to the discussion?

+/**
+ * The {@link ConfigOptionProvider} can be implemented and registered as an <i>OSGi</i>
+ * service to provide {@link ConfigDescription}s options.
+ *
+ * @author Chris Jackson - Initial contribution
+ */
...
+public interface ConfigOptionProvider {
+     * @return the configuration options provided by this provider (not
+     *         null, could be empty)
+     */
+    Collection<ParameterOption> getParameterOptions(URI uri, String param, Locale locale);

I don’t think it’s necessary - what is the use case?

You defined the API / the interface, that this function must not return null, so the implementation must not return null.

Perhaps I am missing some point...

cdjackson · 2015-12-02T14:01:45Z

Why is this not needed? Could you point me to the discussion?

I think the discussion above said that we don’t need to incorporate this method and we can just return an empty list? Maybe I’m wrong?

I don’t think it’s necessary - what is the use case?

You defined the API / the interface, that this function must not return null, so the implementation must not return null.

Perhaps I am missing some point…

I didn’t define the API - this is part of the framework. I just say that I don’t think there’s a use case for returning the configuration for all things, so I didn’t implement this method.

maggu2810 · 2015-12-02T14:06:31Z

I think the discussion above said that we don’t need to incorporate this method and we can just return an empty list?

That is what I would like to say, return an empty list or collection, but do not return null.

I didn’t define the API - this is part of the framework.

Isn't the interface "ConfigOptionProvider" of package "org.eclipse.smarthome.config.core" created by this PR and documented with "Chris Jackson - Initial contribution"
See: https://github.com/eclipse/smarthome/pull/495/files#diff-b9dcb1cc180e9f66b1e19ba7edbb60e7

cdjackson · 2015-12-02T14:14:43Z

On 2 Dec 2015, at 15:06, Markus Rathgeb notifications@github.com wrote:

I think the discussion above said that we don’t need to incorporate this method and we can just return an empty list?

That is what I would like to say, return an empty list or collection, but do not return null.

It does return an empty list already? -:
return Collections.emptySet();

Maybe I’m missing something still, or we’re talking cross terms?

I didn’t define the API - this is part of the framework.

Isn't the interface "ConfigOptionProvider" of package "org.eclipse.smarthome.config.core" created by this PR and documented with "Chris Jackson - Initial contribution"
See: https://github.com/eclipse/smarthome/pull/495/files#diff-b9dcb1cc180e9f66b1e19ba7edbb60e7 https://github.com/eclipse/smarthome/pull/495/files#diff-b9dcb1cc180e9f66b1e19ba7edbb60e7Well yes, but I’m just overriding methods already defined, so I implemented this file - that doesn’t mean I defined the API.

maggu2810 · 2015-12-02T14:25:57Z

or we’re talking cross terms?

Curious, the diff now is fine (but the one javadoc of the parameter), as of time of writing the above message(s) my browser showed me a diff, that still contains the "return null;".
Don't know If this has been related to an outdated browser cache or if I should go to bed / drink more coffee. 😉

kaikreuzer · 2015-12-02T14:27:47Z

So all if fine then? Green light from me as well, so I leave it to @maggu2810 to merge!

cdjackson · 2015-12-02T14:30:44Z

On 2 Dec 2015, at 15:26, Markus Rathgeb notifications@github.com wrote:

I should go to bed / drink more coffee.

Drink more coffee and stay awake :)

maggu2810 · 2015-12-02T14:50:05Z

I think the java doc line needs to be fixed, then I will merge it: https://github.com/eclipse/smarthome/pull/495/files#r46411332

Or is my browser out of date again?

Signed-off-by: Chris Jackson <chris@cd-jackson.com> (+4 squashed commits) Squashed commits: [20b0989] Revert config description provider Signed-off-by: Chris Jackson <chris@cd-jackson.com> [dc1c53c] Updated PR - also fixes comments Signed-off-by: Chris Jackson <chris@cd-jackson.com> [77f12b8] Updated to support ThingConfigDescriptionProvider Signed-off-by: Chris Jackson <chris@cd-jackson.com> [2278c69] Initial implementation of ConfigOptionProvider interface Signed-off-by: Chris Jackson <chris@cd-jackson.com>

maggu2810 · 2015-12-03T12:46:46Z

@cdjackson Please add a comment to the PR if you add a new commit. Github does not send a notification if commits are added, a rebase or a squash is done...

Initial implementation of ConfigOptionProvider interface

maggu2810 · 2015-12-03T12:47:29Z

@cdjackson Thank you!

kaikreuzer · 2015-12-03T13:05:26Z

I have locally many failing tests through this PR, most with an NPE like

java.lang.NullPointerException: null
    at org.eclipse.smarthome.core.thing.internal.ThingConfigDescriptionProvider.getConfigDescription(ThingConfigDescriptionProvider.java:55)
    at org.eclipse.smarthome.config.core.ConfigDescriptionRegistry.getConfigDescription(ConfigDescriptionRegistry.java:146)
    at org.eclipse.smarthome.config.core.ConfigDescriptionRegistry.getConfigDescription(ConfigDescriptionRegistry.java:182)
    at org.eclipse.smarthome.core.thing.binding.ThingFactory.createThing(ThingFactory.java:114)
    at org.eclipse.smarthome.core.thing.binding.BaseThingHandlerFactory.createThing(BaseThingHandlerFactory.java:273)
    at org.eclipse.smarthome.core.thing.setup.ThingSetupManager.createThing(ThingSetupManager.java:675)
    at org.eclipse.smarthome.core.thing.setup.ThingSetupManager.addThing(ThingSetupManager.java:590)
    at org.eclipse.smarthome.core.thing.setup.ThingSetupManager.addThing(ThingSetupManager.java:583)
    at org.eclipse.smarthome.core.thing.setup.ThingSetupManager.addThing(ThingSetupManager.java:162)

Did any of you successfully run the tests locally?

maggu2810 · 2015-12-03T13:08:28Z

I will have a look at, didn't run a local build before merging.

maggu2810 · 2015-12-03T14:12:37Z

First try: Builds fine. Will try again.
Which artifact raises that exception.

kaikreuzer · 2015-12-03T14:14:19Z

Let's wait for the result of https://hudson.eclipse.org/smarthome/job/SmartHomeDistribution-Nightly/665/
Maybe it is only a problem locally on my machine.

kaikreuzer · 2015-12-03T14:33:42Z

This is the same result that I had on my machine: https://hudson.eclipse.org/smarthome/job/SmartHomeDistribution-Nightly/666/testReport/

maggu2810 · 2015-12-03T15:05:14Z

Ah, the line 55...
I already stumbled around that bug a weeg ago, or so.
It is related if the getConfigDescription is called with a null URI.

if ("thing".equals(uri.getScheme()) == false) {

The error that raises that NPE on my machine has been fixed by me, see #535

I will have a look at the call stack, but I think getConfigDescription must not be called with a null URI. The caller should check for null, not that function. But it is not defined for the interface.

maggu2810 · 2015-12-03T15:10:26Z

Tests run: 9, Failures: 0, Errors: 8, Skipped: 0, Time elapsed: 0.14 sec <<< FAILURE! - in org.eclipse.smarthome.core.thing.setup.ThingSetupManagerOSGiTest
enableAndDisableChannel(org.eclipse.smarthome.core.thing.setup.ThingSetupManagerOSGiTest)  Time elapsed: 0.032 sec  <<< ERROR!
java.lang.NullPointerException: null
    at org.eclipse.smarthome.core.thing.internal.ThingConfigDescriptionProvider.getConfigDescription(ThingConfigDescriptionProvider.java:55)
    at org.eclipse.smarthome.config.core.ConfigDescriptionRegistry.getConfigDescription(ConfigDescriptionRegistry.java:146)
    at org.eclipse.smarthome.config.core.ConfigDescriptionRegistry.getConfigDescription(ConfigDescriptionRegistry.java:182)
    at org.eclipse.smarthome.core.thing.binding.ThingFactory.createThing(ThingFactory.java:114)

org.eclipse.smarthome.config.core.ConfigDescriptionRegistry.getConfigDescription(...) states, that the argument URI must not be null.

org.eclipse.smarthome.core.thing.binding.ThingFactory.createThing(ThingFactory.java:114) calls getConfigDescription without checking for null...

            ConfigDescription thingConfigDescription = configDescriptionRegistry
                    .getConfigDescription(thingType.getConfigDescriptionURI());

maggu2810 · 2015-12-03T15:42:26Z

URI org.eclipse.smarthome.core.thing.type.ThingType.getConfigDescriptionURI()

Returns the link to a concrete ConfigDescription.

Returns:
the link to a concrete ConfigDescription (could be null)

"return value: could be null" used as an "argument that must not be null".

I will create a PR

maggu2810 · 2015-12-03T15:51:23Z

Fix #648

cdjackson force-pushed the configOptionProvider branch from 73409b2 to 2278c69 Compare October 18, 2015 14:11

dnobel added the feedback label Oct 18, 2015

cdjackson closed this Oct 18, 2015

cdjackson reopened this Oct 18, 2015

maggu2810 reviewed Oct 19, 2015
View reviewed changes

cdjackson force-pushed the configOptionProvider branch from b499f4c to 9a9b36f Compare November 7, 2015 18:21

cdjackson force-pushed the configOptionProvider branch 4 times, most recently from ab91502 to 77f12b8 Compare November 7, 2015 18:43

kaikreuzer reviewed Nov 9, 2015
View reviewed changes

cdjackson force-pushed the configOptionProvider branch from f32931b to 93be952 Compare December 2, 2015 20:54

kaikreuzer removed the feedback label Dec 3, 2015

kaikreuzer assigned maggu2810 Dec 3, 2015

maggu2810 added a commit that referenced this pull request Dec 3, 2015

Merge pull request #495 from cdjackson/configOptionProvider

3fa40ed

Initial implementation of ConfigOptionProvider interface

maggu2810 merged commit 3fa40ed into eclipse-archived:master Dec 3, 2015

maggu2810 mentioned this pull request Dec 3, 2015

ThingConfigDescriptionProvider fix DS #649

Merged

cdjackson deleted the configOptionProvider branch January 21, 2016 22:41

		@@ -0,0 +1,118 @@
		package org.eclipse.smarthome.io.rest.core.config;

Initial implementation of ConfigOptionProvider interface #495

Initial implementation of ConfigOptionProvider interface #495

Conversation

cdjackson commented Oct 18, 2015

dnobel commented Oct 18, 2015

cdjackson commented Oct 18, 2015

dnobel commented Oct 18, 2015

cdjackson commented Oct 18, 2015

maggu2810 Oct 19, 2015

Choose a reason for hiding this comment

cdjackson Oct 19, 2015

Choose a reason for hiding this comment

kaikreuzer commented Oct 19, 2015

dnobel commented Oct 19, 2015

kaikreuzer commented Oct 19, 2015

dnobel commented Oct 19, 2015

kaikreuzer commented Oct 19, 2015

kaikreuzer commented Oct 27, 2015

cdjackson commented Oct 27, 2015

cdjackson commented Nov 7, 2015

kaikreuzer Nov 9, 2015

Choose a reason for hiding this comment

cdjackson commented Nov 14, 2015

dnobel commented Nov 14, 2015

maggu2810 commented Dec 2, 2015

maggu2810 commented Dec 2, 2015

cdjackson commented Dec 2, 2015

maggu2810 commented Dec 2, 2015

cdjackson commented Dec 2, 2015

maggu2810 commented Dec 2, 2015

cdjackson commented Dec 2, 2015

maggu2810 commented Dec 2, 2015

kaikreuzer commented Dec 2, 2015

cdjackson commented Dec 2, 2015

maggu2810 commented Dec 2, 2015

maggu2810 commented Dec 3, 2015

maggu2810 commented Dec 3, 2015

kaikreuzer commented Dec 3, 2015

maggu2810 commented Dec 3, 2015

maggu2810 commented Dec 3, 2015

kaikreuzer commented Dec 3, 2015

kaikreuzer commented Dec 3, 2015

maggu2810 commented Dec 3, 2015

maggu2810 commented Dec 3, 2015

maggu2810 commented Dec 3, 2015

maggu2810 commented Dec 3, 2015