Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[osgi] Solutions for https://github.com/eclipse/microprofile/issues/33 #381

Merged
merged 2 commits into from
Jun 2, 2020
Merged

[osgi] Solutions for https://github.com/eclipse/microprofile/issues/33 #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fbf273
apply service loader mediator (via bnd annotations), osgi.cdi (via os…
rotty3000 Mar 28, 2020
9941557
eliminate transfer progress logging to simplify reviewing CI build logs
rotty3000 Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,4 @@ jobs:
java-version: ${{matrix.java}}

- name: build with maven
run: mvn verify
run: mvn -ntp verify
26 changes: 22 additions & 4 deletions api/bnd.bnd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,30 @@
-exportcontents: \
org.eclipse.microprofile.*

Import-Package: \
javax.enterprise.util;version="[1.1,3)", \
*

Bundle-SymbolicName: org.eclipse.microprofile.config

# For reproducible builds
-noextraheaders: true
-snapshot: SNAPSHOT

# Apply OSGi Portable Java Contracts [1] with "active" effectiveness so that
# they are not strict runtime requirements. This removes the need to import
# packages with versions.
-contract: *;effective:=active

# Simulate the JavaCDI contract since the compile dependencies used do not
# provide them.
-define-contract: \
osgi.contract;\
osgi.contract=JavaCDI;\
version:List<Version>='2.0,1.1';\
uses:='\
javax.decorator,\
javax.enterprise.context,\
javax.enterprise.context.spi,\
javax.enterprise.event,\
javax.enterprise.inject,\
javax.enterprise.inject.spi,\
javax.enterprise.util'

# [1] https://www.osgi.org/portable-java-contract-definitions/
15 changes: 9 additions & 6 deletions api/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,28 @@
<version>2.0-SNAPSHOT</version>
</parent>

<properties>
<bnd.version>5.0.0</bnd.version>
</properties>
<groupId>org.eclipse.microprofile.config</groupId>
<artifactId>microprofile-config-api</artifactId>
<name>MicroProfile Config API</name>
<description>MicroProfile Config :: API</description>
<url>https://microprofile.io/project/eclipse/microprofile-config</url>
<dependencies>
<dependency>
<groupId>biz.aQute.bnd</groupId>
<artifactId>biz.aQute.bnd.annotation</artifactId>
</dependency>
<dependency>
<groupId>jakarta.enterprise</groupId>
<artifactId>jakarta.enterprise.cdi-api</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.annotation.versioning</artifactId>
<version>1.1.0</version>
<scope>provided</scope>
<artifactId>org.osgi.service.cdi</artifactId>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>osgi.annotation</artifactId>
</dependency>
</dependencies>

Expand Down
13 changes: 13 additions & 0 deletions api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,17 @@
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.ElementType.TYPE;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import static org.osgi.annotation.bundle.Requirement.Resolution.OPTIONAL;
import static org.osgi.service.cdi.CDIConstants.CDI_EXTENSION_PROPERTY;

import java.lang.annotation.Retention;
import java.lang.annotation.Target;

import javax.enterprise.util.Nonbinding;
import javax.inject.Qualifier;

import org.osgi.annotation.bundle.Requirement;

/**
* <p>
* Binds the injection point with a configured value.
Expand Down Expand Up @@ -100,6 +104,15 @@
@Qualifier
@Retention(RUNTIME)
@Target({METHOD, FIELD, PARAMETER, TYPE})
/*
* Two @Requirement annotations are defined so that the result is a _weak requirement_.
* One requirement is resolution:=optional which means that at runtime, if satisfied,
* it will be wired, otherwise it is simply ignored. Another requirement is
* effective:=active which means it is not visible at runtime, but applicable during
* assembly where an effectviness of _active_ is specified.
*/
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", effective = "active")
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", resolution = OPTIONAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very fishy. Why do you have to add Requirement twice? Why cannot it cope with both effective and resolution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rotty3000 What is the goal here in attempting to generate 2 requirements in bundles using the ConfigProperty annotation? I am guessing your goals is to have a non-optional requirements at active time so provisioning resolutions will include a CDI extender bundle and to have an optional requirement at runtime so the bundle will resolve even when there is no bundle providing the CDI extender capability. Sort of a bridge to the future while still working with the past?

If so, we need to have some comments here explaining why we are doing this. Since this confused @Emily-Jiang.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will also need to add some words to the Config spec so that CDI extender bundles provide this capability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjhargrave you are exactly correct!

BTW, in times past when I've discussed this with @pkriens and @tjwatson we've dubbed this combination of two requirements, one optional, one effective active as a weak requirement.

A weak requirement is used because IBM clearly does not want to strictly require Service Loader Mediator. This avoids that. Meanwhile, anyone who does want to use the resolver for assembly can simply enable active effectiveness.

About the OSGi CDI extension name, if folks are in agreement we could document that in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A weak requirement is used because IBM clearly does not want to strictly require Service Loader Mediator.

The case here is not about SLM. It is about the fact that there are some CDI extension implementations which do not provide the capability for resolve time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's the exact same reason. If someone doesn't want to use OSGi CDI and manually wire things up it will still work.

Copy link
Contributor

@bjhargrave bjhargrave Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR should put comments in the source code where we apply the weak requirements to document why there are 2 seemingly duplicate requirements and why we use effective:=active for non-runtime resolution scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable! I'll do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original objection for this PR with the requirement for runtime to provide CDI capability. Otherwise the bundle does not resolve. I think with this weak requirement, it is reasonable change. Agreed with @bjhargrave to add comments to explain this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to agree on, and document in the spec, the name of the CDI extension for Config. @rotty3000 proposes org.eclipse.microprofile.config which seems fine to me. But others (spec readers and implementors) will need to know the agreed upon name.

public @interface ConfigProperty {
String UNCONFIGURED_VALUE="org.eclipse.microprofile.config.configproperty.unconfigureddvalue";
/**
Expand Down
4 changes: 2 additions & 2 deletions api/src/main/java/org/eclipse/microprofile/config/inject/package-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@
*
* @author <a href="mailto:[email protected]">Mark Struberg</a>
* @author <a href="mailto:[email protected]">Emily Jiang</a>
*
*
*/
@org.osgi.annotation.versioning.Version("1.0")
@org.osgi.annotation.versioning.Version("1.0.1")
package org.eclipse.microprofile.config.inject;

12 changes: 12 additions & 0 deletions api/src/main/java/org/eclipse/microprofile/config/spi/ConfigProviderResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

import org.eclipse.microprofile.config.Config;

import aQute.bnd.annotation.spi.ServiceConsumer;

/**
* The service provider for implementations of the MicroProfile Configuration specification.
* <p>
Expand All @@ -36,6 +38,16 @@
* @author <a href="mailto:[email protected]">Romain Manni-Bucau</a>
* @author <a href="mailto:[email protected]">Emily Jiang</a>
*/

/*
* The @ServiceConsumer annotation adds support for Service Loader Mediator in
* order to support wiring of Service Loader providers to consumers in OSGi.
* However, the requirements generated are specified as effective:=active to
* prevent this from being a strict requirement. As such the API is usable in
* runtimes without a Service Loader Mediator implementation while allowing for
* such to be enabled when using the resolver during assembly.
*/
@ServiceConsumer(value = ConfigProviderResolver.class, effective = "active")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is also active for similar reasons as mentioned above. We should also document why we do active here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! Since there has been resistance to the SLM solution, this makes the feature usable during assembly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether this gets documented in the spec is up for debate. I think we could come up with standard language would simplify applying this to other MP specs if desired. It would largely be the same words for every spec except for the identity (which I'd suggest simply going with the bsn of the API for consistency and avoid confusion).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work, given that, at the moment, the service loading isn't done using the thread context classloader?

See line 110:

                instance = loadSpi(ConfigProviderResolver.class.getClassLoader());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work, given that, at the moment, the service loading isn't done using the thread context classloader?

Yes. For example, you can use a fragment to attach the META-INF/service resource and add any necessary Import-Package/DynamicImport-Package statement to this bundle.

Apache Aries SPI Fly is another option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap! The SML spec never limited exactly how the loading was done and so within the past two years the Apache Aries SPI Fly implementation was enhanced to support the two argument load(Class, ClassLoader) method. The heuristic is to use the passed ClassLoader as fallback if no providers are found. So no code change here is required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SML

SLM :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a thread context class loader does not help here as trying to use it would introduce timing issues similar to using setInstance. You would have to arrange to set the TCCL properly before being the first one to cause the ServiceLoader to load the impl class.

To be fair, there are also timing issues with SPI Fly in that you have to ensure it is started (perhaps via start levels) before anyone causes the ServiceLoader call to be made.

Using a fragment is probably the safest as it will be attached at resolve time which is by definition before any code in the host bundle can execute.

public abstract class ConfigProviderResolver {
/**
* Construct a new instance.
Expand Down
20 changes: 19 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

<links>https://osgi.org/javadoc/osgi.annotation/7.0.0/</links>

<bnd.version>5.0.1</bnd.version>
<checkstyle.version>2.17</checkstyle.version>
<checkstyle.methodNameFormat>^_?[a-z][a-zA-Z0-9]*$</checkstyle.methodNameFormat>
</properties>
Expand Down Expand Up @@ -102,9 +103,14 @@
<module>tck</module>
<module>spec</module>
</modules>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>biz.aQute.bnd</groupId>
<artifactId>biz.aQute.bnd.annotation</artifactId>
<version>${bnd.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>jakarta.enterprise</groupId>
<artifactId>jakarta.enterprise.cdi-api</artifactId>
Expand All @@ -123,6 +129,18 @@
<scope>import</scope>
<type>pom</type>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.service.cdi</artifactId>
<version>1.0.0</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>osgi.annotation</artifactId>
<version>7.0.0</version>
<scope>provided</scope>
</dependency>
</dependencies>
</dependencyManagement>

Expand Down
20 changes: 16 additions & 4 deletions tck/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,19 @@
<scope>provided</scope>
</dependency>

<dependency>
<groupId>biz.aQute.bnd</groupId>
<artifactId>biz.aQute.bnd.annotation</artifactId>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.service.cdi</artifactId>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>osgi.annotation</artifactId>
</dependency>

<dependency>
<groupId>jakarta.enterprise</groupId>
<artifactId>jakarta.enterprise.cdi-api</artifactId>
Expand All @@ -69,7 +82,6 @@
<optional>true</optional>
</dependency>


<dependency>
<groupId>org.apache.geronimo.specs</groupId>
<artifactId>geronimo-atinject_1.0_spec</artifactId>
Expand All @@ -89,7 +101,7 @@
</exclusion>
</exclusions>
</dependency>

<!-- add correct junit dependency for testng, which doesn't bundle hamcrest -->
<dependency>
<groupId>junit</groupId>
Expand Down Expand Up @@ -122,7 +134,7 @@
<scope>compile</scope>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
Expand Down Expand Up @@ -151,7 +163,7 @@
</plugin>
</plugins>
</build>

<profiles>
<profile>
<id>eclipse-jarsigner</id>
Expand Down