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.

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

ArC: fix in-application build compatible extensions #37397

Merged
merged 1 commit into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions build-parent/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,11 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-arc-test-supplement</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
Expand Down
4 changes: 4 additions & 0 deletions extensions/arc/deployment/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-arc-test-supplement</artifactId>
</dependency>
<!-- Used to test wrong @Singleton detection -->
<dependency>
<groupId>jakarta.ejb</groupId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package io.quarkus.arc.test.cdi.bcextensions;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.List;

import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.context.Dependent;
import jakarta.enterprise.inject.Instance;
import jakarta.enterprise.inject.build.compatible.spi.BuildCompatibleExtension;
import jakarta.enterprise.inject.build.compatible.spi.Parameters;
import jakarta.enterprise.inject.build.compatible.spi.Synthesis;
import jakarta.enterprise.inject.build.compatible.spi.SyntheticBeanCreator;
import jakarta.enterprise.inject.build.compatible.spi.SyntheticBeanDisposer;
import jakarta.enterprise.inject.build.compatible.spi.SyntheticComponents;
import jakarta.inject.Inject;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.arc.test.supplement.SomeClassInExternalLibrary;
import io.quarkus.builder.Version;
import io.quarkus.maven.dependency.Dependency;
import io.quarkus.test.QuarkusUnitTest;

public class SynthBeanForExternalClass {
// the test includes an _application_ that declares a build compatible extension
// (in the Runtime CL), which creates a synthetic bean for a class that is _outside_
// of the application (in the Base Runtime CL)

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(MyBean.class, MyExtension.class, MySyntheticBeanCreator.class)
.addAsServiceProvider(BuildCompatibleExtension.class, MyExtension.class))
// we need a non-application archive, so cannot use `withAdditionalDependency()`
.setForcedDependencies(List.of(Dependency.of("io.quarkus", "quarkus-arc-test-supplement", Version.getVersion())));

@Inject
MyBean bean;

@Test
public void test() {
assertFalse(MySyntheticBeanCreator.created);
assertFalse(MySyntheticBeanDisposer.disposed);

assertEquals("OK", bean.doSomething());
assertTrue(MySyntheticBeanCreator.created);
assertTrue(MySyntheticBeanDisposer.disposed);
}

@ApplicationScoped
public static class MyBean {
@Inject
Instance<SomeClassInExternalLibrary> someClass;

public String doSomething() {
SomeClassInExternalLibrary instance = someClass.get();
instance.toString(); // force instantiating the bean
someClass.destroy(instance); // force destroying the instance
return "OK";
}
}

public static class MyExtension implements BuildCompatibleExtension {
@Synthesis
public void synthesis(SyntheticComponents syn) {
syn.addBean(SomeClassInExternalLibrary.class)
.type(SomeClassInExternalLibrary.class)
.scope(Dependent.class)
.createWith(MySyntheticBeanCreator.class)
.disposeWith(MySyntheticBeanDisposer.class);
}
}

public static class MySyntheticBeanCreator implements SyntheticBeanCreator<SomeClassInExternalLibrary> {
public static boolean created;

public SomeClassInExternalLibrary create(Instance<Object> lookup, Parameters params) {
SomeClassInExternalLibrary result = new SomeClassInExternalLibrary();
created = true;
return result;
}
}

public static class MySyntheticBeanDisposer implements SyntheticBeanDisposer<SomeClassInExternalLibrary> {
public static boolean disposed;

@Override
public void dispose(SomeClassInExternalLibrary instance, Instance<Object> lookup, Parameters params) {
disposed = true;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package io.quarkus.arc.test.cdi.bcextensions;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.List;

import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.event.Event;
import jakarta.enterprise.inject.build.compatible.spi.BuildCompatibleExtension;
import jakarta.enterprise.inject.build.compatible.spi.Parameters;
import jakarta.enterprise.inject.build.compatible.spi.Synthesis;
import jakarta.enterprise.inject.build.compatible.spi.SyntheticComponents;
import jakarta.enterprise.inject.build.compatible.spi.SyntheticObserver;
import jakarta.enterprise.inject.spi.EventContext;
import jakarta.inject.Inject;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.arc.test.supplement.SomeClassInExternalLibrary;
import io.quarkus.builder.Version;
import io.quarkus.maven.dependency.Dependency;
import io.quarkus.test.QuarkusUnitTest;

public class SynthObserverAsIfInExternalClass {
// the test includes an _application_ that declares a build compatible extension
// (in the Runtime CL), which creates a synthetic observer which is "as if" declared
// in a class that is _outside_ of the application (in the Base Runtime CL)

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(MyBean.class, MyExtension.class, MySyntheticObserver.class)
.addAsServiceProvider(BuildCompatibleExtension.class, MyExtension.class))
// we need a non-application archive, so cannot use `withAdditionalDependency()`
.setForcedDependencies(List.of(Dependency.of("io.quarkus", "quarkus-arc-test-supplement", Version.getVersion())));

@Inject
MyBean bean;

@Test
public void test() {
assertFalse(MySyntheticObserver.notified);

assertEquals("OK", bean.doSomething());
assertTrue(MySyntheticObserver.notified);
}

@ApplicationScoped
public static class MyBean {
@Inject
Event<String> event;

public String doSomething() {
event.fire(""); // force notifying the observer
return "OK";
}
}

public static class MyExtension implements BuildCompatibleExtension {
@Synthesis
public void synthesis(SyntheticComponents syn) {
syn.addObserver(String.class)
.declaringClass(SomeClassInExternalLibrary.class)
.observeWith(MySyntheticObserver.class);
}
}

public static class MySyntheticObserver implements SyntheticObserver<String> {
public static boolean notified;

@Override
public void observe(EventContext<String> event, Parameters params) {
notified = true;
}
}
}
1 change: 1 addition & 0 deletions extensions/arc/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<modules>
<module>deployment</module>
<module>runtime</module>
<module>test-supplement</module>
</modules>

</project>
16 changes: 16 additions & 0 deletions extensions/arc/test-supplement/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
<parent>
<artifactId>quarkus-arc-parent</artifactId>
<groupId>io.quarkus</groupId>
<version>999-SNAPSHOT</version>
</parent>
<modelVersion>4.0.0</modelVersion>

<artifactId>quarkus-arc-test-supplement</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I wonder if this artifact will get released and deployed to the maven central as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. I was thinking I could abuse some other dependency of the ArC deployment module, like AssertJ, but that would require keeping the version string in the test in sync with the POM, which felt like a worse deal than an extra module with a single class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could try to skip this module during a release. I've found https://stackoverflow.com/questions/31495316/skip-submodule-during-maven-release where the following config is used in the module that should be skipped:

<plugin>
  <groupId>org.apache.maven.plugins</groupId>
  <artifactId>maven-deploy-plugin</artifactId>
  <configuration>
    <skip>true</skip>
  </configuration>
</plugin>

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 would work, but the ArC deployment module has a [test-scoped] dependency on this artifact, so quarkus-arc-deployment POM might cause errors. Is this tiny artifact a problem? I'd much rather keep it than to face surprises down the line.

It would be best if I could create an in-memory archive using ShrinkWrap and add it as a non-application archive, but QuarkusUnitTest doesn't offer an API for that and I don't really know if QuarkusBootstrap.Builder allows it or not. @aloubyansky would you please know if that would be feasible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this tiny artifact a problem?

It is not. The central repo is probably full of other huge useless artifacts anyway 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

QuarkusUnitTest has a nice API for creating application archives using ShrinkWrap, but for this particular test, I need a non-application archive.

Copy link
Member

Choose a reason for hiding this comment

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

I actually haven't tested that. It may fail during dependency resolution due to a missing pom.xml. What I tested was BootstrapAppModelResolver.resolveModel(WorkspaceModule module).

But if anything, we could probably enhance the API to do what we need. It just haven't come up before, afaict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do something like

ResolvedDependency dep = ResolvedDependencyBuilder.newInstance()
        .setGroupId(groupId)
        .setArtifactId(artifactId)
        .setVersion(version)
        .setResolvedPath(path)
        .setWorkspaceModule(WorkspaceModule.builder()
                .setModuleId(WorkspaceModuleId.of(groupId, artifactId, version))
                .setModuleDir(path)
                .setBuildDir(path)
                .build())
        .build();

(where groupId, artifactId and version are things I made up, and path is the directory where the .class file is), and when I add that dep to forced dependencies, Quarkus tries to download it from Central.

I also tried to add an AdditionalDependency with the forceApplicationArchive flag set to false to the set of additional dependencies, but it doesn't do what I need (probably is still considered an application archive).

Having an extra module in the ArC extension isn't super pretty, but works as I need, so I'll probably leave it be.

Copy link
Member

Choose a reason for hiding this comment

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

It probably fails to resolve the POM artifact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually tries to download both the POM and the JAR:

Downloading from central: https://repo.maven.apache.org/maven2/com/example/test1540098276/1.0.0/test1540098276-1.0.0.pom
Downloading from central: https://repo.maven.apache.org/maven2/com/example/test1540098276/1.0.0/test1540098276-1.0.0.jar

🤷

<name>Quarkus - ArC - Test Supplement</name>
<description>Supplement archive for ArC tests</description>

</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package io.quarkus.arc.test.supplement;

public class SomeClassInExternalLibrary {
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ public class BeanDeployment {
private final IndexView beanArchiveImmutableIndex;
private final IndexView applicationIndex;

private final Predicate<DotName> applicationClassPredicate;

private final Map<DotName, ClassInfo> qualifiers;
private final Map<DotName, ClassInfo> repeatingQualifierAnnotations;
private final Map<DotName, Set<String>> qualifierNonbindingMembers;
Expand Down Expand Up @@ -142,6 +144,7 @@ public class BeanDeployment {
this.beanArchiveComputingIndex = builder.beanArchiveComputingIndex;
this.beanArchiveImmutableIndex = Objects.requireNonNull(builder.beanArchiveImmutableIndex);
this.applicationIndex = builder.applicationIndex;
this.applicationClassPredicate = builder.applicationClassPredicate;
this.annotationStore = new AnnotationStore(initAndSort(builder.annotationTransformers, buildContext), buildContext);
buildContext.putInternal(Key.ANNOTATION_STORE, annotationStore);

Expand Down Expand Up @@ -1384,7 +1387,7 @@ private RegistrationContext registerSyntheticBeans(List<BeanRegistrar> beanRegis
}
if (buildCompatibleExtensions != null) {
buildCompatibleExtensions.runSynthesis(beanArchiveComputingIndex);
buildCompatibleExtensions.registerSyntheticBeans(context);
buildCompatibleExtensions.registerSyntheticBeans(context, applicationClassPredicate);
}
this.injectionPoints.addAll(context.syntheticInjectionPoints);
return context;
Expand All @@ -1399,7 +1402,7 @@ io.quarkus.arc.processor.ObserverRegistrar.RegistrationContext registerSynthetic
context.extension = null;
}
if (buildCompatibleExtensions != null) {
buildCompatibleExtensions.registerSyntheticObservers(context);
buildCompatibleExtensions.registerSyntheticObservers(context, applicationClassPredicate);
buildCompatibleExtensions.runRegistrationAgain(beanArchiveComputingIndex, beans, observers);
}
return context;
Expand Down Expand Up @@ -1433,7 +1436,7 @@ private void addSyntheticObserver(ObserverConfigurator configurator) {
configurator.observedQualifiers,
Reception.ALWAYS, configurator.transactionPhase, configurator.isAsync, configurator.priority,
observerTransformers, buildContext,
jtaCapabilities, configurator.notifyConsumer, configurator.params));
jtaCapabilities, configurator.notifyConsumer, configurator.params, configurator.forceApplicationClass));
}

static void processErrors(List<Throwable> errors) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ Collection<Resource> generate(DecoratorInfo decorator) {
return Collections.emptyList();
}

boolean isApplicationClass = applicationClassPredicate.test(decorator.getBeanClass());
boolean isApplicationClass = applicationClassPredicate.test(decorator.getBeanClass())
|| decorator.isForceApplicationClass();
ResourceClassOutput classOutput = new ResourceClassOutput(isApplicationClass,
name -> name.equals(generatedName) ? SpecialType.DECORATOR_BEAN : null, generateSources);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ private Collection<Resource> generateSyntheticInterceptor(InterceptorInfo interc
return Collections.emptyList();
}

boolean isApplicationClass = applicationClassPredicate.test(creatorClassName);
boolean isApplicationClass = applicationClassPredicate.test(creatorClassName) || interceptor.isForceApplicationClass();
ResourceClassOutput classOutput = new ResourceClassOutput(isApplicationClass,
name -> name.equals(generatedName) ? SpecialType.INTERCEPTOR_BEAN : null, generateSources);

Expand Down Expand Up @@ -168,7 +168,8 @@ private Collection<Resource> generateClassInterceptor(InterceptorInfo intercepto
return Collections.emptyList();
}

boolean isApplicationClass = applicationClassPredicate.test(interceptor.getBeanClass());
boolean isApplicationClass = applicationClassPredicate.test(interceptor.getBeanClass())
|| interceptor.isForceApplicationClass();
ResourceClassOutput classOutput = new ResourceClassOutput(isApplicationClass,
name -> name.equals(generatedName) ? SpecialType.INTERCEPTOR_BEAN : null, generateSources);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public final class ObserverConfigurator extends ConfiguratorBase<ObserverConfigu
boolean isAsync;
TransactionPhase transactionPhase;
Consumer<MethodCreator> notifyConsumer;
boolean forceApplicationClass;

public ObserverConfigurator(Consumer<ObserverConfigurator> consumer) {
this.consumer = consumer;
Expand Down Expand Up @@ -118,6 +119,15 @@ public ObserverConfigurator notify(Consumer<MethodCreator> notifyConsumer) {
return this;
}

/**
* Forces the observer to be considered an 'application class', so it will be defined in the runtime
* ClassLoader and re-created on each redeployment.
*/
public ObserverConfigurator forceApplicationClass() {
this.forceApplicationClass = true;
return this;
}

public void done() {
if (beanClass == null) {
throw new IllegalStateException("Observer bean class must be set!");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ Collection<Resource> generate(ObserverInfo observer) {
return Collections.emptyList();
}

boolean isApplicationClass = applicationClassPredicate.test(observer.getBeanClass());
boolean isApplicationClass = applicationClassPredicate.test(observer.getBeanClass())
|| observer.isForceApplicationClass();
ResourceClassOutput classOutput = new ResourceClassOutput(isApplicationClass,
name -> name.equals(generatedName) ? SpecialType.OBSERVER : null, generateSources);

Expand Down