search

ECF / JaxRSProviders

Remote Services distribution provider based upon JaxRS. Includes imples based upon Jersey and CXF.
Apache License 2.0
13 stars 19 forks source link

Bug 499165 - more generic solution to registering jax-rs components #3

Closed erdalkaraca closed 8 years ago

erdalkaraca commented 8 years ago
scottslewis commented 8 years ago

Hi Erdal,

First, thanks kindly for the pull request and ideas. I'm not going to be able to merge right away though, as I have some other commitments (e.g. travel) today/Friday and over the weekend.

One thing I was thinking about: After looking at the Jax RS configuration API a little bit I was thinking about using the Configurable interface javax.ws.rs.core.Configurable to create a new distribution provider superclass...for potential use by both server and clients. i.e. call it org.eclipse.ecf.provider.jaxrs.JaxRSDistributionProvider. And then re-parent both the server and client so that they can use the same code (similar to what you have in JaxRSExtensionRegistry). That way both clients and servers could easily reuse this capability to use OSGi whiteboard services for extensions, along with using service properties and ds to declare and inject service properties as your code does. I think this will be useful, as I've already got client-provider-side configuration code, and I would like to use the same extension mechanisms for both client and server. This would be common, as usually the same custom marshalling (e.g.) is required on both ends. I think this will likely be true for other cases as well.

I've been toying around with this in some code (new superclass for providers) and will create a new shared branch and push the code to it so that you can look at it and we can discuss...hopefully before I travel today.

Thanks.

erdalkaraca commented 8 years ago

No problem, Scott. Let me know when you have a new draft/solution. I am happy to try it out for my use case.

scottslewis commented 8 years ago

Hi Erdal,

I've created a new branch for the JaxRSProviders repo: slewis499165. The only difference at this point is the addition of this (super) class:

https://github.com/ECF/JaxRSProviders/blob/slewis499165/bundles/org.eclipse.ecf.provider.jaxrs/src/org/eclipse/ecf/provider/jaxrs/JaxRSDistributionProvider.java

The expectation would be that subclasses (impl-specific distribution providers...like the Jersey and CXF providers) would need to

a) Define and register extension with service properties (via DS typically) OR call addJaxRSExtension method directly.

b) When ready to create a Configuration (e.g. when creating individual containers), they would create a Configurable (server or client) and call registerComponents(Configurable).

I haven't had a chance to try any of this out yet, and I want to rename some of the methods (to use 'component' rather than 'extension') but before doing those things I wanted to get your opinion.

scottslewis commented 8 years ago

Actually, rather than have subclasses call registerComponents, they could more easily call: getConfiguration(Configurable)

erdalkaraca commented 8 years ago

Scott, I had a look at the class and I am fine with it :-)

Did you take into account that JAX-RS components could be added after the configuration/container has been created? Is this dynamic behavior also supported by JAX-RS or can we ignore it?

scottslewis commented 8 years ago

Hi Erdal,

Did you take into account that JAX-RS components could be added after the configuration/container >has been created? Is this dynamic behavior also supported by JAX-RS or can we ignore it?

As far as I know, such dynamics are not supported by JAX-RS 2.0. If you know of or discover otherwise please let me know.

scottslewis commented 8 years ago

Hi Erdal,

I've been working on this this weekend, along with understanding Jax-RS better and I think I have something that will work with both servers and clients, and cover all Jax-RS extensions.

To explain: The Jax-RS extension mechanism supports several types of extensions...e.g. MessageBodyReader/Writer, ContextResolvers, and ExceptionMappers. In 2.0 it also supports javax.ws.rs.core.Features, which represent sets of extensions. I added support methods in JaxRSDistribuitonProvider to support these:

https://github.com/ECF/JaxRSProviders/blob/slewis499165/bundles/org.eclipse.ecf.provider.jaxrs/src/org/eclipse/ecf/provider/jaxrs/JaxRSDistributionProvider.java

Also there is all the 'real meat' of actually doing the registration, with contracts and priorities (optionally specified). With this, and the adding of the appropriate DS metadata to the Jersey server and client, both can be configured via OSGi services. I created an example bundle with an extension for the Jersey server of a Student marshalling/unmarshalling customization (MessageBodyReader/Writer). See these components:

https://github.com/ECF/JaxRSProviders/tree/slewis499165/examples/org.eclipse.ecf.provider.jersey.server.example.student.extensions/src/org/eclipse/ecf/provider/jersey/server/example/student/extensions

Note that all that's necessary to make an instance of this class available for registering as an extension to the Jersey server is this:

@Component(property = "jaxrs-component=org.eclipse.ecf.provider.jersey.server.JerseyServerDistributionProvider") public class CustomMessageBodyReaderExtension implements MessageBodyReader ...

When this service component is registered, DS will match it to the org.eclipse.ecf.provider.jersey.server.JerseyServerDistributionProvider and register it to the appropriate Configurable.

Everything works similarly with the client, In fact there are some extensions needed just get it working consistently with json serialization, and so there are some test/examples of the same mechanisms here:

https://github.com/ECF/JaxRSProviders/tree/slewis499165/bundles/org.eclipse.ecf.provider.jersey.client/src/org/eclipse/ecf/provider/jersey/client

Note all of this can and should be reused with CXF. That's something to add.

Since you came up with many of the ideas used, with your permission I would like the copyright in the source to read:

/***

If all of this looks good to you, I will merge the slewis499165 branch into master and drop this pull request. Thanks for your help with this, I like this a lot and think that it will make it much easier to use and extend these Jax-RS providers.

erdalkaraca commented 8 years ago

Hi Scott, I tried the new version and it works for me! What was the reason for having the "jaxrs-component" property? The only use case for it seems to be to be able to differentiate between which JAX-RS implementation the extension is meant for. But isn't there only one implementation only per runtime (for example OSGi instance)? Or can we have both CXF and Jersey running in the same runtime?

Thanks for mentioning me in the copyright headers :-)

scottslewis commented 8 years ago

Hi Scott, I tried the new version and it works for me! What was the reason for having the "jaxrs-component" property? The only use case for it seems to be to be able to differentiate between which JAX-RS >implementation the extension is meant for. But isn't there only one implementation only per runtime > (for example OSGi instance)? Or can we have both CXF and Jersey running in the same runtime?

This is quite possible with ECF's impl of OSGi RSA, so yes this is one use case. Another use case is for the future (not yet implemented), and that's to allow separate Configurables (and associated targeted extensions) at the level of individual ECF containers, which on server-side would correspond to specific paths..e.g. http://host:port/path1/service/method vs http://host:port/path2/service/method running simultaneously.

So part of the reasoning is being able to do multi-distproviders configuration (where both Jersey and CXF providers might exist), and future where containers within dist providers might need distinct configurations.

Thanks for mentioning me in the copyright headers :-)

Most welcome. I'm sorry that I didn't end up using your code on this pull request. Unless you object, I'll close this pull request and merge my branch into master later today/Monday.

erdalkaraca commented 8 years ago

Maybe, then one little addition/question: can we make providing the jaxrs-component property optional? I the MessageBodyWriter/Reader I provide would work in any JAX-RS implementation, so I would only want to opt-in for a specific impl.

I tried this as an ldap filter for the target: |((jaxrs-component=org.eclipse.ecf.provider.jersey.server.JerseyServerDistributionProvider)(jaxrs-component=*))"

And setting the property to "*" in my MessageBodyWriter.

scottslewis commented 8 years ago

Ok. One thing I need to look into is: if a jaxrs impl (e.g. Jersey, CXF, etc) cannot handle/use a given extension (e.g. I imagine that CXF doesn't allow some Jersey-specific features) what is required to happen when the configurable.register(instance,contracts) is called...e.g. exception, log, etc.

But I get what you are saying...in the usual case (a single JaxRS implementation...e.g. Jersey) it is easier to just register the component without any properties required...and you are done.

scottslewis commented 8 years ago

Hi Erdal,

I've removed the requirement that the jaxrs-component property be set. It's now optional, and registering a Jax-RS extension will now be enough.

I've also changed the jaxrs-component property to jaxrs-component-target. If this property is set, then the value specified will be used to call JaxRSDistributionProvider.isValidComponentTarget(Object value). By default this is implemented as checking if the value is a String, and that it equals the Distribution Provider classname. Providers (clients and servers) can modify this implementation, and I might generalize it over time (e.g. support targets that are the container id and/or other things as discussed above).

I've merged my branch into master and deleted the slewis499165 branch.

I'm going to close this bug. Thanks for your help on this. I think I will do a build of this new provider very soon, create a karaf feature for it, and make it known on the Karaf users mailing list.