Skip to content

Commit 8066e2a

Browse files
FroMageFroMage
committed
KotlinPanache: simplify and fix user method detection
Fixes #50144
1 parent 9891318 commit 8066e2a

File tree

2 files changed

+128
-60
lines changed

2 files changed

+128
-60
lines changed

‎extensions/panache/hibernate-orm-panache-kotlin/deployment/src/test/kotlin/io/quarkus/hibernate/orm/panache/kotlin/deployment/test/Bug50144Test.kt‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
package io.quarkus.hibernate.orm.panache.kotlin.deployment.test
2+
3+
import io.quarkus.hibernate.orm.panache.kotlin.PanacheRepositoryBase
4+
import io.quarkus.test.QuarkusUnitTest
5+
import jakarta.enterprise.context.ApplicationScoped
6+
import jakarta.inject.Inject
7+
import jakarta.persistence.Entity
8+
import jakarta.persistence.GeneratedValue
9+
import jakarta.persistence.GenerationType
10+
import jakarta.persistence.Id
11+
import jakarta.transaction.Transactional
12+
import org.jboss.shrinkwrap.api.ShrinkWrap
13+
import org.jboss.shrinkwrap.api.spec.JavaArchive
14+
import org.junit.jupiter.api.extension.RegisterExtension
15+
import org.junit.jupiter.api.assertThrows
16+
import org.junit.jupiter.api.Test
17+
import java.util.UUID
18+
19+
class Bug50144Test {
20+
companion object {
21+
@RegisterExtension
22+
@JvmField
23+
var runner = QuarkusUnitTest()
24+
.setArchiveProducer {
25+
ShrinkWrap.create(JavaArchive::class.java)
26+
.addClasses(WorkingAssignmentRepository::class.java, BrokenAssignmentRepository::class.java,
27+
WorkingBaseRepository::class.java, BrokenBaseRepository::class.java,
28+
Assignment::class.java, NotFoundException::class.java)
29+
}
30+
}
31+
32+
@Inject
33+
private lateinit var workingAssignmentRepository: WorkingAssignmentRepository
34+
35+
@Inject
36+
private lateinit var brokenAssignmentRepository: BrokenAssignmentRepository
37+
38+
@Transactional
39+
@Test
40+
fun `A working example`() {
41+
val id: UUID = UUID.randomUUID()
42+
43+
assertThrows<NotFoundException> {
44+
workingAssignmentRepository.findByIdOrThrow(id)
45+
}
46+
}
47+
48+
@Transactional
49+
@Test
50+
fun `This breaks for some reason`() {
51+
val id: UUID = UUID.randomUUID()
52+
53+
assertThrows<NotFoundException> {
54+
brokenAssignmentRepository.findByIdOrThrow(id)
55+
}
56+
}
57+
58+
class NotFoundException: Exception()
59+
60+
@Entity
61+
class Assignment {
62+
@Id
63+
@GeneratedValue(strategy = GenerationType.UUID)
64+
private lateinit var _id: UUID
65+
}
66+
67+
abstract class WorkingBaseRepository<EntityT : Any, IdT: Any> : PanacheRepositoryBase<EntityT, IdT> {
68+
fun findByIdOrThrow(
69+
id: IdT,
70+
exception: (IdT) -> Exception,
71+
): EntityT = findById(id) ?: throw exception(id)
72+
}
73+
74+
@ApplicationScoped
75+
class WorkingAssignmentRepository : WorkingBaseRepository<Assignment, UUID>() {
76+
fun findByIdOrThrow(assignmentId: UUID): Assignment {
77+
return findByIdOrThrow(assignmentId) {
78+
NotFoundException()
79+
}
80+
}
81+
}
82+
83+
abstract class BrokenBaseRepository<EntityT : Any, IdT: UUID> : PanacheRepositoryBase<EntityT, IdT> {
84+
fun findByIdOrThrow(
85+
id: IdT,
86+
exception: (IdT) -> Exception,
87+
): EntityT = findById(id) ?: throw exception(id)
88+
}
89+
90+
@ApplicationScoped
91+
class BrokenAssignmentRepository : BrokenBaseRepository<Assignment, UUID>() {
92+
fun findByIdOrThrow(assignmentId: UUID): Assignment {
93+
return findByIdOrThrow(assignmentId) {
94+
NotFoundException()
95+
}
96+
}
97+
}
98+
99+
100+
}

‎extensions/panache/panache-common/deployment/src/main/java/io/quarkus/panache/common/deployment/visitors/KotlinPanacheClassOperationGenerationVisitor.java‎

Lines changed: 28 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import static io.quarkus.deployment.util.AsmUtil.unboxIfRequired;
55
import static io.quarkus.gizmo.Gizmo.ASM_API_VERSION;
66
import static io.quarkus.panache.common.deployment.PanacheConstants.DOTNAME_GENERATE_BRIDGE;
7-
import static java.lang.String.format;
87
import static java.util.stream.Collectors.toList;
98
import static org.objectweb.asm.Opcodes.ACC_BRIDGE;
109
import static org.objectweb.asm.Opcodes.ARRAYLENGTH;
@@ -25,10 +24,11 @@
2524
import java.util.ArrayList;
2625
import java.util.Collections;
2726
import java.util.HashMap;
27+
import java.util.HashSet;
2828
import java.util.List;
2929
import java.util.Map;
30+
import java.util.Set;
3031
import java.util.StringJoiner;
31-
import java.util.TreeMap;
3232
import java.util.function.Function;
3333

3434
import org.jboss.jandex.AnnotationInstance;
@@ -68,10 +68,11 @@ public class KotlinPanacheClassOperationGenerationVisitor extends ClassVisitor {
6868
protected final Function<String, Type> argMapper;
6969
protected final ClassInfo classInfo;
7070
protected final ByteCodeType entityUpperBound;
71+
// These are type arguments to the Panache base entity/repo, not to the current or intermediate type
7172
protected final Map<String, ByteCodeType> typeArguments = new HashMap<>();
7273
private final ByteCodeType baseType;
73-
private final Map<String, MethodInfo> definedMethods = new TreeMap<>();
74-
74+
private final Set<String> userMethods = new HashSet<>();
75+
private final Set<String> baseTypeMethods = new HashSet<>();
7576
private final Map<String, String> erasures = new HashMap<>();
7677
private final IndexView indexView;
7778
protected List<PanacheMethodCustomizer> methodCustomizers;
@@ -101,10 +102,20 @@ public KotlinPanacheClassOperationGenerationVisitor(ClassVisitor outputClassVisi
101102
? byteCodeType.get()
102103
: null;
103104
};
105+
loadBaseTypeMethods();
106+
}
104107

105-
collectMethods(classInfo);
106-
filterNonOverrides();
107-
108+
/**
109+
* This loads the signatures of every base type method that requires a bridge
110+
*/
111+
private void loadBaseTypeMethods() {
112+
for (MethodInfo method : indexView.getClassByName(baseType.dotName()).methods()) {
113+
String descriptor = method.descriptor(type -> typeArguments.getOrDefault(type, OBJECT).get());
114+
AnnotationInstance bridge = method.annotation(DOTNAME_GENERATE_BRIDGE);
115+
if (bridge != null) {
116+
baseTypeMethods.add(method.name() + "/" + descriptor);
117+
}
118+
}
108119
}
109120

110121
public static List<ByteCodeType> recursivelyFindEntityTypeArguments(IndexView indexView, DotName clazz,
@@ -217,27 +228,6 @@ private void checkCast(MethodVisitor mv, Type returnType, String operationReturn
217228
}
218229
}
219230

220-
private void collectMethods(ClassInfo classInfo) {
221-
if (classInfo != null && !classInfo.name().equals(baseType.dotName())) {
222-
classInfo.methods()
223-
.forEach(method -> {
224-
String descriptor = method.descriptor(m -> {
225-
ByteCodeType byteCodeType = typeArguments.get(m);
226-
return byteCodeType != null ? byteCodeType.get() : OBJECT.get();
227-
});
228-
MethodInfo prior = definedMethods.put(method.name() + descriptor, method);
229-
if (prior != null && !isBridgeMethod(method)) {
230-
throw new IllegalStateException(format("Should not run in to duplicate " +
231-
"mappings: \n\t%s\n\t%s\n\t%s", method, descriptor, prior));
232-
}
233-
});
234-
DotName superName = classInfo.superName();
235-
if (superName != null) {
236-
collectMethods(indexView.getClassByName(superName));
237-
}
238-
}
239-
}
240-
241231
private String desc(String name) {
242232
String s = name.replace(".", "/");
243233
return s.startsWith("[") ? s : "L" + s + ";";
@@ -313,16 +303,6 @@ private Label endLabel() {
313303
return labels.get(labels.size() - 1);
314304
}
315305

316-
private void filterNonOverrides() {
317-
new ArrayList<>(definedMethods.values())
318-
.forEach(method -> {
319-
AnnotationInstance generateBridge = method.annotation(DOTNAME_GENERATE_BRIDGE);
320-
if (generateBridge != null) {
321-
definedMethods.remove(method.name() + method.descriptor());
322-
}
323-
});
324-
}
325-
326306
private void generate(MethodInfo method) {
327307
// Note: we can't use SYNTHETIC here because otherwise Mockito will never mock these methods
328308
MethodVisitor mv = cv.visitMethod(Opcodes.ACC_PUBLIC, method.name(),
@@ -460,10 +440,6 @@ private void invokeOperation(MethodVisitor mv, MethodInfo method) {
460440
mv.visitInsn(AsmUtil.getReturnInstruction(returnType));
461441
}
462442

463-
private boolean isBridgeMethod(MethodInfo method) {
464-
return (method.flags() & ACC_BRIDGE) != ACC_BRIDGE;
465-
}
466-
467443
private org.objectweb.asm.Type asmType(Type methodParameter) {
468444
org.objectweb.asm.Type parameter;
469445
if (methodParameter.kind() == Type.Kind.TYPE_VARIABLE) {
@@ -531,38 +507,30 @@ public String toString() {
531507
@Override
532508
public MethodVisitor visitMethod(int access, String name, String descriptor, String signature,
533509
String[] exceptions) {
534-
535-
MethodInfo methodInfo = definedMethods.entrySet().stream()
536-
.filter(e -> e.getKey().equals(name + descriptor))
537-
.map(e -> e.getValue())
538-
.findFirst()
539-
.orElse(null);
540-
if (methodInfo != null && !methodInfo.hasAnnotation(DOTNAME_GENERATE_BRIDGE)) {
541-
return super.visitMethod(access, name, descriptor, signature, exceptions);
542-
} else if (name.contains("$")) {
543-
//some agents such as jacoco add new methods, they generally have $ in the name
544-
return super.visitMethod(access, name, descriptor, signature, exceptions);
545-
} else if (name.equals(CTOR_METHOD_NAME) || name.equals(CLINIT_METHOD_NAME)) {
546-
//Arc can add no-args constructors to support intercepted beans
547-
// Logging with Panache can add a class initializer
548-
return super.visitMethod(access, name, descriptor, signature, exceptions);
510+
// Kotlinc or something will add bridge methods for the base type methods, these are not user methods
511+
// so we filter them out since we add them back in visitEnd()
512+
String sig = name + "/" + descriptor;
513+
if ((access & Opcodes.ACC_BRIDGE) != 0
514+
&& baseTypeMethods.contains(sig)) {
515+
return null;
549516
}
550-
return null;
517+
userMethods.add(sig);
518+
return super.visitMethod(access, name, descriptor, signature, exceptions);
551519
}
552520

553521
@Override
554522
public void visitEnd() {
555523
for (MethodInfo method : indexView.getClassByName(baseType.dotName()).methods()) {
556524
String descriptor = method.descriptor(type -> typeArguments.getOrDefault(type, OBJECT).get());
557525
AnnotationInstance bridge = method.annotation(DOTNAME_GENERATE_BRIDGE);
558-
if (!definedMethods.containsKey(method.name() + descriptor) && bridge != null) {
526+
if (!userMethods.contains(method.name() + "/" + descriptor) && bridge != null) {
559527
generate(method);
560528
if (needsJvmBridge(method)) {
561529
String bridgeDescriptor = bridgeMethodDescriptor(method, type -> {
562530
ByteCodeType mapped = typeArguments.get(type);
563531
return mapped != null ? mapped.get() : null;
564532
});
565-
if (!definedMethods.containsKey(method.name() + bridgeDescriptor)) {
533+
if (!userMethods.contains(method.name() + "/" + bridgeDescriptor)) {
566534
generateBridge(method, bridgeDescriptor);
567535
}
568536

0 commit comments

Comments
 (0)