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

ArC fixes for spec compatibility, round 10 #33523

Merged
merged 4 commits into from
May 29, 2023
Merged

ArC fixes for spec compatibility, round 10 #33523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b50f56d
ArC: fix InstanceHandle.close() to behave as specified in strict mode
Ladicek Jan 30, 2023
9c95114
ArC: fix inheritance of interceptor binding annotations
Ladicek Mar 3, 2023
778abc8
ArC: defer unproxyability errors to runtime in strict mode
Ladicek May 22, 2023
22c5798
ArC: upgrade Arquillian to 1.7.0.Final
Ladicek May 19, 2023
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 build-parent/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
<commonmark.version>0.21.0</commonmark.version>

<!-- Arquillian BOM -->
<arquillian.version>1.7.0.Alpha13</arquillian.version>
<arquillian.version>1.7.0.Final</arquillian.version>

<!-- Enable APT by default for Eclipse -->
<m2e.apt.activation>jdt_apt</m2e.apt.activation>
Expand Down
2 changes: 1 addition & 1 deletion independent-projects/arc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
<version.kotlin-coroutines>1.6.4</version.kotlin-coroutines>
<version.mockito>5.3.1</version.mockito>
<!-- TCK versions -->
<version.arquillian>1.7.0.Alpha14</version.arquillian>
<version.arquillian>1.7.0.Final</version.arquillian>
<version.atinject-tck>2.0.1</version.atinject-tck>
<version.cdi-tck>4.0.9</version.cdi-tck>
<version.junit4>4.13.2</version.junit4>
Expand Down
24 changes: 23 additions & 1 deletion ...pendent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanDeployment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ public class BeanDeployment {

private final Set<BeanInfo> removedBeans;

private final Set<BeanInfo> beansWithRuntimeDeferredUnproxyableError;

private final Map<ScopeInfo, Function<MethodCreator, ResultHandle>> customContexts;

private final Map<DotName, BeanDefiningAnnotation> beanDefiningAnnotations;
Expand Down Expand Up @@ -149,6 +151,7 @@ public class BeanDeployment {
this.removeUnusedBeans = builder.removeUnusedBeans;
this.unusedExclusions = removeUnusedBeans ? new ArrayList<>(builder.removalExclusions) : null;
this.removedBeans = removeUnusedBeans ? new CopyOnWriteArraySet<>() : Collections.emptySet();
this.beansWithRuntimeDeferredUnproxyableError = Collections.newSetFromMap(new ConcurrentHashMap<>());
this.customContexts = new ConcurrentHashMap<>();

this.excludeTypes = builder.excludeTypes != null ? new ArrayList<>(builder.excludeTypes) : Collections.emptyList();
Expand Down Expand Up @@ -506,6 +509,14 @@ public Collection<BeanInfo> getRemovedBeans() {
return Collections.unmodifiableSet(removedBeans);
}

boolean hasRuntimeDeferredUnproxyableError(BeanInfo bean) {
return beansWithRuntimeDeferredUnproxyableError.contains(bean);
}

void deferUnproxyableErrorToRuntime(BeanInfo bean) {
beansWithRuntimeDeferredUnproxyableError.add(bean);
}

public Collection<ClassInfo> getQualifiers() {
return Collections.unmodifiableCollection(qualifiers.values());
}
Expand Down Expand Up @@ -1522,6 +1533,17 @@ private void validateBeans(List<Throwable> errors, Consumer<BytecodeTransformer>
Map<String, List<BeanInfo>> namedBeans = new HashMap<>();
Set<DotName> classesReceivingNoArgsCtor = new HashSet<>();

// this set is only used in strict compatible mode (see `Beans.validateBean()`),
// so no need to initialize it otherwise
Set<BeanInfo> injectedBeans = new HashSet<>();
if (strictCompatibility) {
for (InjectionPointInfo injectionPoint : this.injectionPoints) {
if (injectionPoint.hasResolvedBean()) {
injectedBeans.add(injectionPoint.getResolvedBean());
}
}
}

for (BeanInfo bean : beans) {
if (bean.getName() != null) {
List<BeanInfo> named = namedBeans.get(bean.getName());
Expand All @@ -1532,7 +1554,7 @@ private void validateBeans(List<Throwable> errors, Consumer<BytecodeTransformer>
named.add(bean);
findNamespaces(bean, namespaces);
}
bean.validate(errors, bytecodeTransformerConsumer, classesReceivingNoArgsCtor);
bean.validate(errors, bytecodeTransformerConsumer, classesReceivingNoArgsCtor, injectedBeans);
}

if (!namedBeans.isEmpty()) {
Expand Down
10 changes: 9 additions & 1 deletion independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import jakarta.enterprise.context.spi.CreationalContext;
import jakarta.enterprise.inject.CreationException;
import jakarta.enterprise.inject.IllegalProductException;
import jakarta.enterprise.inject.UnproxyableResolutionException;
import jakarta.enterprise.inject.literal.InjectLiteral;
import jakarta.enterprise.inject.spi.InterceptionType;
import jakarta.interceptor.InvocationContext;
Expand Down Expand Up @@ -1927,7 +1928,10 @@ protected void implementGet(BeanInfo bean, ClassCreator beanCreator, ProviderTyp
MethodCreator get = beanCreator.getMethodCreator("get", providerType.descriptorName(), CreationalContext.class)
.setModifiers(ACC_PUBLIC);

if (BuiltinScope.DEPENDENT.is(bean.getScope())) {
if (bean.getDeployment().hasRuntimeDeferredUnproxyableError(bean)) {
get.throwException(UnproxyableResolutionException.class, "Bean not proxyable: " + bean);
get.returnValue(get.loadNull());
} else if (BuiltinScope.DEPENDENT.is(bean.getScope())) {
// @Dependent pseudo-scope
// Foo instance = create(ctx)
ResultHandle instance = get.invokeVirtualMethod(
Expand Down Expand Up @@ -2214,6 +2218,10 @@ private ResultHandle wrapCurrentInjectionPoint(BeanInfo bean,
}

private void initializeProxy(BeanInfo bean, String baseName, ClassCreator beanCreator) {
if (bean.getDeployment().hasRuntimeDeferredUnproxyableError(bean)) {
return;
}

// Add proxy volatile field
String proxyTypeName = getProxyTypeName(bean, baseName);
beanCreator.getFieldCreator(FIELD_NAME_PROXY, proxyTypeName)
Expand Down
17 changes: 11 additions & 6 deletions independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,8 +582,8 @@ public String getClientProxyPackageName() {
}

void validate(List<Throwable> errors, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
Set<DotName> classesReceivingNoArgsCtor) {
Beans.validateBean(this, errors, bytecodeTransformerConsumer, classesReceivingNoArgsCtor);
Set<DotName> classesReceivingNoArgsCtor, Set<BeanInfo> injectedBeans) {
Beans.validateBean(this, errors, bytecodeTransformerConsumer, classesReceivingNoArgsCtor, injectedBeans);
}

void validateInterceptorDecorator(List<Throwable> errors, Consumer<BytecodeTransformer> bytecodeTransformerConsumer) {
Expand Down Expand Up @@ -797,7 +797,7 @@ private void putLifecycleInterceptors(Map<InterceptionType, InterceptionInfo> li

private void addClassLevelBindings(ClassInfo targetClass, Collection<AnnotationInstance> bindings) {
List<AnnotationInstance> classLevelBindings = new ArrayList<>();
doAddClassLevelBindings(targetClass, classLevelBindings, Set.of());
doAddClassLevelBindings(targetClass, classLevelBindings, Set.of(), false);
bindings.addAll(classLevelBindings);
if (!stereotypes.isEmpty()) {
// interceptor binding declared on a bean class replaces an interceptor binding of the same type
Expand All @@ -808,22 +808,27 @@ private void addClassLevelBindings(ClassInfo targetClass, Collection<AnnotationI
}
for (StereotypeInfo stereotype : Beans.stereotypesWithTransitive(stereotypes,
beanDeployment.getStereotypesMap())) {
doAddClassLevelBindings(stereotype.getTarget(), bindings, skip);
doAddClassLevelBindings(stereotype.getTarget(), bindings, skip, false);
}
}
}

// bindings whose class name is present in `skip` are ignored (this is used to ignore bindings on stereotypes
// when the original class has a binding of the same type)
private void doAddClassLevelBindings(ClassInfo classInfo, Collection<AnnotationInstance> bindings, Set<DotName> skip) {
private void doAddClassLevelBindings(ClassInfo classInfo, Collection<AnnotationInstance> bindings, Set<DotName> skip,
boolean onlyInherited) {
beanDeployment.getAnnotations(classInfo).stream()
.filter(a -> beanDeployment.getInterceptorBinding(a.name()) != null)
.filter(a -> !skip.contains(a.name()))
.filter(a -> !onlyInherited
|| beanDeployment.hasAnnotation(beanDeployment.getInterceptorBinding(a.name()), DotNames.INHERITED))
.forEach(bindings::add);
if (classInfo.superClassType() != null && !classInfo.superClassType().name().equals(DotNames.OBJECT)) {
ClassInfo superClass = getClassByName(beanDeployment.getBeanArchiveIndex(), classInfo.superName());
if (superClass != null) {
doAddClassLevelBindings(superClass, bindings, skip);
// proper interceptor binding inheritance only in strict mode, due to Quarkus expecting security
// annotations (such as `@RolesAllowed`) to be inherited, even though they are not `@Inherited`
doAddClassLevelBindings(superClass, bindings, skip, beanDeployment.strictCompatibility);
}
}
}
Expand Down
90 changes: 55 additions & 35 deletions independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Beans.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,25 +711,34 @@ static void validateInterceptorDecorator(BeanInfo bean, List<Throwable> errors,
}

static void validateBean(BeanInfo bean, List<Throwable> errors, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
Set<DotName> classesReceivingNoArgsCtor) {
Set<DotName> classesReceivingNoArgsCtor, Set<BeanInfo> injectedBeans) {

// by default, we fail deployment due to unproxyability for all beans, but in strict mode,
// we only do that for beans that are injected somewhere -- and defer the error to runtime otherwise,
// due to CDI spec requirements
boolean failIfNotProxyable = bean.getDeployment().strictCompatibility ? injectedBeans.contains(bean) : true;

if (bean.isClassBean()) {
ClassInfo beanClass = bean.getTarget().get().asClass();
String classifier = bean.getScope().isNormal() ? "Normal scoped" : null;
if (classifier == null && bean.isSubclassRequired()) {
classifier = "Intercepted";
failIfNotProxyable = true;
}
if (Modifier.isFinal(beanClass.flags()) && classifier != null) {
// Client proxies and subclasses require a non-final class
if (bean.getDeployment().transformUnproxyableClasses) {
bytecodeTransformerConsumer
.accept(new BytecodeTransformer(beanClass.name().toString(), new FinalClassTransformFunction()));
} else {
} else if (failIfNotProxyable) {
errors.add(new DeploymentException(String.format("%s bean must not be final: %s", classifier, bean)));
} else {
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
}
if (bean.getDeployment().strictCompatibility && classifier != null) {
validateNonStaticFinalMethods(beanClass, bean.getDeployment().getBeanArchiveIndex(),
classifier, errors);
validateNonStaticFinalMethods(bean, beanClass, bean.getDeployment().getBeanArchiveIndex(),
classifier, errors, failIfNotProxyable);
}

MethodInfo noArgsConstructor = beanClass.method(Methods.INIT);
Expand All @@ -754,13 +763,16 @@ static void validateBean(BeanInfo bean, List<Throwable> errors, Consumer<Bytecod
classesReceivingNoArgsCtor.add(beanClass.name());
}

} else {
} else if (failIfNotProxyable) {
errors.add(cannotAddSyntheticNoArgsConstructor(beanClass));
} else {
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
} else if (failIfNotProxyable) {
errors.add(new DeploymentException(String.format(
"Normal scoped beans must declare a non-private constructor with no parameters: %s", bean)));
} else {
errors.add(new DeploymentException(String
.format("Normal scoped beans must declare a non-private constructor with no parameters: %s",
bean)));
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
}

Expand All @@ -770,12 +782,12 @@ static void validateBean(BeanInfo bean, List<Throwable> errors, Consumer<Bytecod
bytecodeTransformerConsumer.accept(
new BytecodeTransformer(beanClass.name().toString(),
new PrivateNoArgsConstructorTransformFunction()));
} else if (failIfNotProxyable) {
errors.add(new DeploymentException(String.format(
"%s bean is not proxyable because it has a private no-args constructor: %s. To fix this problem, change the constructor to be package-private",
classifier, bean)));
} else {
errors.add(
new DeploymentException(
String.format(
"%s bean is not proxyable because it has a private no-args constructor: %s. To fix this problem, change the constructor to be package-private",
classifier, bean)));
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
}

Expand Down Expand Up @@ -811,15 +823,16 @@ static void validateBean(BeanInfo bean, List<Throwable> errors, Consumer<Bytecod
bytecodeTransformerConsumer
.accept(new BytecodeTransformer(returnTypeClass.name().toString(),
new FinalClassTransformFunction()));
} else if (failIfNotProxyable) {
errors.add(new DeploymentException(
String.format("%s must not have a return type that is final: %s", classifier, bean)));
} else {
errors.add(
new DeploymentException(String.format("%s must not have a" +
" return type that is final: %s", classifier, bean)));
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
}
if (bean.getDeployment().strictCompatibility) {
validateNonStaticFinalMethods(returnTypeClass, bean.getDeployment().getBeanArchiveIndex(),
classifier, errors);
validateNonStaticFinalMethods(bean, returnTypeClass, bean.getDeployment().getBeanArchiveIndex(),
classifier, errors, failIfNotProxyable);
}
MethodInfo noArgsConstructor = returnTypeClass.method(Methods.INIT);
if (noArgsConstructor == null) {
Expand All @@ -844,22 +857,23 @@ static void validateBean(BeanInfo bean, List<Throwable> errors, Consumer<Bytecod
} else {
errors.add(cannotAddSyntheticNoArgsConstructor(returnTypeClass));
}
} else if (failIfNotProxyable) {
errors.add(new DefinitionException(String.format(
"Return type of a producer %s for normal scoped beans must declare a non-private constructor with no parameters: %s",
methodOrField, bean)));
} else {
errors.add(new DefinitionException(String
.format("Return type of a producer %s for normal scoped beans must" +
" declare a non-private constructor with no parameters: %s", methodOrField, bean)));
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
} else if (Modifier.isPrivate(noArgsConstructor.flags())) {
if (bean.getDeployment().transformUnproxyableClasses) {
bytecodeTransformerConsumer.accept(
new BytecodeTransformer(returnTypeClass.name().toString(),
new PrivateNoArgsConstructorTransformFunction()));
} else if (failIfNotProxyable) {
errors.add(new DeploymentException(String.format(
"%s is not proxyable because it has a private no-args constructor: %s.", classifier, bean)));
} else {
errors.add(
new DeploymentException(
String.format(
"%s is not proxyable because it has a private no-args constructor: %s.",
classifier, bean)));
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
}
}
Expand Down Expand Up @@ -894,21 +908,23 @@ static void validateBean(BeanInfo bean, List<Throwable> errors, Consumer<Bytecod
} else {
errors.add(cannotAddSyntheticNoArgsConstructor(beanClass));
}
} else if (failIfNotProxyable) {
errors.add(new DeploymentException(String.format(
"Normal scoped beans must declare a non-private constructor with no parameters: %s", bean)));
} else {
errors.add(new DeploymentException(String
.format("Normal scoped beans must declare a non-private constructor with no parameters: %s",
bean)));
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
}
if (bean.getScope().isNormal() && !Modifier.isInterface(beanClass.flags())
&& bean.getDeployment().strictCompatibility) {
validateNonStaticFinalMethods(beanClass, bean.getDeployment().getBeanArchiveIndex(), "Normal scoped", errors);
validateNonStaticFinalMethods(bean, beanClass, bean.getDeployment().getBeanArchiveIndex(), "Normal scoped",
errors, failIfNotProxyable);
}
}
}

private static void validateNonStaticFinalMethods(ClassInfo clazz, IndexView beanArchiveIndex,
String classifier, List<Throwable> errors) {
private static void validateNonStaticFinalMethods(BeanInfo bean, ClassInfo clazz, IndexView beanArchiveIndex,
String classifier, List<Throwable> errors, boolean failIfNotProxyable) {
// see also Methods.skipForClientProxy()
while (!clazz.name().equals(DotNames.OBJECT)) {
for (MethodInfo method : clazz.methods()) {
Expand All @@ -920,9 +936,13 @@ private static void validateNonStaticFinalMethods(ClassInfo clazz, IndexView bea
}

if (Modifier.isFinal(method.flags())) {
errors.add(new DeploymentException(String.format(
"%s bean must not declare non-static final methods with public, protected or default visibility: %s",
classifier, method)));
if (failIfNotProxyable) {
errors.add(new DeploymentException(String.format(
"%s bean must not declare non-static final methods with public, protected or default visibility: %s",
classifier, method)));
} else {
bean.getDeployment().deferUnproxyableErrorToRuntime(bean);
}
}
}

Expand Down
Loading