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

davidgraeff commented 5 years ago

What does this dependency do?

It scans all OSGi services (by STARTING THEM!!) for JAX-RS @Path or @Provided annotations and adds them to a servlet.

The problems with this dependency

Unfortunately the package is not maintained (no major update since 5 years, no commit in general since 3 years). It is not using newer OSGi features (the entire codebase would shrink by half or more), it uses tycho as its main buildsystem with respective outdated examples, and it does really bad things like actually starting up services to find annotations.

I debugged my Hue Emulation Service for probably 3 hours to find out that double service starts are caused by that bundle. Especially if you do a lot of heavy initialization in activate() (even if async) to throw that away moments later, that adds a lot to OH start up time.

Solution

We have the huge advantage in openHAB, that all our REST endpoints not only are annotated with @Path or @Provided but also implement RestResouce and register themselves to the rest core bean. We can therefore spin up our own servlet in about 20 lines of code without this really badly designed dependency.

Example code (without debouncing/restarting to accommodate for dynamically appearing endpoints):

ResourceConfig resourceConfig = ResourceConfig.forApplicationClass(JerseyApplication.class);

resourceConfig.property(ServerProperties.APPLICATION_NAME, "openHAB");

// don't look for implementations described by META-INF/services/*
resourceConfig.property(ServerProperties.METAINF_SERVICES_LOOKUP_DISABLE, false);

// disable auto discovery (class path scanning)
resourceConfig.property(ServerProperties.FEATURE_AUTO_DISCOVERY_DISABLE, true);
resourceConfig.property(ServerProperties.PROCESSING_RESPONSE_ERRORS_ENABLED, true);

resourceConfig.registerInstances(allOurRestResourceInstancesList);

try {
    Hashtable<String, String> initParams = new Hashtable<>();
    initParams.put("com.sun.jersey.api.json.POJOMappingFeature", "false");
    initParams.put(ServletProperties.PROVIDER_WEB_APP, "false");
    httpService.registerServlet("/rest", new ServletContainer(resourceConfig), initParams, null);
} catch (ServletException | NamespaceException e) {
}

Notes

This dependency is not related to com.eclipsesource.jaxrs.provider.swagger which is also used by openHAB. provider.swagger configures swagger via a properties file and is apart from that unrelated to jax-rs.

cweitkamp commented 5 years ago

Thanks for adding the bounty @J-N-K !

I added the "help wanted" and "PR pending" labels as there already is a pending PR (#739) waiting for someone to jump in and continue working on it.

maggu2810 commented 5 years ago

IMHO we should migrate to the usage of the R7 JAX-RS whiteboard pattern. If the migration is finished, this change will be obsolete.

@davidgraeff You already summarized what the publisher is doing. For reference I would like to add also the following link: https://github.com/hstaudacher/osgi-jax-rs-connector#publisher

I did not test it myself, but what do you think about that idea:

Do not try to implement the change you comment here. As it is not necessary if the whiteboard pattern is used.
Try to be able to use the old JAX-RS implementation and a R7 JAX-RS Whiteboard implementation at the same time (on transition phase).
The Whiteboard pattern requires the OSGi property osgi.jaxrs.resource e.g. by annotate the resources with @JaxrsResource. As this is not present on the "old" REST interface implementations they will not be consumed at all by the whiteboard pattern.
Migrated ones should be ignored by the old one: Add a little "hack" to the current publisher implementation so we could mark REST resources to be ignored by the current publisher. The hack should not affect the existing REST resources and the bad behaviour you comment above (it keeps the same as before). The simplest machanism would be to ignore the classes that are annotated with the respective "new" annotations or that service contains the "new" properties.

That way we could use both mechanism for the transition phase and migrate one by one.

WDYT?

davidgraeff commented 5 years ago

The JAX-RS Connector ignores routes with a special annotation (forgot the name). A R7 whiteboard route could have this annotation until JAX-RS Connector is removed.

My migration PR to the whiteboard pattern did more than just using a different API though. There is a lot of inconsistency of where DTOs and DTO mappers are located. I partly cleaned that up. Just as a warning, that a "1 by 1" migration will still touch code across different core bundles and probably shift code around.

maggu2810 commented 5 years ago

https://github.com/hstaudacher/osgi-jax-rs-connector/blob/master/bundles/com.eclipsesource.jaxrs.publisher/src/com/eclipsesource/jaxrs/publisher/internal/Activator.java#L94-L99

If my reading is correct if a ResourceFilter service is present on activation time, this one is used. Otherwise the AllResourceFilter is used that uses the following filter itself: "(&(objectClass=*)(!(" + PUBLISH + "=false)))" (https://github.com/hstaudacher/osgi-jax-rs-connector/blob/master/bundles/com.eclipsesource.jaxrs.publisher/src/com/eclipsesource/jaxrs/publisher/internal/AllResourceFilter.java#L24)

At the time I looked at your PR I realized that there is much more cleanup done than just the migration. Great work! Thank you again.

IMHO it is easier to work with upstream if small and separate (by topic) improvements are provided instead of a big PR that clean ups multiple stuff. That's the reason for my migration path.

kaikreuzer commented 5 years ago

Afair, the only "problematic" REST endpoints were the ones of the sitemaps (or even more specifically, the subscriptions for sitemap changes). So if those would be kept for the moment, the rest could be migrated without much need for a step-by-step migration or am I overlooking something?

maggu2810 commented 5 years ago

If even only one implementation is kept that requires the old implementation we need a way to use both implementations in parallel.

So, I don't see any reason to require to first migrate all except one. It does not make any difference if we need both in parallel if the migration is step by step. I assume work can be done by different consumers if the base architectural change has been done to support both implementations.

Another option is to wait for a volunteer that invest all the work to migrate all REST implementation except the sitemap one and do a long review session.

maggu2810 commented 5 years ago

I made it! :wink: Migrated to JAX-RS Whiteboard -- also the UI Sitemap REST endpoint. Created a JAX-RS Whiteboard Swagger 1 generator, so I could drop all the eclipsesource dependencies and the annoying OSGi service impacts. The openHAB REST Doc UI is working as ever (or perhaps better than before).

wborn commented 5 years ago

That sounds like a very nice accomplishment! :smile: :1st_place_medal:

kaikreuzer commented 5 years ago

Sounds awesome, @maggu2810! I'd suggest to apply this change right after the 2.5 release - until then, we should can imho live with the status quo.

maggu2810 commented 4 years ago

I realized that the Web UI "cometvisu" also uses the old JAX-RS implementations:

bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/CvActivator.java:import org.glassfish.jersey.media.sse.SseFeature;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/async/BlockingAsyncBinder.java:import org.glassfish.hk2.utilities.binding.AbstractBinder;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/async/BlockingAsyncBinder.java:import org.glassfish.jersey.internal.inject.CustomAnnotationLiteral;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/async/BlockingAsyncBinder.java:import org.glassfish.jersey.servlet.spi.AsyncContextDelegateProvider;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/async/BlockingAsyncContextDelegateProvider.java:import org.glassfish.jersey.media.sse.SseFeature;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/async/BlockingAsyncContextDelegateProvider.java:import org.glassfish.jersey.servlet.spi.AsyncContextDelegate;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/async/BlockingAsyncContextDelegateProvider.java:import org.glassfish.jersey.servlet.spi.AsyncContextDelegateProvider;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/backend/ReadResource.java:import org.glassfish.jersey.media.sse.EventOutput;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/backend/ReadResource.java:import org.glassfish.jersey.media.sse.SseBroadcaster;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/backend/ReadResource.java:import org.glassfish.jersey.media.sse.SseFeature;
bundles/org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/util/SseUtil.java:import org.glassfish.jersey.media.sse.OutboundEvent;

What's your plan?

--

WRT the 2.5.x branches of openhab-webui and openhab2-addons the following bundles needs be migrated:

org.openhab.ui.cometvisu
org.openhab.ui.cometvisu.php (perhaps not really, but it depends on the previous one)
org.openhab.io.hueemulation
org.openhab.io.neeo
org.openhab.transform.jinja
org.openhab.binding.lametrictime
org.openhab.binding.neeo
org.openhab.binding.nest
org.openhab.binding.sleepiq
org.openhab.binding.tesla

wborn commented 4 years ago

I made it! :wink:

Will you be creating PR(s) for solving this issue @maggu2810? That would be highly appreciated! There's quite a significant as reward for solving it as well. :slightly_smiling_face:

maggu2810 commented 4 years ago

Sorry @wborn, but currently I don't plan to put any effort into openHAB development. There are more important things for me to do right now.

wborn commented 4 years ago

That's unfortunate to hear @maggu2810, but at least we know someone else will have to put effort into this. Do you perhaps have some branches with your (WIP) changes on GitHub that we can use as source of inspiration?

splatch commented 4 years ago

What's take on this one? Is it on the roadmap for 3.0?

wborn commented 4 years ago

Yes it certainly is. That's why I ask around if there are still contributors looking into fixing it. If there are none I might just have to look into it myself one day. :wink:

Any previous efforts regarding this would certainly help. There's also David's branch as source of inspiration.

kaikreuzer commented 4 years ago

I also would love to see this solved for 3.0 and I would hope that there isn't even too much work left as @maggu2810 apparently already made it work. I very much hope that he can provide us a link to his work, so that somebody can take it over from there instead of starting from scratch, which would be a real pity.

maggu2810 commented 4 years ago

You can have a look at into this branch: https://github.com/maggu2810/openhab-core/tree/for-openhab-upstream-to-look-into

This commits should be of your interest:

wborn commented 4 years ago

Many thanks for the branch and pointers to commits @maggu2810! I'll certainly have a look at it. :+1:

wborn commented 4 years ago

I started porting those commits to the current code base @maggu2810! Let's hope it results in something useful. :wink:

wborn commented 4 years ago

We made it! :-) The code by @maggu2810 was also top-notch and reusing it saved a lot of time. :+1:

kaikreuzer commented 4 years ago

Yes, awesome work by @maggu2810, a big thanks to him as well!

openhab / openhab-core