Skip to content

Fix for 5274 #5276

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

Merged
merged 4 commits into from
Nov 7, 2019
Merged

Fix for 5274 #5276

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
d2a645d
Panache: do not create findById bridge for abstract entity repositories
FroMage Nov 7, 2019
7604ce0
Test for #5274: duplicate findById for abstract repositories
FroMage Nov 7, 2019
34df40e
Fix #5274 for Mongo too
FroMage Nov 7, 2019
df79555
Test for #5274 for Mongo
FroMage Nov 7, 2019
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
38 changes: 20 additions & 18 deletions ...c/main/java/io/quarkus/hibernate/orm/panache/deployment/PanacheJpaRepositoryEnhancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,26 @@ protected String getPanacheOperationsBinaryName() {

@Override
public void visitEnd() {
// Bridge for findById
MethodVisitor mv = super.visitMethod(Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC | Opcodes.ACC_BRIDGE,
"findById",
"(Ljava/lang/Object;)Ljava/lang/Object;",
null,
null);
mv.visitParameter("id", 0);
mv.visitCode();
mv.visitIntInsn(Opcodes.ALOAD, 0);
mv.visitIntInsn(Opcodes.ALOAD, 1);
mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL,
daoBinaryName,
"findById",
"(Ljava/lang/Object;)" + entitySignature, false);
mv.visitInsn(Opcodes.ARETURN);
mv.visitMaxs(0, 0);
mv.visitEnd();

// Bridge for findById, but only if we actually know the end entity (which we don't for intermediate
// abstract repositories that haven't fixed their entity type yet
if (!"Ljava/lang/Object;".equals(entitySignature)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you just avoid any enahcement for abstract repositories/entities ?
This way it will speed up the build process no ?
And you will also be able to remove some cheks inside the processor as PanacheEntity itself is abstract

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this also happens for non-abstract classes, as my test shows.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

MethodVisitor mv = super.visitMethod(Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC | Opcodes.ACC_BRIDGE,
"findById",
"(Ljava/lang/Object;)Ljava/lang/Object;",
null,
null);
mv.visitParameter("id", 0);
mv.visitCode();
mv.visitIntInsn(Opcodes.ALOAD, 0);
mv.visitIntInsn(Opcodes.ALOAD, 1);
mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL,
daoBinaryName,
"findById",
"(Ljava/lang/Object;)" + entitySignature, false);
mv.visitInsn(Opcodes.ARETURN);
mv.visitMaxs(0, 0);
mv.visitEnd();
}
super.visitEnd();
}

Expand Down
37 changes: 20 additions & 17 deletions ...t/src/main/java/io/quarkus/mongodb/panache/deployment/PanacheMongoRepositoryEnhancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,26 @@ protected String getPanacheOperationsBinaryName() {

@Override
public void visitEnd() {
// Bridge for findById
MethodVisitor mv = super.visitMethod(Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC | Opcodes.ACC_BRIDGE,
"findById",
"(Ljava/lang/Object;)Ljava/lang/Object;",
null,
null);
mv.visitParameter("id", 0);
mv.visitCode();
mv.visitIntInsn(Opcodes.ALOAD, 0);
mv.visitIntInsn(Opcodes.ALOAD, 1);
mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL,
daoBinaryName,
"findById",
"(Ljava/lang/Object;)" + entitySignature, false);
mv.visitInsn(Opcodes.ARETURN);
mv.visitMaxs(0, 0);
mv.visitEnd();
// Bridge for findById, but only if we actually know the end entity (which we don't for intermediate
// abstract repositories that haven't fixed their entity type yet
if (!entitySignature.equals("Ljava/lang/Object;")) {
MethodVisitor mv = super.visitMethod(Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC | Opcodes.ACC_BRIDGE,
"findById",
"(Ljava/lang/Object;)Ljava/lang/Object;",
null,
null);
mv.visitParameter("id", 0);
mv.visitCode();
mv.visitIntInsn(Opcodes.ALOAD, 0);
mv.visitIntInsn(Opcodes.ALOAD, 1);
mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL,
daoBinaryName,
"findById",
"(Ljava/lang/Object;)" + entitySignature, false);
mv.visitInsn(Opcodes.ARETURN);
mv.visitMaxs(0, 0);
mv.visitEnd();
}

super.visitEnd();
}
Expand Down
6 changes: 6 additions & 0 deletions ...n-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/AbstractRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package io.quarkus.it.panache;

import io.quarkus.hibernate.orm.panache.PanacheRepositoryBase;

public class AbstractRepository<T> implements PanacheRepositoryBase<T, String> {
}
7 changes: 7 additions & 0 deletions ...ts/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/Bug5274EntityRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package io.quarkus.it.panache;

import javax.enterprise.context.ApplicationScoped;

@ApplicationScoped
public class Bug5274EntityRepository extends AbstractRepository<Person> {
}
11 changes: 11 additions & 0 deletions ...gration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,4 +789,15 @@ public Person ignoredProperties() throws NoSuchMethodException, SecurityExceptio
person.status = Status.DECEASED;
return person;
}

@Inject
Bug5274EntityRepository bug5274EntityRepository;

@GET
@Path("5274")
@Transactional
public String testBug5274() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe named it testWithAbstractParentEntity and add a link to the bug will be more explainatory ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer having the issue number in the test name because that really has more info than whatever name I could come up with. We could add a link as comment, but I've never felt the need for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, as you prefere :)

bug5274EntityRepository.count();
return "OK";
}
}
5 changes: 5 additions & 0 deletions ...s/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,9 @@ public void testPanacheSerialisation() {
public void testPanacheInTest() {
Assertions.assertEquals(0, Person.count());
}

@Test
public void testBug5274() {
RestAssured.when().get("/test/5274").then().body(is("OK"));
}
}
6 changes: 6 additions & 0 deletions .../mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/bugs/AbstractRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package io.quarkus.it.mongodb.panache.bugs;

import io.quarkus.mongodb.panache.PanacheMongoRepositoryBase;

public class AbstractRepository<T> implements PanacheMongoRepositoryBase<T, String> {
}
9 changes: 9 additions & 0 deletions ...odb-panache/src/main/java/io/quarkus/it/mongodb/panache/bugs/Bug5274EntityRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package io.quarkus.it.mongodb.panache.bugs;

import javax.enterprise.context.ApplicationScoped;

import io.quarkus.it.mongodb.panache.book.Book;

@ApplicationScoped
public class Bug5274EntityRepository extends AbstractRepository<Book> {
}
24 changes: 24 additions & 0 deletions ...n-tests/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/bugs/BugResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package io.quarkus.it.mongodb.panache.bugs;

import javax.inject.Inject;
import javax.ws.rs.Consumes;
import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;

@Path("/bugs")
@Produces(MediaType.TEXT_PLAIN)
@Consumes(MediaType.TEXT_PLAIN)
public class BugResource {

@Inject
Bug5274EntityRepository bug5274EntityRepository;

@GET
@Path("5274")
public String testBug5274() {
bug5274EntityRepository.count();
return "OK";
}
}
6 changes: 6 additions & 0 deletions ...n-tests/mongodb-panache/src/test/java/io/quarkus/it/mongodb/panache/BookResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.quarkus.it.mongodb.panache;

import static io.restassured.RestAssured.get;
import static org.hamcrest.Matchers.is;

import java.io.IOException;
import java.time.LocalDate;
Expand Down Expand Up @@ -324,4 +325,9 @@ private Date fromYear(int year) {
return Date.from(LocalDate.of(year, 1, 1).atStartOfDay().toInstant(ZoneOffset.UTC));
}

@Test
public void testBug5274() {
get("/bugs/5274").then().body(is("OK"));
}

}