osgi / bugzilla-archive

Archive of OSGi Alliance Specification Bugzilla bugs. The Specification Bugzilla system was decommissioned with the move to GitHub. The issues in this repository are imported from the Specification Bugzilla system for archival purposes.
0 stars 1 forks source link

[rfc 222][ds] Implementations of ServletContextHelper with DS #2658

Closed bjhargrave closed 8 years ago

bjhargrave commented 9 years ago

Original bug ID: BZ#2790 From: @cziegeler Reported version: R6

bjhargrave commented 9 years ago

Comment author: @cziegeler

Implementations of the ServletContextHelper should be of scope BUNDLE, especially for the resource handling. When doing an implementation of a ServletContextHelper with DS, it's not possible to pass the using bundle to the abstract ServletContextHelper as the bundle can only be set using the constructor. It seems this has partially been discussin in https://osgi.org/members/bugzilla/show_bug.cgi?id=2619 already and we came up with the current solution.

If we would add an additional setBundle method, own implementations could be easily done with DS

Of course, if we leave it like it is, it's still possible to use DS for an implementation. It is just a little bit more work

bjhargrave commented 9 years ago

Comment author: @tjwatson

The helper should be registered as a ServiceFactory. If I was to do this with DS it would look like this. Which I admit does not really use the spirit of DS, but is not really much work.

@ Component class MyHelper implements ServiceFactory{ // add any @ References needed

@ Activate protected void activate(BundleContext context) { context.register(ServletContextHelper.class, this, null); } protected void deactivate(BundleContext context) { // unregister the helper }

// implement the getService and ungetService to create the helper for a bundle. }

bjhargrave commented 9 years ago

Comment author: @bjhargrave

(In reply to Thomas Watson from comment BZ#1)

The helper should be registered as a ServiceFactory. If I was to do this with DS it would look like this. Which I admit does not really use the spirit of DS, but is not really much work.

@ Component

You better add service={} otherwise DS will register a ServiceFactory service out of your component!

class MyHelper implements ServiceFactory{ // add any @ References needed

@ Activate protected void activate(BundleContext context) { context.register(ServletContextHelper.class, this, null); } protected void deactivate(BundleContext context) { // unregister the helper }

// implement the getService and ungetService to create the helper for a bundle. }

bjhargrave commented 9 years ago

Comment author: @cziegeler

(In reply to Thomas Watson from comment BZ#1)

The helper should be registered as a ServiceFactory. If I was to do this with DS it would look like this. Which I admit does not really use the spirit of DS, but is not really much work.

@ Component class MyHelper implements ServiceFactory{ // add any @ References needed

@ Activate protected void activate(BundleContext context) { context.register(ServletContextHelper.class, this, null); } protected void deactivate(BundleContext context) { // unregister the helper }

// implement the getService and ungetService to create the helper for a bundle. }

Why do you register the SCH in activate?

bjhargrave commented 9 years ago

Comment author: @tjwatson

(In reply to Carsten Ziegeler from comment BZ#3)

Why do you register the SCH in activate?

Because I am unsure how you register a real ServiceFactory from DS. If you know how then it should be even easier, right?

bjhargrave commented 9 years ago

Comment author: David Jencks <djencks@us.ibm.com>

DS already works by registering a ServiceFactory, so you can't too. I think the DS way to do this is with a wrapper class component with scope bundle. The component would implement ServletContextHelper and delegate to the actual class of interest, would receive the bundle in the activate method, and would create the delegate in the activate method using the bundle.

bjhargrave commented 9 years ago

Comment author: @cziegeler

(In reply to David Jencks from comment BZ#5)

DS already works by registering a ServiceFactory, so you can't too. I think the DS way to do this is with a wrapper class component with scope bundle. The component would implement ServletContextHelper and delegate to the actual class of interest, would receive the bundle in the activate method, and would create the delegate in the activate method using the bundle.

Right, this was my conclusion to implement it as well, but in this case although SCH is an abstract class, the component registered as a service factory needs to reimplement all methods just to pass it down to the bundle aware instance - which defeats the purpose of the abstract class. If we add a simple setBundle method to SCH you can do: @ Component(service=ServletContextHelper.class, scope=ServiceScope.BUNDLE) class MyHelper extends ServletContextHelper { @ Activate protected void activate(BundleContext context) { this.setBundle(context.getBundle()); } }

and then just overwrite the methods where you want different behaviour, if at all.

bjhargrave commented 9 years ago

Comment author: @tjwatson

(In reply to Carsten Ziegeler from comment BZ#6)

(In reply to David Jencks from comment BZ#5)

DS already works by registering a ServiceFactory, so you can't too. I think the DS way to do this is with a wrapper class component with scope bundle. The component would implement ServletContextHelper and delegate to the actual class of interest, would receive the bundle in the activate method, and would create the delegate in the activate method using the bundle.

Right, this was my conclusion to implement it as well, but in this case although SCH is an abstract class, the component registered as a service factory needs to reimplement all methods just to pass it down to the bundle aware instance - which defeats the purpose of the abstract class. If we add a simple setBundle method to SCH you can do: @ Component(service=ServletContextHelper.class, scope=ServiceScope.BUNDLE) class MyHelper extends ServletContextHelper { @ Activate protected void activate(BundleContext context) { this.setBundle(context.getBundle()); } }

and then just overwrite the methods where you want different behaviour, if at all.

Not sure what you are trying to do by using the bundle from the BundleContext there. I guess you only want to load resources from the helper bundle and not from the bundles that register the WB http servlets? I would have thought you would use the bundle passed into activate as David suggested.

@ Component(service=ServletContextHelper.class, scope=ServiceScope.BUNDLE) class MyHelper extends ServletContextHelper { @ Activate protected void activate(Bundle bundle) { this.setBundle(bundle); } }

I also assume setBundle is protected. This means that the Bundle field in ServletContextHelper cannot be final. The bundle field will now need to be accessed with thread safety in mind. Not arguments against this, just pointing out the things that need to be done if we want to do this. I agree it is possible and would make the DS scenario easier. I'm as

bjhargrave commented 9 years ago

Comment author: @cziegeler

(In reply to Thomas Watson from comment BZ#7)

(In reply to Carsten Ziegeler from comment BZ#6)

(In reply to David Jencks from comment BZ#5)

DS already works by registering a ServiceFactory, so you can't too. I think the DS way to do this is with a wrapper class component with scope bundle. The component would implement ServletContextHelper and delegate to the actual class of interest, would receive the bundle in the activate method, and would create the delegate in the activate method using the bundle.

Right, this was my conclusion to implement it as well, but in this case although SCH is an abstract class, the component registered as a service factory needs to reimplement all methods just to pass it down to the bundle aware instance - which defeats the purpose of the abstract class. If we add a simple setBundle method to SCH you can do: @ Component(service=ServletContextHelper.class, scope=ServiceScope.BUNDLE) class MyHelper extends ServletContextHelper { @ Activate protected void activate(BundleContext context) { this.setBundle(context.getBundle()); } }

and then just overwrite the methods where you want different behaviour, if at all.

Not sure what you are trying to do by using the bundle from the BundleContext there. I guess you only want to load resources from the helper bundle and not from the bundles that register the WB http servlets? I would have thought you would use the bundle passed into activate as David suggested.

@ Component(service=ServletContextHelper.class, scope=ServiceScope.BUNDLE) class MyHelper extends ServletContextHelper { @ Activate protected void activate(Bundle bundle) { this.setBundle(bundle); } }

I just realized that all these examples are wrong - now as this is a service factory (bundle scope), the idea is that the implementation uses the using bundle and not the bundle where this is implemented. So the activate should look like this: @ Activate protected void activate(ComponentContext ctx) { this.setBundle(ctx.getUsingBundle()); }

I also assume setBundle is protected.
Yes

This means that the Bundle field in ServletContextHelper cannot be final.
Yes, which is not a problem

The bundle field will now need to be accessed with thread safety in mind. Not arguments against this, just pointing out the things that need to be done if we want to do this. I agree it is possible and would make the DS scenario easier. I'm as

Right, we need to take care of this.

It's a minor change but makes things much easier

bjhargrave commented 9 years ago

Comment author: @bjhargrave

I guess this is where constructor injection would be nice!

bjhargrave commented 9 years ago

Comment author: @cziegeler

(In reply to BJ Hargrave from comment BZ#9)

I guess this is where constructor injection would be nice!

Absolutely :)

bjhargrave commented 9 years ago

Comment author: @bjhargrave

Move to R7 as use case for DS constructor injection.

bjhargrave commented 8 years ago

Comment author: @bosschaert

Typically a ServletContextHelper subclass would take a ComponentContext as constructor argument. Then the constructor could call super(cc.getUsingBundle())

Chicago F2F: added as requirement to RFC 222.