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

Provide JAX-RS and Jersey defaults so they don't have to be duplicated in EDEF files #31

Closed modular-mind closed 3 years ago

modular-mind commented 3 years ago

Many of the EDEF properties will be the same for most JAX-RS and Jersey clients. It would be nice to have a way to specify defaults at both the JAX-RS and Jersey levels and then allow developers to override these if they wish. Currently, I'm doing the following for defaults:

JAX-RS

ecf.endpoint.id.ns = ecf.namespace.jaxrs remote.intents.supported = { "passByValue", "exactlyOnce", "ordered", "jaxrs" } service.intents = { "osgi.async" } endpoint.id = randomly generated UUID ecf.rsvc.id = 0 ecf.endpoint.rsfilter = (objectClass=*)

Jersey

service.imported.configs = { "ecf.jaxrs.jersey.server" }

This was the smallest set of defaults that I could come up with that would create a valid Jersey endpoint description (no validation error). The only other properties that were needed (and specified in the EDEF) are:

ecf.endpoint.id = http://myurl objectClass = { "com.myorg.MyServiceClient" }

There may be better defaults, and this is just to start a discussion.

I do have a set of EndpointDescriptionReaders that are specific to JAX-RS and Jersey. These readers set the defaults and then call the superclass. I'll submit them as a PR, but there may obviously be better ways to handle this.

scottslewis commented 3 years ago

Hi Patrick. I'm finally getting around to dealing with this. Sorry about the delay.

My thoughts: perhaps I should build the notion of 'default properties' into the ECF RSA impl. I was thinking the following:

If there is a file named default.properties in the same directory as the file specified in the Remote-Service: dir/edeffile.xml it is read, and the name value pairs specified there apply to any/all edefs defined in the same directory.

To do this the properties files will have to support arrays of strings, randomly generated UUID, integer sequences, as well as String and Integer, Long and perhaps other types. I'm prepared to add support for these type to both default.properties as well as any .properties.

At EDEF file load time, the EndpointDescriptionLocator would:

1) Look for default.properties in same dir as EDEF file. If found read and parse (i.e. with support for arrays and other types) 2) Combine name/value pairs from default.properties with any in edeffile xml. 3) If edeffile.properties exists, read it (with same support for array types, etc) and apply to create the EndpointDescription instance.

This way the minimal xml file would be empty (but file would at least exist), default.properties would hold the defaults for all files in given directory, and then if edeffile.properties existed it would be applied as it is now.

This would then work the same for any/all providers (including JaxRS), and should make it much easier for anyone creating client-side edef (although admittedly this is most likely with jaxrs provider).

wdyt?

scottslewis commented 3 years ago

I've decided to give this a try with the default.properties file and have an initial issue/enhancement here:

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

modular-mind commented 3 years ago

This sounds great. The only thing I'd add is that it would be great to have a default for the JAX-RS provider itself. I'm guessing that developers using this in a REST/micro-service world would have many EDEF files in separate plug-in projects, so the defaults would have to be duplicated across them.

Is there any way to modify ECF have a defaults file by provider type and embed that in the provider itself? At least for JAX-RS providers...

scottslewis commented 3 years ago

I've taken a first stab at implementation of this. Here's an example 'file discovery' project (with default.properties and .properties

https://git.eclipse.org/c/ecf/org.eclipse.ecf.git/tree/examples/bundles/com.mycorp.examples.timeservice.consumer.filediscovery

These properties and xml files are in the edef/ subdirectory:

https://git.eclipse.org/c/ecf/org.eclipse.ecf.git/tree/examples/bundles/com.mycorp.examples.timeservice.consumer.filediscovery/edef

The default.properties are read first, then the timeserviceendpointdescription.properties override/add to them, then the xml file is read/processes (note that the example has no properties, just the edef top-level elements), and any properties in the edef are merged/overridden by the merged properties.

You can turn on tracing for the org.eclipse.ecf.osgi.service.remoteserviceadmin bundle (in Eclipse debug) and then the ENDPOINT_DESCRIPTION_LOCATOR and the ENDPOINT_DESCRIPTION_READER to get spam output about what's happening with the properties loading.

Note that there is syntax for specifying array, list, set types as well as all the simple types.

Note also that for long/Long, one can also specify either "nanoType", "milliTime" to get the Long nanoTime or currentTimeMillis values.

The uuid type will create a random uuid.

I will think further about your suggestion wrt the provider-specific default.properties. A difficult thing about that, however, is that many of the properties (e.g. service.imported.intents) are needed to find/select the appropriate provider...so these have to be specified just so the correct provider can be determined. The relation between the selected provider(s) and the (e.g.) intents is determined by spec...so the impl has to honor that selection process. I'll give it some more thought.

One thought I has is that perhaps subdirectories be used for the edef files...and the parent directories could be processed for default.properties...that could simplify things for edef creators.

Here's a build with the current snapshot:

https://download.eclipse.org/rt/ecf/snapshot/site.p2/3.14.19.v20201225-0109/

scottslewis commented 3 years ago

I've been making some further additions, and they are now checked in, built and partially . The latest snapshot is here

https://download.eclipse.org/rt/ecf/snapshot/site.p2/3.14.19.v20201227-0740/

What I've added:

1) Rather than only reading any default.properties (in the directory that contains the endpoint description xml file), the EndpointDescriptionLocator will read any default.properties file in all parent directories up to the root. So, for example, see this project:

https://git.eclipse.org/c/ecf/org.eclipse.ecf.git/tree/examples/bundles/com.mycorp.examples.timeservice.consumer.filediscovery

There is both a default.properties at the root directory, and a default properties in the edef sub-directory (where the timeserviceendpointdescription.xml file is). Now, both/any default.properties files will be read if present. They are read in top-down order, so that the /default.properties is read first, then the /edef/default.properties will override any value with same key. Then the timeserviceendpointdescription.properties are read+merged and then finally the properties in the xml file itself. All of these together make up properties to create the EndpointDescription.

This is shown in the timeservice example, that I've been using for debugging and testing. The /default.properties are read first, then the /edef/default.properties, then the /edef/timeserviceendpointdescription.properties then /edef/timeserviceendpointdescription.xml. All are merged together to create the endpoint description that is imported.

I'm hoping this is will work better than having defaults associated with the provider (in this case jersey jaxrs provider). The reason is that with ECF the properties are actually used at import time to decide which consumer provider to use, and so that would complicate things tremendously. Also, I'm not sure how authors of these endpoint descriptions (and properties files) would be able to properly predict/debug what would happen at runtime if there was some sort of defaults associated with providers...some of which may be completely irrelevant to the author's concerns.

In any event, please let me know what you think of this.

modular-mind commented 3 years ago

I had a chance to work with the new functionality and it looks great :-) A few things:

  1. I think default.properties is too general a name, especially if it can go into a bundle root folder. Perhaps something more specific such as edef-default.properties?

  2. Everything worked for me except for specifying

ecf.endpoint.rsfilter=(objectClass=*)

It works if I include this in the XML, but not in the properties file.

  1. I just want to make sure I understand the hierarchy of properties. Based on your message, this is the order of precedences, with each subsequent file overriding properties in previous files:

root default.properties /edef/default.properties (or wherever the EDEF XML is stored) /edef/MyService_EDEF.properties /edef/MyService_EDEF.xml

My understanding was that MyService_EDEF.properties would override MyService_EDEF.xml. Is that different in the new implementation?

Also, it would be great if profile-based properties could be integrated into the tree. Currently if I specify a profile of "dev" then the file MyService_EDEF.properties would be replaced with MyService_EDEF-dev.properties. But a developer might want to put most of the properties in the base file and then have a few that vary by profile. So the order would be:

root default.properties /edef/default.properties (or wherever the EDEF XML is stored) /edef/MyService_EDEF.properties ---> /edef/MyService_EDEF-dev.properties /edef/MyService_EDEF.xml

  1. Finally, it looks like this is all headed towards eliminating the EDEF xml files entirely, at least for use cases such as this. Eventually it might make sense to have a endpoint description locator that works specifically with properties files and a manifest entry such as:

Remote-Service: OSGI-INF/MyService_EDEF.properties

Jus throwing this out there as a longer term thought...

scottslewis commented 3 years ago

I had a chance to work with the new functionality and it looks great :-) A few things:

1. I think default.properties is too general a name, especially if it can go into a bundle root folder. Perhaps something more specific such as edef-default.properties?

Ok.

2. Everything worked for me except for specifying

ecf.endpoint.rsfilter=(objectClass=*)

Ok. Might be something about the Properties.load parser. I'll check it out.

It works if I include this in the XML, but not in the properties file.

1. I just want to make sure I understand the hierarchy of properties. Based on your message, this is the order of precedences, with each subsequent file overriding properties in previous files:

root default.properties /edef/default.properties (or wherever the EDEF XML is stored) /edef/MyService_EDEF.properties /edef/MyService_EDEF.xml

My understanding was that MyService_EDEF.properties would override MyService_EDEF.xml. Is that different in the new implementation?

No. Starting with root, each subsequent .properties file overrides the previous .properties. But all of these properties combined overrides the MyService_EDEF.xml.

Also, it would be great if profile-based properties could be integrated into the tree. Currently if I specify a profile of "dev" then the file MyService_EDEF.properties would be replaced with MyService_EDEF-dev.properties. But a developer might want to put most of the properties in the base file and then have a few that vary by profile. So the order would be:

root default.properties /edef/default.properties (or wherever the EDEF XML is stored) /edef/MyService_EDEF.properties ---> /edef/MyService_EDEF-dev.properties /edef/MyService_EDEF.xml

Ok, I'll do this.

1. Finally, it looks like this is all headed towards eliminating the EDEF xml files entirely, at least for use cases such as this. Eventually it might make sense to have a endpoint description locator that works specifically with properties files and a manifest entry such as:

Remote-Service: OSGI-INF/MyService_EDEF.properties

Jus throwing this out there as a longer term thought...

I think I can do this...will need to look at the spec to make sure.

In a day or two I'll try to have another version that implements the additions/changes suggested.

scottslewis commented 3 years ago

Ok I've made the following changes/additions:

  1. I think default.properties is too general a name, especially if it can go into a bundle root folder. Perhaps something more specific such as edef-default.properties?

I've made it edef_defaults.properties

  1. Everything worked for me except for specifying ecf.endpoint.rsfilter=(objectClass=*)

This should now work. There was a problem with parsing values that had '=' in them...this is now fixed.

Also, it would be great if profile-based properties could be integrated into the tree.

I've done this for both the edef_defaults.properties and the named properties file (e.g. timeserviceendpointdescription.properties). Now, if you specify a profile of 'dev' it should look for/use:

/edef_defaults-dev.properties /edef/edef_defaults-dev.properties and /edef/timeserviceendpointdescription-dev.properties

  1. Finally, it looks like this is all headed towards eliminating the EDEF xml files entirely,

I've allowed/supported the following:

Remote-Service: edef/timeserviceendpointdescription.properties rather than: Remote-Service: edef/timeserviceendpointdescription.xml

Note that all of the required properties must be present in either the edef_defaults.properties and/or the .properties once all merged together.

I've also put a faire amount of tracing in place... so you should be able to see what's happening if you turn on the EndpointDescriptionLocator debug option.

The build including these changes is here:

https://download.eclipse.org/rt/ecf/snapshot/site.p2/3.14.19.v20201231-0313/

modular-mind commented 3 years ago

I had time to work with the new functionality this morning. It looks like everything is working great except for the base defaults/profile defaults combination.

If I specify a profile as a system argument, the description locator looks for properties files with the profile extension but does not read the base properties file. This is true both for the edef_defaults and for the service specific properties files.

With "test" set as profile:

edef_defaults.properties --> not read edef_defaults-test.properties --> read

MyService_EDEF.properties --> not read MyService_EDEF-test.properties --> read

scottslewis commented 3 years ago

I had time to work with the new functionality this morning. It looks like everything is working great except for the base defaults/profile defaults combination.

If I specify a profile as a system argument, the description locator looks for properties files with the profile extension but does not read the base properties file. This is true both for the edef_defaults and for the service specific properties files.

With "test" set as profile:

edef_defaults.properties --> not read edef_defaults-test.properties --> read

MyService_EDEF.properties --> not read MyService_EDEF-test.properties --> read

Hi Patrick.

Ok...I thought this was the desired behavior wrt profiles....but just to confirm...you would rather have it read both the edef_defaults.properties AND the edef_defaults-.properties? (presumably in that order). Is that right?

FYI: I'm planning on releasing ECF 3.14.19 on Monday, Jan 5 so I'm hoping to get this wrapped up in the next few days.

Thanks. Scott

modular-mind commented 3 years ago

Yes, I was thinking that both the base properties and the profile properties would be read. That way some properties (e.g.. objectClass) could be specified across all profiles and then profile-specific properties (e.g. endpoint URI) could be specified individually.

And yes, the profile properties would override the base properties.

Thanks for working on this. I'd be happy to write up some documentation after the release if you like. I'll also write up a blog post.

Happy new years :-)

scottslewis commented 3 years ago

Yes, I was thinking that both the base properties and the profile properties would be read. That way some properties (e.g.. objectClass) could be specified across all profiles and then profile-specific properties (e.g. endpoint URI) could be specified individually.

And yes, the profile properties would override the base properties.

Thanks for working on this. I'd be happy to write up some documentation after the release if you like. I'll also write up a blog post.

Happy new years :-)

Happy New Years to you. We are all hoping for a better 2021.

I've made the changes and tested the profile handling. Seems to work as desired now. Latest build available here:

https://download.eclipse.org/rt/ecf/snapshot/site.p2/3.14.19.v20201231-2336/

I would appreciate some contributed documentation. If you like, feel free to use the contents of this project:

https://git.eclipse.org/c/ecf/org.eclipse.ecf.git/tree/examples/bundles/com.mycorp.examples.timeservice.consumer.filediscovery

as I've been using it for testing and debugging. Or...modify/enhance this project and I'll add your enhancements.

For reference, the wiki page is here: https://wiki.eclipse.org/Using_Properties_to_Import_Endpoint_Descriptions

Feel free to edit anything as appropriate for all the new stuff.

scottslewis commented 3 years ago

Hi Patrick.

I've released version 3.14.19 of ECF here:

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

It's not on the download page yet because of some problem with my committer access to the web page repo, but I'm confident it will be fixed soon and I'll release 'officially'.

LMK about your thoughts wrt documentation.

modular-mind commented 3 years ago

I think the best thing would be re-structure the existing Properties wiki page to focus on Properties-only EDEFs, while also showing how they can extend an XML EDEF. It would probably mean re-writing most of the content. If you have any issues with that, let me know. Otherwise I'll take a crack at it tomorrow.

modular-mind commented 3 years ago

I just finished up modifying the documentation. You may want to go over it and correct any errors/assumptions. One thing I wasn't sure about was how to generate a random value such as a UUID. Do you specify:

my.property.name:uuid=

That's the way I have it written up now, but I see you also have properties specified as uuid=0. Would that generate a random UUID as well? Or just use 0?

In any case, thanks for all your help with this. I'm planning on blogging about it in the next day or two.

scottslewis commented 3 years ago

Hi Patrick. Thanks I will take a look (been a little busy with personal and job stuff).

wrt

prop.name:uuid=0 or otther...in either case it will generate the uuid and set the prop.name to ithe generated value t in the properties. The main reason I put a 0 there was that it requires some value other than null, and the Properties.load code doesn't read it if it has a return right after the equals. So there has to be something there...but in any case whether it's an empty space(s) or number or whatever it will still replace any value loaded by Properties.load with a randomly generated uuid.

Thanksinadvance for the docs work on this...I'll also be blogging about it as the major new feature in 3.14.19, and linking to docs, examples, etc as well.

Scott

modular-mind commented 3 years ago

Hi Scott,

I just posted an article about the default features.

https://modumind.com/2021/01/07/eclipse-rcp-and-rest-default-properties-make-configuration-even-easier/

I'm closing this issue and again, thank you for all the work.

BTW, the only remaining request I have related to this project is that it be integrated into the main Eclipse p2 repository as a regular ECF Remote Services provider. What work do you think needs to be done for that to happen? I'd be happy to help in any way I can.

Patrick

scottslewis commented 3 years ago

Hi Patrick.

WRT the dev work: you're welcome. Apologies it took so long.

Thanks also for the documentation work. I will also blog, and point to your blogs.

WRT getting this distribution provider deployed as part of the EF repo: The toughest part of that is the dependency on jersey...and it's dependencies. AFAIK, right now Jersey doesn't actually build a p2 repo for it and it's dependencies...and rather only deploys on maven central.

Plus, Jersey has released version 3, which has the javax. -> jakarta. namespace change for Jax-RS...which complicates the current situation for the EC distribution provider further. FWIW: I've got a start on the Jersey/Jakarta changes started on branch named jersey_3_0_0 branch. I'll be looking to you and other consumers to inform and timing of that transition, since it breaks compatibility at API level.

In any event...I think the first thing to focus on is to lobby the Jersey project to produce a p2 repo...along with Orbit for dependencies (I guess). If they did that then it would be very easy for me to contribute the github ECF distribution provider to the EF ECF project. I've proposed that they/Jersey produce a p2 repo a few years ago, but at that time they weren't willing to do so. Perhaps if you or others were to raise with them again on the EF jersey-dev mailing list it would help: https://accounts.eclipse.org/mailing-list/jersey-dev

modular-mind commented 3 years ago

Hi Scott,

I could definitely bring this up with the Jersey project, but I was wondering if you've seen the latest m2e PDE functionality that lets us add Maven artifacts directly to a target definition. I just blogged about this the other day:

https://modumind.com/2021/01/11/including-maven-artifacts-in-an-eclipse-rcp-target-platform/

Would this be a way around having to bring the Jersey JARs into Orbit or requiring them to produce a p2 repo?

scottslewis commented 3 years ago

Hi Scott,

I could definitely bring this up with the Jersey project, but I was wondering if you've seen the latest m2e PDE functionality that lets us add Maven artifacts directly to a target definition. I just blogged about this the other day:

https://modumind.com/2021/01/11/including-maven-artifacts-in-an-eclipse-rcp-target-platform/

Would this be a way around having to bring the Jersey JARs into Orbit or requiring them to produce a p2 repo?

Yes. I'm not sure yet how difficult...as I haven't tried the maven artifacts in target platform. I particularly have some questions about how dependencies will be handled. For example, jersey has dependencies on Jackson, jax-rs and a number of other libraries, which of course have their own dependencies, in some cases, etc. That may all just happen correctly with maven artifacts and versioning in target platform...or it may be necessary to construct all the repos/versions in a specific target. In any case, if you wish please open an issue for a maven artifacts for Jersey target platform and we'll give it a try.