Skip to content

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Nov 7, 2019

No description provided.

@FroMage
Copy link
Member Author

FroMage commented Nov 7, 2019

@loicmathieu do you have time to review this? I took the liberty to fix the same bug for mongo-panache.

@FroMage FroMage requested a review from loicmathieu November 7, 2019 10:58
@FroMage
Copy link
Member Author

FroMage commented Nov 7, 2019

@gsmet this is a candidate fix for 1.0 as it's simple, tested, low-risk and affects an existing user. How do I need to tag it?


// 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

@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 :)

@FroMage
Copy link
Member Author

FroMage commented Nov 7, 2019

Huh, it doesn't look like your review is enough. @gsmet ?

@gsmet
Copy link
Member

gsmet commented Nov 7, 2019 via email

@FroMage
Copy link
Member Author

FroMage commented Nov 7, 2019

4 commits: two fixes, two tests. I can squash them if you prefer.

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@gastaldi gastaldi added this to the 1.0.0.Final milestone Nov 7, 2019
@gastaldi gastaldi merged commit ffdb0b5 into quarkusio:master Nov 7, 2019
@gastaldi gastaldi modified the milestones: 1.0.0.Final, 1.1.0 Nov 8, 2019
@gsmet gsmet removed the backport? label Nov 14, 2019
@gsmet gsmet modified the milestones: 1.1.0, 1.0.0.Final Nov 14, 2019
gsmet pushed a commit that referenced this pull request Nov 14, 2019
* Panache: do not create findById bridge for abstract entity repositories

If the entity type is not fixed in the hierarchy yet, don't generate the bridge

Fix #5274

* Test for #5274: duplicate findById for abstract repositories

* Fix #5274 for Mongo too

* Test for #5274 for Mongo
ia3andy pushed a commit to dmlloyd/quarkus that referenced this pull request Nov 19, 2019
* Panache: do not create findById bridge for abstract entity repositories

If the entity type is not fixed in the hierarchy yet, don't generate the bridge

Fix quarkusio#5274

* Test for quarkusio#5274: duplicate findById for abstract repositories

* Fix quarkusio#5274 for Mongo too

* Test for quarkusio#5274 for Mongo
basalt79 added a commit to basalt79/mg-backend that referenced this pull request Nov 21, 2019
mmusgrov pushed a commit to mmusgrov/quarkus that referenced this pull request Dec 13, 2019
* Panache: do not create findById bridge for abstract entity repositories

If the entity type is not fixed in the hierarchy yet, don't generate the bridge

Fix quarkusio#5274

* Test for quarkusio#5274: duplicate findById for abstract repositories

* Fix quarkusio#5274 for Mongo too

* Test for quarkusio#5274 for Mongo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants