Initial contribution of an IoT Market extension service #3076

kaikreuzer · 2017-02-26T21:07:55Z

implements #2924

Signed-off-by: Kai Kreuzer kai@openhab.org

maggu2810 · 2017-02-27T07:24:00Z

I assume the file bundles/automation/org.eclipse.smarthome.automation.module.script.rulesupport/.project should not be part of that PR.

kaikreuzer · 2017-02-27T07:39:22Z

thx, removed it.

maggu2810 · 2017-02-27T15:47:46Z

extensions/extensionservice/org.eclipse.smarthome.extensionservice.marketplace/build.properties

+               OSGI-INF/,\
+               about.html,\
+               OSGI-INF/MarketplaceExtensionService.xml,\
+               OSGI-INF/MarketplaceTemplateProvider.xml


The two lines above could be removed, the whole directory is already included.

maggu2810 · 2017-02-27T15:48:08Z

extensions/extensionservice/org.eclipse.smarthome.extensionservice.marketplace/pom.xml

+  <artifactId>org.eclipse.smarthome.extensionservice.marketplace</artifactId>
+  <packaging>eclipse-plugin</packaging>
+
+  <name>Eclipse SmartHome Web Audio Support</name>


I would expect another name 😉

maggu2810 · 2017-02-27T15:49:02Z

...n/java/org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtension.java

+    }
+
+    // we mark it as volatile, so that it isn't serialized through GSON
+    private volatile String downloadUrl;


Isn't transient the correct keyword to disable serialization?

maggu2810 · 2017-02-27T15:51:49Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+                    }
+                    break;
+                default:
+                    postFailureEvent(extensionId, "Id not known.");


A break (also for the last statement) would be nice.

In uninstall you return, so no post(Un)installedEvent is called. Do you want to call return here, too?

kaikreuzer · 2017-02-27T18:21:34Z

@maggu2810 Code has not been cleaned up yet, I mainly pushed it as WIP for testing the new Paper UI features. But thanks anyway, I'll incorporate your feedback (and will let you know once the PR is really ready for review :-))

kaikreuzer · 2017-02-28T16:39:36Z

@maggu2810 Ready for review now - it is still a basic implementation that only covers certain use cases, but I think it is a good starting point.
I tried to make the code clean, fully functional and robust, so it is not just a PoC.

maggu2810

This has been just a first reading of the code, need to check it out later and start a detailed review.

maggu2810 · 2017-02-28T16:42:57Z

...ons/extensionservice/org.eclipse.smarthome.extensionservice.marketplace/META-INF/MANIFEST.MF

+Manifest-Version: 1.0
+Bundle-ManifestVersion: 2
+Bundle-Name: Eclipse SmartHome IoT Marketplace Extension Service
+Bundle-SymbolicName: org.eclipse.smarthome.extensionservice.marketplace


Do we allow different version in the same runtime or only one (singleton:=true)?
I assume we didn't look at this attribute most of the time, but I assume we should consider what we want to support.
As long as we do not use version constraints, we should perhaps use singleton plugins.

I agree, we should make sure to use the singleton flag

maggu2810 · 2017-02-28T16:46:32Z

...n/java/org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtension.java

+    }
+
+    // we mark it as transient, so that it isn't serialized through GSON
+    private transient String downloadUrl;


IMHO member variables should be placed above the constructor.

maggu2810 · 2017-02-28T16:51:49Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+    @Override
+    public void uninstall(String extensionId) {
+        Extension ext = getExtension(extensionId, null);
+        if (ext != null && ext.isInstalled()) {


Above you checked for MarketplaceExtension, do we want to ensure a correct extensionId here to?
If it is not a MarketplaceExtension (also above) should this be ignored silently or should we throw an IllegalArgumentException or another one?

Or perhaps post an failureEvent...

maggu2810 · 2017-02-28T16:52:30Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+                    uninstallBinding(extensionId);
+                    break;
+                default:
+                    postFailureEvent(extensionId, "Id not known.");


Type is not known, if the ID is not known, we don't step into this place at all.

maggu2810 · 2017-02-28T16:52:36Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+                    installBinding(extensionId, mpExt);
+                    break;
+                default:
+                    postFailureEvent(extensionId, "Id not known.");


Type is not known, if the ID is not known, we don't step into this place at all.

maggu2810 · 2017-02-28T17:22:21Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+        if (ext instanceof MarketplaceExtension && !ext.isInstalled()) {
+            MarketplaceExtension mpExt = (MarketplaceExtension) ext;
+            String type = getType(extensionId);
+            switch (type) {


Isn't this a potential NPE as getType may return null.

maggu2810 · 2017-02-28T17:26:49Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+                    String[] parts = line.split(";");
+                    try {
+                        map.put(parts[0], Long.valueOf(parts[1]));
+                    } catch (Exception e) {


You should check for parts.length >= 2 before instead of execute the code and catch exceptions.
So, the only thing to catch is the NumberFormatException.

maggu2810 · 2017-02-28T17:29:14Z

...eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceRuleTemplateProvider.java

+        this.parser = parser;
+    }
+
+    protected void unseteParser(Parser<RuleTemplate> parser) {


unset without an "e" at the end

maggu2810 · 2017-02-28T17:31:15Z

...main/java/org/eclipse/smarthome/extensionservice/marketplace/internal/model/Marketplace.java

+     * The category of the marketplace
+     */
+    public Category[] categories;
+}


We should add a new line at the end of the file.
Without the new line this will lead in noisy diffs in Git (and some quality tools also prefer new lines).
We should change the IDE to do it for us.

maggu2810 · 2017-02-28T17:33:25Z

...ce/src/main/java/org/eclipse/smarthome/extensionservice/marketplace/internal/model/Node.java

+    public String packageformat;
+
+    public String updateurl;
+}


We should add a new line at the end of the file.
Without the new line this will lead in noisy diffs in Git (and some quality tools also prefer new lines).
We should change the IDE to do it for us.

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer · 2017-03-01T16:15:08Z

updated

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer · 2017-03-03T09:39:41Z

Please refrain from a final review for the moment - I just noticed that I need some refactoring to also make it possible to use this extension service, if no automation component is installed...

maggu2810 · 2017-03-03T09:56:03Z

Ah, okay.
My plan has been to do a checkout and tests at the weekend.
Will wait for a ping.

…late) support to a separate bundle Signed-off-by: Kai Kreuzer <kai@openhab.org>

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer · 2017-03-03T14:46:55Z

Ping!
I think the additional time spent was worthwhile - the code is imho much better structured (split into different classes) and it is much easier to add support for additional formats&types through additional bundles.

maggu2810 · 2017-03-04T22:29:15Z

features/karaf/esh-ext/src/main/feature/feature.xml

+    <bundle>mvn:org.eclipse.smarthome.extensionservice/org.eclipse.smarthome.extensionservice.marketplace/${project.version}</bundle>
+  </feature>
+
+  <feature name="esh-extensionservice-marketplace-automation" version="${project.version}">


Does the feature verification complete?
Shouldn't marketplace-automation depend on the marketplace feature?

We do not have feature validation in place at ESH. You are right, it should depend on the marketplace feature.

Updated. Any further review comments?

We do not have feature validation in place at ESH.

You are wrong!

It has been present from the beginning.

commit 8484160501a8eb8fc62f84eef82b44bbf2050877 Date: Mon Apr 18 10:24:48 2016 +0200

Yourself moved it from features/karaf-verify to features/karaf/verify at the beginning of 2017.

Perhaps you remember that we talked on introduction of the Karaf feature if the verifiction should be enabled at build time to be executed automatically or only be in place to be triggered manually.

It's just about the line  in the POM file, if we bind the goal to a phase or not.

To trigger the verification manually, you can do it as usual using this command:

mvn -pl features/karaf/verify org.apache.karaf.tooling:karaf-maven-plugin:verify@verify-esh

Updated. Any further review comments?

Haven't had any time for review, just looked at the features yesterday.
Will try to start a review soon, but it's a matter of spare time...

Signed-off-by: Kai Kreuzer <kai@openhab.org>

maggu2810 · 2017-03-05T10:40:52Z

...rthome/extensionservice/marketplace/automation/internal/MarketplaceRuleTemplateProvider.java

+ * This class provides local access to the market place content. Once started, it downloads the catalog and then makes
+ * its content available from memory.
+ *
+ * Note that there is no progressive/lazy browsing implemented yet, but the service downloads the whole catalog.


Is this comment this correct?
This file does not download anything, does it?

Moved to MarketplaceProxy, correct.
Change comment please

maggu2810 · 2017-03-05T10:43:57Z

...ava/org/eclipse/smarthome/extensionservice/marketplace/internal/BindingExtensionHandler.java

+ * them through the standard OSGi bundle installation mechanism.
+ * The information, which installed bundle corresponds to which extension is written to a file in the bundle's data
+ * store. It is therefore wiped together with the bundles upon an OSGi "clean".
+ * We might want to move this class into a separate bundle in future, when we add support for further extension types.


Wouldn't it be already a good time as we have a different one for the automation one?

If ok for you, I would do that as a follow up PR.

Okay for me if the "follow up PR" is following 😉

maggu2810 · 2017-03-05T10:49:04Z

...ain/java/org/eclipse/smarthome/extensionservice/marketplace/MarketplaceExtensionHandler.java

+ * multiple handers support it, it is undefined which one will be called.
+ * This mechanism allows solutions to add support for specific formats (e.g. Karaf features) that are not supported by
+ * ESH out of the box.
+ * It also allows to decide which extension types are made available at all.


As there is only one handler allowed per type+format and every solution is allowed to add its own type+format handlers, shouldn't the type and format be defined by the handler itself instead adding global constants to the MarketplaceExtension?
So, moving

EXT_TYPE_BINDING and EXT_FORMAT_BUNDLE to BindingExtensionHandler

EXT_TYPE_RULE_TEMPLATE and EXT_FORMAT_JSON to AutomationExtensionHandler

Ah, okay, see the usage below...

The types are statically defined in the IoT marketplace - so if the marketplace is extended, we should also maintain the new types here at a central place as constants.

maggu2810

IMHO we should move the bundle market place handler to a separate binding as it has been done for the automation ones.
Some minor comments.
I assume you already tested that the code is working and I don't need to test it myself.

maggu2810 · 2017-03-05T10:53:28Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+    private void postFailureEvent(String extensionId, String msg) {
+        Event event = ExtensionEventFactory.createExtensionFailureEvent(extensionId, msg);
+        eventPublisher.post(event);
+


Empty line can be removed

maggu2810 · 2017-03-05T10:54:52Z

...org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceExtensionService.java

+            default:
+                return null;
+        }
+        sb.append(node.id.replaceAll("[^a-zA-Z0-9_]", ""));


Isn't the extension ID defined this way (REST): [a-zA-Z_0-9-:]*
You added a colon (:) above but here you remove colons.

We are adding the "marketplace:" prefix for the extension ids and hence the part AFTER that prefix should not contain any additional colons. What is converted here is the node id and this is (from what I have seen so far) always a numerical number anyhow, so this line is only a safeguard for the future.

We are adding the "marketplace:" prefix for the extension ids and hence the part AFTER that prefix should not contain any additional colons.

Ah, okay.

maggu2810 · 2017-03-05T10:55:30Z

...rthome/extensionservice/marketplace/automation/internal/MarketplaceRuleTemplateProvider.java

+ * This class provides local access to the market place content. Once started, it downloads the catalog and then makes
+ * its content available from memory.
+ *
+ * Note that there is no progressive/lazy browsing implemented yet, but the service downloads the whole catalog.


Moved to MarketplaceProxy, correct.
Change comment please

maggu2810 · 2017-03-05T10:58:32Z

.../main/java/org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceProxy.java

+    public MarketplaceProxy() {
+        try {
+            url = new URL(MP_URL);
+            ThreadPoolManager.getScheduledPool("marketplace").scheduleWithFixedDelay(() -> refresh(), 0,


At which place is that pool cleaned up again?
getScheduledPool(...) creates a new if it is not known and put it to a map.
I would expect the MarketProxy should remove ti is it is removed from the system.
But this seems to be a general problem in the ThreadPoolManager, isn't it?
It adds new Pools to its map and never cleans it up. Miss I something?

You are right - I definitely missed to cancel the scheduled job.
And as we do not need a thread pool but only a single executor, I am not using ThreadPoolManager anymore now.

maggu2810 · 2017-03-05T11:04:18Z

We can now install and uninstall a bundle.
After a bundle is installed its state should be INSTALLED.
Who is responsible to start and stop the bundle?
Make it sense to have a service to add and remove bundles if we don't provide a service to start and stop (at least start) the installed bundles?

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer · 2017-03-05T18:20:40Z

Who is responsible to start and stop the bundle?

Normally, the bindings will start by themselve as they have "activate immediately" DS components.
But for this service, I think it makes sense to start bundles automatically as the user wants to add a functionality (and not just populate a local cache). I have therefore added the bundle start to the code. If anyone does not like that, we can also make that a configuration option in future.

@maggu2810 I have pushed my changes, I hope this is ok now. Would be happy to have it merged soon to have an initial version to start testing. The market will only launch officially by the end of March, so we can improve things that come up by follow up PRs.

Signed-off-by: Kai Kreuzer <kai@openhab.org>

maggu2810 · 2017-03-05T21:01:55Z

Normally, the bindings will start by themselve as they have "activate immediately" DS components.

Really?
IIRC the order for start is bundle start, bundle activator, bundle DS. For stop, stop deactivate all DS, deactivate activator, stop bundle.
Activate immediately does IMHO state "the DS should be activated if not used by any other service, too".

Do you think about a lazy activation, that result in a bundle start if the DS inspect the services?

Would be great, if you could give me further details.

maggu2810 · 2017-03-05T21:07:11Z

.../main/java/org/eclipse/smarthome/extensionservice/marketplace/internal/MarketplaceProxy.java

+        try {
+            url = new URL(MP_URL);
+            this.refreshJob = Executors.newSingleThreadScheduledExecutor().scheduleWithFixedDelay(() -> refresh(), 0,
+                    refresh_interval, TimeUnit.SECONDS);


IIRC you should call shutdown after you added the job (https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ExecutorService.html#shutdown()).
If you don't use it, the executor does not "know" about, that it does not accept any further jobs.

final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); this.refreshJob = executor.scheduleWithFixedDelay(() -> refresh(), 0, refresh_interval, TimeUnit.SECONDS); executor.shutdown();

Ok, just pushed a commit to properly shut it down

maggu2810

One minor comment about the executor shutdown.

If you change it or you explain me that it is really not necessary (because stuff is broken doing so), I approve this PR.
if you need it today, you can merge it after the fix yourself (I agree).

maggu2810 · 2017-03-05T21:08:17Z

...ava/org/eclipse/smarthome/extensionservice/marketplace/internal/BindingExtensionHandler.java

+ * them through the standard OSGi bundle installation mechanism.
+ * The information, which installed bundle corresponds to which extension is written to a file in the bundle's data
+ * store. It is therefore wiped together with the bundles upon an OSGi "clean".
+ * We might want to move this class into a separate bundle in future, when we add support for further extension types.


Okay for me if the "follow up PR" is following 😉

kaikreuzer · 2017-03-05T21:10:54Z

Do you think about a lazy activation, that result in a bundle start if the DS inspect the services?

Yes, that's what I thought about, you are right - anyhow, I have changed the code to always do a bundle start.

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer · 2017-03-05T21:18:01Z

Okay for me if the "follow up PR" is following

Yes, I will have to, because I actually plan a different implementation on openHAB side later, which will use felix fileinstall instead of directly installing the bundles. So no risk that this follow-up PR is forgotten :-)

* master: (483 commits) improve cron expression tests (eclipse-archived#3133) Fix eclipse-archived#3132 (eclipse-archived#3134) Display of image items in classic UI and basic UI (eclipse-archived#3055) Implemented tests for org.eclipse.smarthome.core.audio (eclipse-archived#2687) Changes in org.eclipse.smarthome.core.audio: (eclipse-archived#3119) Fix RuleEnablementHandler name (eclipse-archived#3125) run Eclipse formatter rules on Hue binding (eclipse-archived#3123) Bug fix: months shown from 0-11 (eclipse-archived#3124) Hide rule from catalog entry if no rule extensions exist (eclipse-archived#3117) Parsefloat instead if int (eclipse-archived#3118) Move Jekyll preamble to doc build process (out of source files) (eclipse-archived#3116) Initial contribution of an IoT Market extension service (eclipse-archived#3076) Fixes for transformation readmes (eclipse-archived#3111) Eclipse setup: set source 1.8 compliance level (eclipse-archived#3109) Change max / min value types to Object (for better handling of floats) (eclipse-archived#3103) removed not recommended items in lifx README (eclipse-archived#3030) Parameter context docs updated Tycho respect Eclipse settings on Maven build Changed the names of NtpOSGiTest test methods (eclipse-archived#5) NTP binding test: replace Groovy by Java ... # Conflicts: # docs/documentation/development/bindings/xml-reference.md

kaikreuzer force-pushed the iotmarket branch from d5a416a to 24e9601 Compare February 27, 2017 07:39

maggu2810 reviewed Feb 27, 2017

View reviewed changes

kaikreuzer force-pushed the iotmarket branch 2 times, most recently from 737f3f8 to 446abee Compare February 28, 2017 16:36

kaikreuzer changed the title ~~[WIP] Initial contribution of an IoT Market extension service~~ Initial contribution of an IoT Market extension service Feb 28, 2017

kaikreuzer requested a review from maggu2810 February 28, 2017 16:39

maggu2810 reviewed Feb 28, 2017

View reviewed changes

kaikreuzer added 2 commits March 1, 2017 15:49

initial contribution of an IoT Market extension service

973a724

Signed-off-by: Kai Kreuzer <kai@openhab.org>

addressed review comments

a00bcf5

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer force-pushed the iotmarket branch from 446abee to a00bcf5 Compare March 1, 2017 14:50

added content validation

6f9fedc

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer force-pushed the iotmarket branch from 619145c to 6f9fedc Compare March 1, 2017 16:17

kaikreuzer changed the title ~~Initial contribution of an IoT Market extension service~~ [WIP] Initial contribution of an IoT Market extension service Mar 3, 2017

kaikreuzer added 2 commits March 3, 2017 15:41

refactored code to make it extensible and moved automation (rule temp…

84b749e

…late) support to a separate bundle Signed-off-by: Kai Kreuzer <kai@openhab.org>

added bundles to build and features

05f8a03

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer changed the title ~~[WIP] Initial contribution of an IoT Market extension service~~ Initial contribution of an IoT Market extension service Mar 3, 2017

maggu2810 reviewed Mar 4, 2017

View reviewed changes

added missing dependency to Karaf feature

694beb1

Signed-off-by: Kai Kreuzer <kai@openhab.org>

maggu2810 reviewed Mar 5, 2017

View reviewed changes

addressed review comments

dbb298d

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer added 2 commits March 5, 2017 21:32

mark extension as installed, even if starting fails

13bffde

Signed-off-by: Kai Kreuzer <kai@openhab.org>

always set a version for bindings as well

ede593c

Signed-off-by: Kai Kreuzer <kai@openhab.org>

maggu2810 reviewed Mar 5, 2017

View reviewed changes

shutdown executor service on dispose

344a982

Signed-off-by: Kai Kreuzer <kai@openhab.org>

kaikreuzer merged commit d44794e into eclipse-archived:master Mar 5, 2017

kaikreuzer deleted the iotmarket branch March 5, 2017 21:24

kaikreuzer modified the milestone: 0.9.0 Jun 26, 2017

cweitkamp mentioned this pull request Jul 11, 2020

Use the same regex as the install and uninstall method openhab/openhab-core#1543

Merged

Initial contribution of an IoT Market extension service #3076

Initial contribution of an IoT Market extension service #3076

Conversation

kaikreuzer commented Feb 26, 2017 • edited Loading

maggu2810 commented Feb 27, 2017

kaikreuzer commented Feb 27, 2017

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

kaikreuzer commented Feb 27, 2017

kaikreuzer commented Feb 28, 2017

maggu2810 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

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

kaikreuzer commented Mar 1, 2017

kaikreuzer commented Mar 3, 2017

maggu2810 commented Mar 3, 2017

kaikreuzer commented Mar 3, 2017

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

maggu2810 Mar 5, 2017 • 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

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

maggu2810 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

maggu2810 commented Mar 5, 2017

kaikreuzer commented Mar 5, 2017

maggu2810 commented Mar 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Mar 5, 2017

kaikreuzer commented Mar 5, 2017

kaikreuzer commented Feb 26, 2017 •

edited

Loading

maggu2810 Mar 5, 2017 •

edited

Loading