[WIP] Use OSGi Compendium R7 JAX-RS Whiteboard pattern for REST #739

davidgraeff · 2019-04-16T17:01:11Z

Fixes #726

Remove all associated quirks and workarounds ("core.io.rest.optimize")
Simplify RootResource of org.openhab.core.io.rest. We don't need to update
osgi-jax-rs-connector's configuration anymore.
Add JerseyServletContainerInitializer to satisfy pax-web. Should be
removed when pax-web is not used anymore.
No double-service activation anymore
About 3k lines less code by using modern JAX-RS features.
Implementation independent. No glassfish, jersey or any other specific features required.

TODO:

I'm not sure if the JAX-RS specified SSE works in a way that the Sitemap implementation is using it (in a subscription like fashion with sessions). That need to be verified, but not by me as I'm not using sitemaps.
Finish adapting tests. Use test setup of Hue emulation service and spin up real jax-rs instances for testing.

The RESTdoc bundle in the webui repo probably requires an update
to query the introduced RESTservice interface for the REST root.

Signed-off-by: David Graeff [email protected]

maggu2810

Seems to clean up some stuff.

As org.openhab.core.io.rest.sitemap and org.openhab.core.io.rest.sse already use glassfish API, we could also use it here -- IMHO.

It seems Travis and Jenkins CI build fail.

The REST documentation provided by Swagger still works? Okay, you already have written, it need "probably" an update... Did you check?

maggu2810 · 2019-04-17T19:50:24Z

...ore.io.rest/src/main/java/org/eclipse/smarthome/io/rest/internal/resources/RootResource.java

-            if (configuration != null) {
-                Dictionary properties = configuration.getProperties();
+    public void activate(Map<String, Object> properties) {
+        restRoot = (String) properties.getOrDefault(Constants.ROOT_PROPERTY, RESTConstants.REST_URI);


Another option would be to call toString() instead of this cast. I assume it could also be a number if I set 123 in the configuration file.

maggu2810 · 2019-04-17T19:53:53Z

...ore.io.rest/src/main/java/org/eclipse/smarthome/io/rest/internal/resources/RootResource.java

+            httpService.registerServlet(restRoot, servletContainer, initParams, null);
+            logger.info("openHAB REST started on {}", restRoot);
+        } catch (ServletException | NamespaceException e) {
+            logger.warn("Could not start REST API on {}: {}", restRoot, e.getMessage(), e);


Does it make sense to catch that exception?
Shouldn't the caller be informed that the activation failed?

That depends if we see REST as essential. But yeah I'll change that

My understanding has been that the root resource is exactly used for this purpose (REST).
Does it provide any other stuff, so it would make sense to mark it active if the ServletException or NamespaceException has been thrown. So, does the service make sense without the servlet registration succeeded?

maggu2810 · 2019-04-17T19:57:31Z

...ore.io.rest/src/main/java/org/eclipse/smarthome/io/rest/internal/resources/RootResource.java

+    /**
+     * Debounce reloading of the servlet container
+     */
+    private void scheduleReload() {


Are you sure that all methods called by SCR will be called from the same thread?
If we want to be on the safe side, we could add a synchronized to that method.

It should be the one and only service resolution thread in my naive assumption, but yes I agree. That should be synchronized.

IIRC there has been already some comments about that topic.
There is no word in the spec that optional dependencies and the activation etc. are called from the same thread (AFAIK -- but I could be wrong).

davidgraeff · 2019-04-18T05:41:20Z

As org.openhab.core.io.rest.sitemap and org.openhab.core.io.rest.sse already use glassfish API, we could also use it here -- IMHO.

The org.glassfish.jersey.servlet.ServletContainer is glassfish.
Is there other, simpler API to be used from glassfish elsewhere?

The replaced dependency was also using the ServletContainer so I'm not introducing new dependencies to the table.

openhab-bot · 2019-04-18T09:13:02Z

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

https://community.openhab.org/t/setup-maintenance-a-paper-ui-replacement-proposal/72614/7

maggu2810 · 2019-04-18T09:18:26Z

The org.glassfish.jersey.servlet.ServletContainer is glassfish.
Is there other, simpler API to be used from glassfish elsewhere?

I don't know.

The replaced dependency was also using the ServletContainer so I'm not introducing new dependencies to the table.

My comment should already state that it is okay for me. 😉

kaikreuzer · 2019-04-20T02:12:39Z

...ore.io.rest/src/main/java/org/eclipse/smarthome/io/rest/internal/resources/RootResource.java

-    protected void setConfigurationAdmin(ConfigurationAdmin configurationAdmin) {
-        this.configurationAdmin = configurationAdmin;
+    @Override
+    public String getRESTroot() {


CamelCase -> getRESTRoot

kaikreuzer · 2019-04-20T02:15:21Z

bundles/org.openhab.core.io.rest/src/main/java/org/eclipse/smarthome/io/rest/RESTservice.java

+ * 
+ * @author David Graeff - Initial contribution
+ */
+public interface RESTservice {


CamelCase -> RESTService

kaikreuzer · 2019-04-20T02:16:20Z

bundles/org.openhab.core.io.rest/src/main/java/org/eclipse/smarthome/io/rest/RESTservice.java

+ * @author David Graeff - Initial contribution
+ */
+public interface RESTservice {
+    String getRESTroot();


-> getRESTRoot

kaikreuzer · 2019-04-20T02:20:04Z

...ore.io.rest/src/main/java/org/eclipse/smarthome/io/rest/internal/resources/RootResource.java

+    protected @NonNullByDefault({}) HttpService httpService;
+    private @NonNullByDefault({}) ServletContainer servletContainer;
+
+    private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor();


This adds a permanent additional (and during runtime idle) thread. I'd prefer to instead use the common thread pool for this job.

davidgraeff · 2019-04-30T14:12:49Z

I had a good read on OSGi core and compendium releases recently and I'm proposing another solution later that day. It builds on JAX-RS 2.1 for SSE (This closes #743) and the JAX-RS Whiteboard Pattern for REST.
That's possible because we already have Apache Aries as dependency (at least in the BOMs?!).

maggu2810 · 2019-04-30T14:48:50Z

There has been already some work on a Karaf feature for the whiteboard pattern (see Karaf mailing list).
I don't know if the whole part is working in Karaf 4.2.x as it does not support R7 officially (just some compendium implementations already support their part e.g. SCR).

davidgraeff · 2019-04-30T14:54:29Z

Fortunately the JAX-RS whiteboard pattern is part of the compendium specification. We don't need a R7 OSGi core.
And yes according to mailing lists people are using karaf and aries to already have JAX-RS whiteboard.

I will try to remove pax-web or jersey while I'm on it. We don't need 3 competing javax.servlet providing libraries.

maggu2810 · 2019-05-13T07:50:38Z

@davidgraeff Did you move forward?

davidgraeff · 2019-05-13T08:49:28Z

I have a branch that fully works with the R7 compendium Jax-RS whiteboard pattern. Everything based on the standard API, no glassfish or other implemention specific stuff.

I need to adapt this PR's title & description. It is a few more lines to review, because I'm using the newest variant of jax-rs and reworked our REST bundles. SLOC are dropped by 3k.

I just didn't came to test all the finicky SSE parts with the Sitemap bundle, where you register a client first etc. That's so much antipattern, so not really supported by how Jax-RS SSE works, and I think websockets should have been used here. That's why I'm so unmotivated to finish this bundle also considering that sitemaps will be dropped.

* Remove all associated quirks and workarounds ("core.io.rest.optimize") * Simplify RootResource of org.openhab.core.io.rest. We don't need to update osgi-jax-rs-connector's configuration anymore. * Add JerseyServletContainerInitializer to satisfy pax-web. Should be removed when pax-web is not used anymore. * Update all REST bundles to use proper Jax-RS API The RESTdoc bundle in the webui repo probably requires an update to accomodate Signed-off-by: David Graeff <[email protected]>

Signed-off-by: David Graeff <[email protected]>

davidgraeff · 2019-05-15T19:23:23Z

...o.rest.core/src/main/java/org/eclipse/smarthome/io/rest/core/item/EnrichedItemDTOMapper.java

+        } else {
+            try {
+                return TransformationHelper.isTransform(pattern)
+                        ? new StateDescription(desc.getMinimum(), desc.getMaximum(), desc.getStep(), "",


I need to cheery pick #655 changes and apply them to this file again.

Wouldn't it make sense to rebase the working branch on a regular basis?

I rebased, but for this particular file I chose to just take mine to not break the PR. But will reapply the made changes.

Why will this PR be broken if you rebase to the master?

Again: I have rebased. Just for this one file I had conflicts that I didn't feel comfortable to hackish resolve them, so I just kept my changes, but annotated with a comment to remind me to cheery pick the changes back.

maggu2810

I had only a few minutes, ...
Will have a look at the next days.

Or you ping me if you think you are finished.

maggu2810 · 2019-05-15T20:58:08Z

bom/compile/pom.xml

+    <dependency>
+      <groupId>org.osgi</groupId>
+      <artifactId>org.osgi.service.jaxrs</artifactId>
+      <version>1.0.0</version>


That should be already part of org.osgi:osgi.cmpn:7.0.0 (declared above).

maggu2810 · 2019-05-15T20:58:41Z

bom/compile/pom.xml

      <scope>compile</scope>
    </dependency>
-
+    


trailing whitespaces

maggu2810 · 2019-05-15T20:59:27Z

...omation.rest/src/main/java/org/openhab/core/automation/rest/internal/ModuleTypeResource.java

-    protected void unsetLocaleService(LocaleService localeService) {
-        this.localeService = null;
-    }
+@Component(immediate = true)


For here and other places: I assume immediate = true is not necessary at all if the whiteboard pattern is used.

That might be true, not sure.

In contrast to the pure whiteboard pattern found in literature the jax-rs application is created procedural, via code, and not declarative. That's done to be able to change the root path of the application. And I don't know exactly if the components must be started for this to work.

maggu2810 · 2019-05-15T21:02:41Z

...omation.rest/src/main/java/org/openhab/core/automation/rest/internal/ModuleTypeResource.java

-        this.localeService = null;
-    }
+@Component(immediate = true)
+@JaxrsApplicationSelect("(osgi.jaxrs.name=" + RESTService.REST_APP_NAME + ")")


For here and other places: Shouldn't something similar to

@JaxrsApplicationSelect @JaxrsName(RESTService.REST_APP_NAME)

work too?

My understanding has been that the property type has been introduced to not use constructs like "(osgi.jaxrs.name=" + RESTService.REST_APP_NAME + ")" anymore.

Yes I guess so. Eclipse just went nuts on me and didn't resolve JaxrsName when I made those changes.

kaikreuzer · 2019-05-27T21:40:30Z

What's the current state here? I didn't look into the details, but it feels to be a risky change, while if all works out, it is a worthwhile cleanup of the code base.

Background of my question is #597: This seems to be a rather critical issue by now as it occurs almost on every initial setup. If this PR here might soon be ready, we could check if it solves this problem on the way - if not, we will probably have to invest time to solve it in the current code base.

davidgraeff · 2019-05-27T22:38:54Z

It will probably fix the issue. The Jax-rs implementation independent SSE parts are used.

But I'm also stuck because of SSE. It was used in the sitemaps part in a way that it was not intended to be used and the API does not allow the same pattern.

openhab-bot · 2019-05-28T07:48:11Z

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

https://community.openhab.org/t/problem-install-eclipse-ide/75140/22

davidgraeff · 2019-05-30T20:02:32Z

I will not continue to work on this PR. So it might be closed or taken over :)

maggu2810 · 2019-06-18T09:20:42Z

@davidgraeff That's really sad.
I don't use the sitemap, too.
I assume there will be no volunteer.
(At least) for me it seems people are more interested to not change anything instead of moving to a more stable and standardized technology.
The R7 JAX-RS whiteboard pattern would allow us to drop some old outdated unmaintained third party code but without dropping or rewriting the siteamp stuff we cannot move forward.

davidgraeff · 2019-06-18T09:39:39Z

(At least) for me it seems people are more interested to not change anything instead of moving to a more stable and standardized technology.

Exactly. Unfortunately that makes OH unattractive to me. I picked OH because I had hoped to benefit from synergistic effects. Instead I wrote or rewrote all required addons myself (MQTT, Network Binding, Yamaha Binding, Hue Emulation, Deconz, Hue). That's not an issue itself, the community benefits, was my thought.

But this phobic clinging to old concepts here in core is what turns me away tbh. I see huge problems with the current REST interface (openhab/openhab-webui#24 (comment) for details) and updating the software stack is just the tip of the iceberg. And I don't see that changing, now that Yannick has written a new UI that again uses Sitemaps (sigh...).

maggu2810 · 2019-06-18T17:27:26Z

I assume I don't share all your motivation or thinking about a fast moving target as I also need to support commercial products.
But I assume we should think about such topics in general.

Using specifications and simplifications of R7 makes IMHO really sense.
WRT your PR I would like to be interested which developer would like to work on the sitemap codebase or would like to keep them up to date.

If there is no one who would like to work on that code and there are just consumers that state we need to keep such code working, I don't know how to move forward...

@davidgraeff Can you tell me about the work that is done on openHAB 3? Who will contribute? Who will move openHAB forward (not organization but the code base)?

kaikreuzer · 2019-06-19T21:59:28Z

(At least) for me it seems people are more interested to not change anything instead of moving to a more stable and standardized technology.

Not sure if you refer to me? If so, you are wrong here. I was very happy to see this PR and hoped we can get rid off a lot of code and dependencies here. So I share the disappointment that @davidgraeff gives up on it.

I just didn't came to test all the finicky SSE parts with the Sitemap bundle, where you register a client first etc. That's so much antipattern, so not really supported by how Jax-RS SSE works

If this is the part that led you to give up on it, could you at least provide some details on how far you got here, what obstacles there were and what exactly you consider an antipattern that cannot be resolved without removing complete features? Is SSE working for item streams and the problem is only with SSE for sitemaps? Could you exclude the sitemap SSE support from this PR and for the moment keep the Glassfish dependency for it and I can look into it separately?

davidgraeff · 2019-06-19T22:49:35Z

what exactly you consider an antipattern

I haven't read the SSE specification, but the subscribe/unsubscribe pattern that is used by Sitemap has no matching API and I haven't seen it anywhere else. I guess the idiomatic way would be to open / close connections on unique http endpoints if its about unique event streams. Right now some kind of manual multiplexing and session tracking is performed.

exclude the sitemap SSE support from this PR

I can't recall it exactly, but I think that was my first attempt actually. Overlapping APIs and library responsibilities and therefore build and runtime failures made me start to touch that part as well.

maggu2810 · 2019-06-28T10:15:45Z

@davidgraeff While my modifications of the eclipsesource JAX-RS publisher I realized (again) that it uses a custom service tracker to register resources (also for message body reader / writer, our REST resources, ...).
I changed some of that service trackers to DS references to provide a running IDE for the community and this can be done easily instead of this whole migration...
If you have a look at e.g. maggu2810@5b10f8d#diff-d2b15dd840e0a539deed5f80784684eb
we could also change the remaining tracker and the DS references to specific services or e.g. hardcode the specific message body writer and reader impl. Would this be an option to use the old for non migrated stuff and the JAX-RS Whiteboard pattern for the migrated ones?
If we control the publisher and so the registration of all things for the Jersey connector, can't we migrate in smaller steps and use both parallel?

davidgraeff · 2019-06-28T11:10:54Z

Would this be an option

Yes of course. If you get the eclipsesource bundle to run with recent whiteboard pattern compatible jax-rs libraries. At the moment it is hard-wired to jersey .22 afaik. Should be doable.

This PR does not only contain the whiteboard pattern migration (that would be a few hundred lines of annotation changes only). It is also modernizing, by using recent jax-rs features, and cutting down a lot of code. This will also work with the old eclipsesource bundle, if jersey can be updated to a more recent version.

I wanted to get rid of eclipsesource, because of double service activation in the first place. It activates a service to check if it's suitable and shuts it down again. If you have service inter-dependencies, some of your services get activated multiple times, just to get shut down again. Absolute anti-pattern imo.

maggu2810 · 2019-08-09T18:52:13Z

@davidgraeff I played around with SSE today a little bit to migrate some projects to pure javax.ws.rs package space without depending on a specific implementation.
I used the Aries JAX-RS Whiteboard bundle at runtime.

As all the time a new Sse object (of the context) is injected it makes no sense to create a new broadcaster all the time (so for every peer). You could trace the setSee calls yourself.
For me it seems also if a broadcaster does not remove a event sink after registration if this one has been closed at any time e.g. because the peer terminates the connection.

A very simple working implementation can be found here:
https://github.com/maggu2810/playground-osgi/commit/37063f8c8df9781b8f495997aa666fff12152ce6#diff-d969275f7520fc6d63d1ccc458191d88R1

There is one REST endpoint to register for listening and an broadcast interface that can be used by other components to broadcast events.

davidgraeff · 2019-08-10T09:23:07Z

migrate some projects to pure javax.ws.rs package space

👍

As all the time a new Sse object (of the context) is injected, it makes no sense to create a new broadcaster all the time (so for every peer)

I agree. Apparently I did not read the api specification / contract well enough.

It's a slightly different situation for Sitemaps (multiple SSE endpoints) and I think the original code tried to harmonize multiple use cases and I followed along with this implementation.

maggu2810 · 2019-08-10T14:04:00Z

For the documentation (Swagger) you could be perhaps interested in something like that:
https://github.com/maggu2810/playground-osgi/compare/bafb6e4a6d09000b8fd887020fd337c0a19b0be6...a11358ddbd93f221f3f847135366ab79dc5b5380

It uses the wabjars swagger-ui (only the default URL is changed) that is registered using the HTTP Whiteboard spec.
A short demo of documented REST API using the OpenAPI annotations (https://swagger.io/specification/)
A simple oapi generator that currently tracks all JAX RS resources (using the filter "(osgi.jaxrs.resource=true)")

boc-tothefuture · 2019-08-10T14:54:36Z

I think @davidgraeff abandoned this due to sitemaps. I can't tell... @maggu2810 are you picking it up to complete the sitemap side of it?

davidgraeff · 2019-08-10T17:27:04Z

The question is if this can be done in smaller parts. There are quite some merge conflicts accumulating. I guess it needs to be merged in one go though.

maggu2810 · 2019-08-10T19:48:06Z

@maggu2810 are you picking it up to complete the sitemap side of it?

No, I don't.

Signed-off-by: Scott Rushworth [email protected] (github: openhab-5iver)

davidgraeff changed the title ~~Remove osgi-jax-rs-connector dependency, jersey 2.28 update~~ WIP: Remove osgi-jax-rs-connector dependency, jersey 2.28 update Apr 16, 2019

wborn added the work in progress A PR that is not yet ready to be merged label Apr 16, 2019

maggu2810 reviewed Apr 17, 2019

View reviewed changes

kaikreuzer reviewed Apr 20, 2019

View reviewed changes

This was referenced Apr 22, 2019

[hueemulation] Migrate to bnd openhab/openhab-addons#5243

Merged

SSE implementation (Jersey vs. Jetty) #743

Closed

wborn mentioned this pull request May 5, 2019

Add support for running openHAB with Java 11 openhab/openhab-distro#768

Closed

maggu2810 mentioned this pull request May 6, 2019

fix audio tests #799

Closed

davidgraeff force-pushed the ownrest branch from 1fa5ee9 to 28ae2ee Compare May 15, 2019 19:05

David Graeff added 2 commits May 15, 2019 21:18

Use whiteboard pattern. Modernize

c00d2db

Signed-off-by: David Graeff <[email protected]>

davidgraeff force-pushed the ownrest branch from 28ae2ee to c00d2db Compare May 15, 2019 19:20

davidgraeff commented May 15, 2019

View reviewed changes

davidgraeff changed the title ~~WIP: Remove osgi-jax-rs-connector dependency, jersey 2.28 update~~ WIP: Use OSGi Compendium R7 JAX-RS Whiteboard pattern for REST May 15, 2019

maggu2810 reviewed May 15, 2019

View reviewed changes

wborn added the REST/SSE label May 22, 2019

wborn mentioned this pull request May 26, 2019

SSE breaks due to BundleException in SseActivator.start() on first startup #597

Closed

maggu2810 mentioned this pull request Sep 4, 2019

Multiple bundles for Guava and Jetty #892

Closed

cweitkamp changed the title ~~WIP: Use OSGi Compendium R7 JAX-RS Whiteboard pattern for REST~~ [WIP] Use OSGi Compendium R7 JAX-RS Whiteboard pattern for REST Oct 2, 2019

cweitkamp added the help wanted label Oct 2, 2019

cweitkamp mentioned this pull request Oct 10, 2019

Remove JAX-RS Connector (com.eclipsesource.jaxrs) dependency #726

Closed

davidgraeff closed this Mar 30, 2020

Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020

Update jsr223.md (openhab#739)

cb42e0a

Signed-off-by: Scott Rushworth [email protected] (github: openhab-5iver)

[WIP] Use OSGi Compendium R7 JAX-RS Whiteboard pattern for REST #739

[WIP] Use OSGi Compendium R7 JAX-RS Whiteboard pattern for REST #739

Conversation

davidgraeff commented Apr 16, 2019 • edited Loading

maggu2810 left a comment • 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

davidgraeff commented Apr 18, 2019

openhab-bot commented Apr 18, 2019

maggu2810 commented Apr 18, 2019

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

davidgraeff commented Apr 30, 2019

maggu2810 commented Apr 30, 2019

davidgraeff commented Apr 30, 2019

maggu2810 commented May 13, 2019

davidgraeff commented May 13, 2019 • 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

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

kaikreuzer commented May 27, 2019

davidgraeff commented May 27, 2019 • edited Loading

openhab-bot commented May 28, 2019

davidgraeff commented May 30, 2019

maggu2810 commented Jun 18, 2019 • edited Loading

davidgraeff commented Jun 18, 2019

maggu2810 commented Jun 18, 2019

kaikreuzer commented Jun 19, 2019

davidgraeff commented Jun 19, 2019

maggu2810 commented Jun 28, 2019

davidgraeff commented Jun 28, 2019 • edited Loading

maggu2810 commented Aug 9, 2019 • edited Loading

davidgraeff commented Aug 10, 2019

maggu2810 commented Aug 10, 2019

boc-tothefuture commented Aug 10, 2019

davidgraeff commented Aug 10, 2019

maggu2810 commented Aug 10, 2019

davidgraeff commented Apr 16, 2019 •

edited

Loading

maggu2810 left a comment •

edited

Loading

davidgraeff commented May 13, 2019 •

edited

Loading

davidgraeff commented May 27, 2019 •

edited

Loading

maggu2810 commented Jun 18, 2019 •

edited

Loading

davidgraeff commented Jun 28, 2019 •

edited

Loading

maggu2810 commented Aug 9, 2019 •

edited

Loading