Skip to content

Conversation

loicmathieu
Copy link
Contributor

Fixes #1368

This is a draft PR as when #5583 is merged we will want to consider also having overload for with LockModeType for findByIdOptional.

cc @FroMage and @emmanuelbernard

@loicmathieu loicmathieu changed the title Panache/optionally find Provides Optional support inside Hibernate with Panache and MongoDB with Panache Nov 20, 2019
@loicmathieu loicmathieu force-pushed the panache/optionally-find branch from 8783c65 to 13ae9cd Compare November 20, 2019 10:00
@FroMage FroMage self-requested a review November 20, 2019 12:42
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

LGTM

@FroMage
Copy link
Member

FroMage commented Nov 20, 2019

Ah wait… it's missing tests.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Missing tests

@FroMage
Copy link
Member

FroMage commented Nov 20, 2019

CI failed due to formatting errors.

@loicmathieu loicmathieu force-pushed the panache/optionally-find branch 2 times, most recently from f0a4c42 to 76af4ab Compare November 20, 2019 15:42
@loicmathieu
Copy link
Contributor Author

@FroMage I add tests and correct a bunch of bugs :)

I added an overload on findByIdOptional with a LockModeType

Two questions:

  • I don't very like the implementatiion of PanacheQueryImpl.singleResultOptional that reads the database with a limit of 2 to be sure that there is 0 or 1 items. But I don't comes up with something better. If you have an idea ?
  • Generics implementation of PanacheEntityBase is not very easily usable and we need to use a not very known construct Person.<Person>findByIdOptional(personId); . I don't know if we have a better option here.

@FroMage
Copy link
Member

FroMage commented Nov 20, 2019

I don't very like the implementatiion of PanacheQueryImpl.singleResultOptional that reads the database with a limit of 2 to be sure that there is 0 or 1 items. But I don't comes up with something better. If you have an idea ?

No, I think that's fine.

Generics implementation of PanacheEntityBase is not very easily usable and we need to use a not very known construct Person.<Person>findByIdOptional(personId); . I don't know if we have a better option here.

The Person.<Person>findByIdOptional(personId); issue is the same you'll get with the RX mongodb API. We have to tell users to use intermediate variables:

Optional<Person> res = Person.findByIdOptional(personId);
res...

@loicmathieu loicmathieu force-pushed the panache/optionally-find branch from 76af4ab to 5da6f77 Compare November 20, 2019 16:09
@loicmathieu
Copy link
Contributor Author

@FroMage I updated the documenation to use an interlediate variable.
The PR is ready now :)

@loicmathieu loicmathieu marked this pull request as ready for review November 20, 2019 16:09
@FroMage
Copy link
Member

FroMage commented Nov 21, 2019

Keycloak tests failed due to:

Caused by: java.lang.ClassNotFoundException: io.quarkus.jackson.ObjectMapperProducer

Panache tests failed due to:

org.opentest4j.AssertionFailedError: expected: <{"id":null,"name":"max","uniqueName":null,"address":null,"status":null,"dogs":[],"serialisationTrick":1}> but was: <{"id":null,"name":"max","uniqueName":null,"address":null,"status":null,"dogs":[],"serialisationTrick":1,"persistent":false}>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1124)
	at io.quarkus.it.panache.PanacheFunctionalityTest.jacksonDeserilazationIgnoresPersitantAttribute(PanacheFunctionalityTest.java:71)

Except the last one is one of those tests that can't possibly fail. It's a test that's reusing a module which doesn't have the fix.

@loicmathieu
Copy link
Contributor Author

@FroMage I just launched the Hibernate with Panache native integration test on the branch locally and it pass. Can you re-launch the CI ? We saw multiple CI intermitent failures lately ....

@FroMage
Copy link
Member

FroMage commented Nov 21, 2019

Re-running them, but last time, once they started failing, they kept failing. The CI was corrupted. Let's see how this goes.

@machi1990
Copy link
Member

@loicmathieu you can rebase with master an force push to have a fresh CI run

@loicmathieu loicmathieu force-pushed the panache/optionally-find branch from 5da6f77 to 73d918b Compare November 21, 2019 13:25
@loicmathieu
Copy link
Contributor Author

@machi1990 done 🤞

@machi1990
Copy link
Member

Thanks @loicmathieu let’s see what CI has to say.

@loicmathieu
Copy link
Contributor Author

@machi1990 still failing seems CI related :(

@FroMage
Copy link
Member

FroMage commented Nov 21, 2019

That's what I thought: once it's bust, it's dead. Perhaps open a new PR?

@FroMage
Copy link
Member

FroMage commented Nov 21, 2019

@machi1990 you were on that PR which had the same issues last week, right? How did it get solved there?

@machi1990
Copy link
Member

@machi1990 still failing seems CI related :(

@loicmathieu :-( shame, hope we get this sorted soon.

@machi1990 you were on that PR which had the same issues last week, right? How did it get solved there?

Yeah, I have seen CI related issues in some PR lately and many (including mine) were solved by rebasing on top of master. But there was this #5306 PR that you manually had to connect to Azure and clear the cache or something. @n1hility helped with that.

@geoand
Copy link
Contributor

geoand commented Nov 22, 2019

Rebase onto master is usually the way to go. We need to look into this more as part of #5520

@loicmathieu loicmathieu force-pushed the panache/optionally-find branch from 73d918b to 7201009 Compare November 25, 2019 10:52
@loicmathieu
Copy link
Contributor Author

Rebased again on master, if it still fails I will close it and open a new one ... 🤞

@loicmathieu
Copy link
Contributor Author

@FroMage @machi1990 @geoand it works !!! 🎉

@geoand geoand requested a review from FroMage November 25, 2019 14:44
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

LGTM except minor points.

@loicmathieu loicmathieu force-pushed the panache/optionally-find branch from 7201009 to d5fcfab Compare November 28, 2019 15:11
@loicmathieu loicmathieu force-pushed the panache/optionally-find branch from d5fcfab to e200795 Compare November 29, 2019 10:54
@loicmathieu
Copy link
Contributor Author

@FroMage I think I adress all your points. Can you approve thsi PR ?

@FroMage FroMage merged commit f87febc into quarkusio:master Dec 2, 2019
@FroMage
Copy link
Member

FroMage commented Dec 2, 2019

Done, thanks.

@loicmathieu loicmathieu deleted the panache/optionally-find branch December 2, 2019 14:25
@gsmet gsmet added this to the 1.1.0 milestone Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Returning Optional for single result find Panache operations

5 participants