ECF / JaxRSProviders

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

Create simpler and more flexible endpoint descriptions #29

Closed modular-mind closed 3 years ago

modular-mind commented 3 years ago

I'm creating this issue really just to start a discussion. In my work with the JAX-RS client in the past I've found the EDEF xml files to be difficult to work with. There are two main issues:

  1. When creating a simple REST client, I think there are really only two properties that are unique: the endpoint URI (ecf.endpoint.id) and the remote service interface (objectClass). The rest of the properties (I think) can be set to defaults or generated randomly.

  2. The endpoint URI often needs to be configured for different environments (test/beta/prod).

My solution has been to create an extension point (these are RCP apps) called "endpointDescriptions" that has only these two attributes. When multiple environments are needed, I've had the extension reference a properties file listing all of the URIs. Finally, an OSGi service called an EndpointManager reads these from the extensions on startup and then generates and registers the actual EndpointDescriptions.

I was wondering what your opinion of a best practice is in this context. I've looked into Config Admin of course, but I want to keep this extremely simple to allow for easy integration into Eclipse RCP applications. I'm sure extension points are not ideal when looking at other non-Eclipse environments like Karaf. Is there an easier way to get this done?

scottslewis commented 3 years ago

Hi Patrick.

First let me explain why things are the way they are now (and a little about how they are now).

EDEF itself is specified by the OSGi remote services and rsa chapters (100 and 122). The edef format is simply an xml expresssion of an arbitrary set of properties (name->type:value pairs). The remote services spec, however, has a set of properties that must (or optionally) be present...under different service import/export circumstances. For the Jersey provider, as you say there are really only a couple properties that change and the others can be set to defaults or generated randomly.

ECF implements the spec and so the normal rsa mechanism generates edef on export, and then typically uses discovery to publish and discover those edefs. Just FYI: As per spec, there is a 'topology manager'...which can be replaced...that listens for discovered endpointdescriptions, and with default topology manager simply calls RemoteServiceAdmin.importEndpoint(EndpointDescription ed) to actually import the endpoint description.

For your use case, however, since you are constructing and 'discovering' client-side edef (and there is no server-side edef and no discovery needed) you need to either create edef manually OR create instances of EndpointDescriptions at runtime and import them via the RemoteServiceAdmin service instance via its importService method

https://git.eclipse.org/c/ecf/org.eclipse.ecf.git/tree/osgi/bundles/org.eclipse.osgi.services.remoteserviceadmin/src/org/osgi/service/remoteserviceadmin/RemoteServiceAdmin.java#n101

ECF has a couple of utility classes in it's implementation of the RemoteServiceAdmin that are helpful in reading, writing, and constructing EndpointDescription instances...i.e.:

in this package: https://git.eclipse.org/c/ecf/org.eclipse.ecf.git/tree/osgi/bundles/org.eclipse.ecf.osgi.services.remoteserviceadmin/src/org/eclipse/ecf/osgi/services/remoteserviceadmin

EndpointDescriptionReader EndpointDescriptionWriter EndpointDescription

This is all for your context...as I understand that all you want is a simpler way to create your client-side edef that your system can discover.

Particularly since others have described similar use cases (client-side only edef creation and use for service import), I do think it might make sense to have some sort of new ECF-specific service that (e.g.) 1) takes a set of default properties (as map), 2) Takes another properties map that specifies additional and/or overriding values. These two maps could then be merged, and it could return an ECF EndpointDescription...that can then be used either to pass to RemoteServiceAdmin.importService directly...or can be written to a stream/file via EndpointDescriptionWriter. e.g.:

public interface EndpointDescriptionTemplater {

   EndpointDescription createUsingTemplate(Map<String,?> templateProperties, Map<String, ?> overridingProperties);

}

The idea would be that your service could use such a service with a fixed-constant map for templateProperties (for rest remote services); and just read and provide props for overridingProps.

To me, this does appear well-suited to config admin, but I don't think that RCP apps have an impl of config admin present by default, do they?

I would prefer introducing a new service instead of extension point as then it would be usable in all OSGi environments (karaf, etc).

Another idea would be to use the Eclipse variable filtering/text substitution...i.e. have a text file with variable references (i.e. a template edef with variables), have eclipse do the variable substitution using org.eclipse.core.variables. If an RCP app, org.eclipse.core.variables may very well be in your target platform already.

LMK what you think. Let me know your thoughts.

modular-mind commented 3 years ago

Thanks for the detailed response. You're right that the base RCP target does not have a config admin implementation, it also does not include o.e.core.variables.

I agree about choosing a service over an extension. Perhaps we could create something at the JAX-RS level and see how it works. Then elevate it to broader ECF usage if it makes sense? I'd be happy to work on this. I'm guessing it will be hard to do this without config admin, though. If you have any ideas, please let me know.

scottslewis commented 3 years ago

Hi Patrick.

I had the following idea: In the org.eclipse.ecf.osgi.services.remoteserviceadmin bundle (at Eclipse repo), there is this class:

org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionReader

This currently has the one api method: org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionReader.readEndpointDescriptions(InputStream)

I was thinking of adding the following method:

org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionReader.readAndMergeEndpointDescriptions(InputStream, Map<String, Object>)

which would merge the given Map (property name values) ..i.e. the second argument...into the resulting EndpointDescriptions. So for example:

map.put("ecf.endpoint.id","http://blah.blah"); eds = new EndpointDescriptionReader().readAndMergeEndpointDescriptions(inputstream,map);

would read the edef from input stream, replace the file-given ecf.endpoint.id with the value from map (http://blah.blah) and return it in the eds array.

this way, any edef could be read to start with, and whatever values were present in map would be merged to produce the endpoint description.

ECF is coming up on a release 3.14.17 very soon, so this could be made available pretty easily as it's easy to implement. What do you think?

modular-mind commented 3 years ago

That sounds like a great idea. I'm not sure how the new method would be accessed though. I see that the current default EndpointDescriptionReader is called from the bundle listener in the EndpointDescriptionLocator. Would this need to be modified or would we provide the map of input properties in some other way?

scottslewis commented 3 years ago

Hi Patrick. You could do one of two things:

1) You could write your own EndpointDescriptionReader subclass...that overrides the method: readEndpointDescriptions(InputStream)...figures out which Map of property subs to make...and then turns around and calls: readAndMergeEndpointDescriptions(InputStream,Map) with the appropriate map. Then register your EndpointDescriptionReader subclass with interface: org.eclipse.ecf.osgi.services.remoteserviceadmin.IEndpointDescriptionReader to override the default one. Obviously this has to be done on startup/before your service is imported so that the EndpointServiceLocator uses your IEndpointDescriptionReader.

2) You can control the remote service import yourself...in code rather than the EndpointDescriptionLocator. Basically you need the ECF impl of this osgi standard service singleton: org.osgi.service.remoteserviceadmin.RemoteServiceAdmin. Then at appropriate time you can a) edr = new EndpointDescriptionLocator() and call edr.readAndMergeEndpointDescriptions(InputStream yourFile, Map yourprops/values) and then to actually import just call: rsa.importService(edsreturned from readAndMerge);

1 uses the ability to replace the IEndpointDescriptionReader with your own. 2 replaces the implicit import with explicit readAndMergeEndpointDescriptions call followed by rsa.importEndpoint(EndpointDescription ed);

modular-mind commented 3 years ago

So for this pull request the JaxRSClientEndpointDescriptionReader and JerseyClientEndpointDescriptionReader classes would remain, but we would be using the superclass to do the actual merging of properties. Would that work?

That would leave the variable substitution in the EDEF as a Jersey-specific implementation, but we could look at adding this type of logic to ECF Remote Admin in the future if you like.

For now, it would be great to have this method added in the next ECF release. I'm guessing that as it's a simple change it would make more sense for you to get it done, but I'm happy to help if you'd like.

scottslewis commented 3 years ago

So for this pull request the JaxRSClientEndpointDescriptionReader and >JerseyClientEndpointDescriptionReader classes would remain, but we would be using the > superclass to do the actual merging of properties. Would that work?

Yes, I think so. When your readEndpointDescriptions is called you would construct the appropriate Map of override props (in code, from properties file, from system props, or whatever) and call

return readAndMergeEndpointDescriptions(input,overrideprops);

More can be built upon this...e.g. some mechanism could be introduced later to have the EndpointDescriptionLocator pass in some properties (if provided in properties file of same name as edef for example) so I think having this in place right away will be helpful). I have other consumers that are using 2 from above and so I would like to help them too.

I've almost got impl done/and tested. Here's the issue at EF:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=568265

modular-mind commented 3 years ago

Perfect! Thanks for making the change.

I assume you'll be creating a new target after the 3.14.17 release. When the Jersey client is building against that I'll update the pull request to call the new method.

scottslewis commented 3 years ago

Hi Patrick.

I decided to add another kind of support.

Specifically, I modified the the EndpointDescriptionLocator to look for a properties file...next to the edef read from the bundle. For example, if the following file is in a bundle:

myedef.xml

then this properties file

myedef.properties

will be read and any/all properties present passed to the readEndpointDescriptions(inputstream, Map) method...i.e. this method:

org.eclipse.ecf.osgi.services.remoteserviceadmin.IEndpointDescriptionReader.readEndpointDescriptions(InputStream, Map<String, Object>)

Here's the implementation of the API:

org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionReader.readEndpointDescriptions(InputStream, Map<String, Object>)

If no properties file is present next to the edef then things happen as usual.

I think this will make your use case easier to handle. An optional properties file doesn't handle all use cases, but it should make this mechanism easier to use...and maybe eliminate the need for you to implement your own EndpointDescriptionReader.

I've got a snapshot build with this included available here:

https://download.eclipse.org/rt/ecf/snapshot/

modular-mind commented 3 years ago

Thanks for the snapshot build and the properties file mechanism, I'll take a run at modifying my implementation. I'm not sure how the properties file will change my use case though. I'm using the EndpointDescriptionReader to:

  1. Provide a set of defaults that apply to all Jersey REST service clients. I'm guessing with the properties file I would still need to supply these properties for each service independently. Do you think it's ok to supply the defaults across all clients but allow overrides in the EDEF?

  2. Allow for variable substitution based on a system argument passed at startup (currently suggested as "org.eclipse.ecf.provider.jaxrs.client.profile"). Currently this is applied to properties in the EDEF like this:

<property name="%profile-name.ecf.endpoint.id" value-type="String" value="https://api.spacexdata.com/v3" />

Can you let me know how you see the properties file fitting into these use cases? I just want to make sure I'm not missing something.

scottslewis commented 3 years ago

Hi Patrick.

I built and release 3.14.17 last night here:

https://download.eclipse.org/rt/ecf/3.14.17/site.p2

At the last minute I decided to add the following to 3.14.17:

In EndpointDescriptionLocator.handleEndpointDescriptionFile, when an edef file is specified with Remote-Service header in manifest, the edef loading code checks to see if a file by the name of form: .properties exists alongside the edef file. This is done in findOverrideProperties here.

If the properties file does exist, it is loaded by the Properties.load(InputStream) method (assumes properties file format as specified in Properties class), and the resulting props are then merged inside the new impl of this method:

EndpointDescriptionReader.readEndpointDescriptions in the locator code here.

I'm hoping this may address your use case somewhat (i.e. use of .properties file if exists alongside .xml edef). If not I believe your extension of EndpointDescriptionReader should still work. Or if you wish you can override EndpointDescriptionLocator as I added a few public and protected methods to facillitate customization.

modular-mind commented 3 years ago

I've started working with the new code. It looks like there's a small bug in EndpointDescriptionLocator.getPropsURLFromEDFileURL(). There's no path separator right before the properties file is appended to the parent path, so in my case I get:

bundleentry://10.fwk2141775504/OSGI-INFLaunchService_EDEF.properties

I've made a fix locally so I can keep working on the Jersey Client modifications.

modular-mind commented 3 years ago

In looking at the code, it seems like now we have an easy way to support multiple profiles at the property file level. Looking at Spring Boot as an example, they have a pattern of application-{profile}.properties and then set the profile with a system argument - "spring.profiles.active=myprofile". If no profile is set then application.properties is read.

So I'm wondering if we could support a base LaunchService_EDEF.properties and then have LaunchService_EDEF-myprofile.properties. It would be relatively easy to add this to the getPropsURLFromEDFileURL method. Perhaps use something like "org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionLocator.localPropertiesProfile" as the system argument.

modular-mind commented 3 years ago

You can see my changes to the method below. We could also modify it fall back on the default properties file if the profile-specific file isn't available, or maybe this should fail silently (as I have it now).

Also, as I add defaults and environment-specific values to the .properties files the EDEF XML is getting pretty sparse and could actually be completely empty in some cases. It's making me think that maybe a properties file could in some cases be a complete alternative to the XML. Perhaps something to handle in the future.

    protected URL getPropsURLFromEDFileURL(URL edFileURL) {
        String localPropertiesProfile = System.getProperty("org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionLocator.localPropertiesProfile");
        String profileSuffix = localPropertiesProfile != null ? "-" + localPropertiesProfile : "";

        String edFile = edFileURL.getFile();
        int slashIndex = edFile.lastIndexOf('/');
        String parentPath = ""; //$NON-NLS-1$
        if (slashIndex > -1) {
            parentPath = edFile.substring(0, slashIndex) + "/";
            edFile = edFile.substring(slashIndex + 1);
        }
        int dotIndex = edFile.lastIndexOf('.');
        if (dotIndex > 0) {
            edFile = edFile.substring(0, dotIndex);
        }
        // Now combine to create new path name/filename.ext
        try {
            return new URL(edFileURL.getProtocol(), edFileURL.getHost(), edFileURL.getPort(),
                    parentPath + edFile + profileSuffix + "." + "properties"); //$NON-NLS-1$//$NON-NLS-2$
        } catch (MalformedURLException e) {
            return null;
        }
    }
scottslewis commented 3 years ago

Sure Patrick...I would accept this generalization (i.e. the org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionLocator.localPropertiesProfile property and the properties file name) and of course will fix the '/' handling also.

Since this RSA code is hosted at EF, I would appreciate it if you would open an issue via:

https://bugs.eclipse.org/bugs/enter_bug.cgi?product=ECF

with component=ecf.remoteserviceadmin and attaching the gerrit change (or as patch...either way) and I will add to next release 3.14.18. Thanks very much.

modular-mind commented 3 years ago

Done. Thanks for your help :-)

https://bugs.eclipse.org/bugs/show_bug.cgi?id=568413

scottslewis commented 3 years ago

Thanks Patrick for your use case, ideas, fixes, and generalizations. I've applied your changes and done a snapshot build. You can access the snapshot here:

http://download.eclipse.org/rt/ecf/snapshot

I'll eventually need to document this, so that other folks can use it. If you are able to publish a blog or something showing usage for your use case please let me know and we can include that.

I'm going to resolve this issue. If necessary please reopen, or open a new one as appropriate.

scottslewis commented 3 years ago

Closing

modular-mind commented 3 years ago

Sounds good. Once 3.14.18 is released I'm going to update my SpaceX client with multiple properties files and then write a blog post about it. Feel free to copy/paste anything from the blog into the ECF documentation if you wish.

The second piece is specifying the JAX-RS and Jersey defaults so they don't have to be listed in the EDEF. I'll create a pull request soon on that to get your feedback. It turns out that setting the defaults is now the only reason we would need to have the JAX-RS and Jersey specific endpoint readers.

scottslewis commented 3 years ago

Hi Patrick.

I've built and released 3.14.18 here: https://download.eclipse.org/rt/ecf/3.14.18

modular-mind commented 3 years ago

Thanks! Just published the blog post:

https://modumind.com/2020/11/05/ecf-and-rest-improved-support-for-properties-and-profiles/

If you'd like me to adapt any of this for the ECF docs, I'd be happy to do that.