Skip to content

Commit f2fa3c8

Browse files
geoandgeoand
authored
Merge pull request quarkusio#47876 from geoand/quarkusio#24340
Make placing JPA listener annotations a little safer
2 parents 0567c9a + 3da3ff8 commit f2fa3c8

File tree

4 files changed

+283
-18
lines changed

4 files changed

+283
-18
lines changed

‎extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmCdiProcessor.java‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22

33
import static org.apache.commons.lang3.BooleanUtils.isFalse;
44

5+
import java.lang.reflect.Modifier;
56
import java.util.ArrayList;
67
import java.util.Arrays;
78
import java.util.List;
89
import java.util.Locale;
910
import java.util.Set;
11+
import java.util.function.BiFunction;
1012
import java.util.function.Function;
1113
import java.util.stream.Collectors;
1214

@@ -24,26 +26,35 @@
2426
import org.hibernate.query.criteria.HibernateCriteriaBuilder;
2527
import org.hibernate.relational.SchemaManager;
2628
import org.jboss.jandex.AnnotationInstance;
29+
import org.jboss.jandex.AnnotationTarget;
2730
import org.jboss.jandex.AnnotationTarget.Kind;
2831
import org.jboss.jandex.AnnotationTransformation;
2932
import org.jboss.jandex.AnnotationValue;
33+
import org.jboss.jandex.ClassInfo;
3034
import org.jboss.jandex.ClassType;
35+
import org.jboss.jandex.CompositeIndex;
3136
import org.jboss.jandex.DotName;
3237
import org.jboss.jandex.FieldInfo;
38+
import org.jboss.jandex.MethodInfo;
3339
import org.jboss.jandex.ParameterizedType;
3440
import org.jboss.jandex.Type;
41+
import org.objectweb.asm.ClassVisitor;
3542

3643
import io.agroal.api.AgroalDataSource;
3744
import io.quarkus.agroal.spi.JdbcDataSourceBuildItem;
3845
import io.quarkus.arc.deployment.AdditionalBeanBuildItem;
3946
import io.quarkus.arc.deployment.AnnotationsTransformerBuildItem;
4047
import io.quarkus.arc.deployment.BeanDefiningAnnotationBuildItem;
48+
import io.quarkus.arc.deployment.BeanDiscoveryFinishedBuildItem;
4149
import io.quarkus.arc.deployment.SyntheticBeanBuildItem;
4250
import io.quarkus.arc.deployment.SyntheticBeanBuildItem.ExtendedBeanConfigurator;
4351
import io.quarkus.arc.deployment.UnremovableBeanBuildItem;
4452
import io.quarkus.arc.deployment.ValidationPhaseBuildItem;
4553
import io.quarkus.arc.processor.AnnotationsTransformer;
54+
import io.quarkus.arc.processor.BeanInfo;
55+
import io.quarkus.arc.processor.BuiltinScope;
4656
import io.quarkus.arc.processor.DotNames;
57+
import io.quarkus.arc.processor.ScopeInfo;
4758
import io.quarkus.arc.processor.Transformation;
4859
import io.quarkus.deployment.Capabilities;
4960
import io.quarkus.deployment.Capability;
@@ -52,7 +63,10 @@
5263
import io.quarkus.deployment.annotations.BuildSteps;
5364
import io.quarkus.deployment.annotations.ExecutionTime;
5465
import io.quarkus.deployment.annotations.Record;
66+
import io.quarkus.deployment.builditem.BytecodeTransformerBuildItem;
5567
import io.quarkus.deployment.builditem.CombinedIndexBuildItem;
68+
import io.quarkus.gizmo.ClassTransformer;
69+
import io.quarkus.gizmo.MethodDescriptor;
5670
import io.quarkus.hibernate.orm.PersistenceUnit;
5771
import io.quarkus.hibernate.orm.runtime.HibernateOrmRecorder;
5872
import io.quarkus.hibernate.orm.runtime.HibernateOrmRuntimeConfig;
@@ -276,6 +290,62 @@ void registerBeans(HibernateOrmConfig hibernateOrmConfig,
276290
unremovableBeans.produce(UnremovableBeanBuildItem.beanTypes(jpaModel.getPotentialCdiBeanClassNames()));
277291
}
278292

293+
@BuildStep
294+
void transformBeans(JpaModelBuildItem jpaModel, JpaModelIndexBuildItem indexBuildItem,
295+
BeanDiscoveryFinishedBuildItem beans,
296+
BuildProducer<BytecodeTransformerBuildItem> producer) {
297+
if (!HibernateOrmProcessor.hasEntities(jpaModel)) {
298+
return;
299+
}
300+
301+
// the idea here is to remove the 'private' modifier from all methods that are annotated with JPA Listener methods
302+
// and don't belong to entities
303+
CompositeIndex index = indexBuildItem.getIndex();
304+
for (DotName dotName : jpaModel.getPotentialCdiBeanClassNames()) {
305+
if (jpaModel.getManagedClassNames().contains(dotName.toString())) {
306+
continue;
307+
}
308+
ClassInfo classInfo = index.getClassByName(dotName);
309+
List<BeanInfo> matchingBeans = beans.getBeans().stream().filter(bi -> bi.getBeanClass().equals(dotName)).toList();
310+
if (matchingBeans.size() == 1) {
311+
ScopeInfo beanScope = matchingBeans.get(0).getScope();
312+
for (DotName jpaListenerDotName : ClassNames.JPA_LISTENER_ANNOTATIONS) {
313+
for (AnnotationInstance annotationInstance : classInfo.annotations(jpaListenerDotName)) {
314+
AnnotationTarget target = annotationInstance.target();
315+
if (target.kind() != AnnotationTarget.Kind.METHOD) {
316+
continue;
317+
}
318+
MethodInfo method = target.asMethod();
319+
if (Modifier.isPrivate(method.flags())) {
320+
if (beanScope.getDotName().equals(BuiltinScope.SINGLETON.getName())) {
321+
// we can safely transform in this case
322+
producer.produce(new BytecodeTransformerBuildItem(method.declaringClass().name().toString(),
323+
new BiFunction<>() {
324+
@Override
325+
public ClassVisitor apply(String cls, ClassVisitor clsVisitor) {
326+
var classTransformer = new ClassTransformer(cls);
327+
classTransformer.modifyMethod(MethodDescriptor.of(method))
328+
.removeModifiers(Modifier.PRIVATE);
329+
return classTransformer.applyTo(clsVisitor);
330+
}
331+
}));
332+
} else {
333+
// we can't transform because the client proxy does not know about the transformation and
334+
// will therefore simply copy the private method which will then likely fail because it does
335+
// not contain the injected fields
336+
throw new IllegalArgumentException(
337+
"Methods that are annotated with JPA Listener annotations should not be private. Offending method is '"
338+
+ method.declaringClass().name() + "#" + method.name() + "'");
339+
}
340+
}
341+
}
342+
}
343+
} else {
344+
// we don't really know what to do here, just bail and CDI will figure it out
345+
}
346+
}
347+
}
348+
279349
@BuildStep
280350
void registerAnnotations(BuildProducer<AdditionalBeanBuildItem> additionalBeans,
281351
BuildProducer<BeanDefiningAnnotationBuildItem> beanDefiningAnnotations) {

‎extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/JpaJandexScavenger.java‎

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -429,24 +429,13 @@ private void enlistPotentialCdiBeanClasses(Collector collector, DotName dotName)
429429

430430
for (AnnotationInstance annotation : jpaAnnotations) {
431431
AnnotationTarget target = annotation.target();
432-
ClassInfo beanType;
433-
switch (target.kind()) {
434-
case CLASS:
435-
beanType = target.asClass();
436-
break;
437-
case FIELD:
438-
beanType = target.asField().declaringClass();
439-
break;
440-
case METHOD:
441-
beanType = target.asMethod().declaringClass();
442-
break;
443-
case METHOD_PARAMETER:
444-
case TYPE:
445-
case RECORD_COMPONENT:
446-
default:
447-
throw new IllegalArgumentException(
448-
"Annotation " + dotName + " was not expected on a target of kind " + target.kind());
449-
}
432+
ClassInfo beanType = switch (target.kind()) {
433+
case CLASS -> target.asClass();
434+
case FIELD -> target.asField().declaringClass();
435+
case METHOD -> target.asMethod().declaringClass();
436+
default -> throw new IllegalArgumentException(
437+
"Annotation " + dotName + " was not expected on a target of kind " + target.kind());
438+
};
450439
DotName beanTypeDotName = beanType.name();
451440
collector.potentialCdiBeanTypes.add(beanTypeDotName);
452441
}

‎extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/JpaListenerOnPrivateMethodOfApplicationScopedCdiBeanTest.java‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package io.quarkus.hibernate.orm;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.fail;
5+
6+
import jakarta.enterprise.context.ApplicationScoped;
7+
import jakarta.persistence.Entity;
8+
import jakarta.persistence.EntityListeners;
9+
import jakarta.persistence.GeneratedValue;
10+
import jakarta.persistence.GenerationType;
11+
import jakarta.persistence.Id;
12+
import jakarta.persistence.PostPersist;
13+
import jakarta.transaction.Transactional;
14+
15+
import org.junit.jupiter.api.Test;
16+
import org.junit.jupiter.api.extension.RegisterExtension;
17+
18+
import io.quarkus.test.QuarkusUnitTest;
19+
20+
public class JpaListenerOnPrivateMethodOfApplicationScopedCdiBeanTest {
21+
22+
@RegisterExtension
23+
static QuarkusUnitTest runner = new QuarkusUnitTest()
24+
.withApplicationRoot((jar) -> jar
25+
.addAsResource("application.properties"))
26+
.assertException(e -> {
27+
assertThat(e).isInstanceOf(IllegalArgumentException.class)
28+
.hasMessageContaining("SomeEntityListener#postPersist");
29+
});
30+
31+
@Test
32+
@Transactional
33+
public void test() {
34+
fail("should never be called");
35+
}
36+
37+
@Entity
38+
@EntityListeners(SomeEntityListener.class)
39+
public static class SomeEntity {
40+
private long id;
41+
private String name;
42+
43+
public SomeEntity() {
44+
}
45+
46+
public SomeEntity(String name) {
47+
this.name = name;
48+
}
49+
50+
@Id
51+
@GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "myEntitySeq")
52+
public long getId() {
53+
return id;
54+
}
55+
56+
public void setId(long id) {
57+
this.id = id;
58+
}
59+
60+
public String getName() {
61+
return name;
62+
}
63+
64+
public void setName(String name) {
65+
this.name = name;
66+
}
67+
68+
@Override
69+
public String toString() {
70+
return "SomeEntity:" + name;
71+
}
72+
}
73+
74+
@ApplicationScoped
75+
public static class SomeEntityListener {
76+
77+
@PostPersist
78+
private void postPersist(SomeEntity someEntity) {
79+
fail("should not reach here");
80+
}
81+
}
82+
}

‎extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/JpaListenerOnPrivateMethodOfSingletonCdiBeanTest.java‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
package io.quarkus.hibernate.orm;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
5+
import java.util.List;
6+
import java.util.concurrent.CopyOnWriteArrayList;
7+
8+
import jakarta.enterprise.context.ApplicationScoped;
9+
import jakarta.inject.Inject;
10+
import jakarta.inject.Singleton;
11+
import jakarta.persistence.Entity;
12+
import jakarta.persistence.EntityListeners;
13+
import jakarta.persistence.EntityManager;
14+
import jakarta.persistence.GeneratedValue;
15+
import jakarta.persistence.GenerationType;
16+
import jakarta.persistence.Id;
17+
import jakarta.persistence.PostPersist;
18+
import jakarta.persistence.PrePersist;
19+
import jakarta.transaction.Transactional;
20+
21+
import org.junit.jupiter.api.Test;
22+
import org.junit.jupiter.api.extension.RegisterExtension;
23+
24+
import io.quarkus.arc.Arc;
25+
import io.quarkus.test.QuarkusUnitTest;
26+
27+
public class JpaListenerOnPrivateMethodOfSingletonCdiBeanTest {
28+
29+
@RegisterExtension
30+
static QuarkusUnitTest runner = new QuarkusUnitTest()
31+
.withApplicationRoot((jar) -> jar
32+
.addAsResource("application.properties"));
33+
34+
@Inject
35+
EventStore eventStore;
36+
37+
@Inject
38+
EntityManager em;
39+
40+
@Test
41+
@Transactional
42+
public void test() {
43+
Arc.container().requestContext().activate();
44+
try {
45+
SomeEntity entity = new SomeEntity("test");
46+
em.persist(entity);
47+
em.flush();
48+
} finally {
49+
Arc.container().requestContext().terminate();
50+
}
51+
52+
assertThat(eventStore.getEvents()).containsExactly("prePersist", "postPersist");
53+
}
54+
55+
@Entity
56+
@EntityListeners(SomeEntityListener.class)
57+
public static class SomeEntity {
58+
private long id;
59+
private String name;
60+
61+
public SomeEntity() {
62+
}
63+
64+
public SomeEntity(String name) {
65+
this.name = name;
66+
}
67+
68+
@Id
69+
@GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "myEntitySeq")
70+
public long getId() {
71+
return id;
72+
}
73+
74+
public void setId(long id) {
75+
this.id = id;
76+
}
77+
78+
public String getName() {
79+
return name;
80+
}
81+
82+
public void setName(String name) {
83+
this.name = name;
84+
}
85+
86+
@Override
87+
public String toString() {
88+
return "SomeEntity:" + name;
89+
}
90+
}
91+
92+
@Singleton
93+
public static class SomeEntityListener {
94+
95+
private final EventStore eventStore;
96+
97+
public SomeEntityListener(EventStore eventStore) {
98+
this.eventStore = eventStore;
99+
}
100+
101+
@PrePersist
102+
void prePersist(SomeEntity someEntity) {
103+
eventStore.addEvent("prePersist");
104+
}
105+
106+
@PostPersist
107+
private void postPersist(SomeEntity someEntity) {
108+
eventStore.addEvent("postPersist");
109+
}
110+
}
111+
112+
@ApplicationScoped
113+
public static class EventStore {
114+
private final List<String> events = new CopyOnWriteArrayList<>();
115+
116+
public void addEvent(String event) {
117+
events.add(event);
118+
}
119+
120+
public List<String> getEvents() {
121+
return events;
122+
}
123+
}
124+
}

0 commit comments

Comments
 (0)