Skip to content
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.

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

Jump to bottom

Panache reactive based on Hibernate Reactive #10769

Merged
merged 9 commits into from Sep 17, 2020
Merged

Panache reactive based on Hibernate Reactive #10769

merged 9 commits into from Sep 17, 2020

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Jul 16, 2020

Probably a few things left to fix, but at least all the tests pass, so a good candidate for running tests in CI.

@boring-cyborg boring-cyborg bot added area/dependencies Pull requests that update a dependency file area/panache labels Jul 16, 2020
@FroMage
Copy link
Member Author

FroMage commented Jul 16, 2020

Missing docs.

@gsmet gsmet changed the title Panache reactive based on HR Panache reactive based on Hibernate Reactive Jul 16, 2020
@gavinking
Copy link

oh, nice, do you have a link to an example?

@FroMage
Copy link
Member Author

FroMage commented Jul 20, 2020

oh, nice, do you have a link to an example?

quarkus/integration-tests/hibernate-reactive-panache/src/main/java/io/quarkus/it/panache/reactive/TestEndpoint.java

Line 56 in 592c911

return Panache.withTransaction(() -> Person.findAll().list()

But there's so much stuff in these tests that it really doesn't do justice to the API.

@gavinking
Copy link

Right, I was more looking for an example app than tests.

@FroMage
Copy link
Member Author

FroMage commented Jul 20, 2020

I'll add a quickstart too

@FroMage FroMage force-pushed the panache-reactive branch 3 times, most recently from 71f4932 to 66cd30a Compare July 21, 2020 15:33
gsmet
gsmet reviewed Jul 22, 2020
View reviewed changes
...anache/hibernate-reactive-panache/runtime/src/main/resources/META-INF/quarkus-extension.yaml Outdated
- "panache"
- "hibernate"
- "jpa"
guide: "https://quarkus.io/guides/hibernate-orm-panache"
Copy link
Member

Choose a reason for hiding this comment

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

You won't have a specific guide?

Copy link
Member Author

Choose a reason for hiding this comment

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

We will, but there's no guide yet, as is the case for HR.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, better remove the guide than pointing to an inappropriate one.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

@gsmet
Copy link
Member

gsmet commented Jul 22, 2020

I suppose we want that for 1.7. What's the plan for the reviews?

@FroMage
Copy link
Member Author

FroMage commented Jul 22, 2020

Well, CI finally seems to pass, so let's get this out of draft.

@FroMage FroMage marked this pull request as ready for review July 22, 2020 08:47
@gsmet
Copy link
Member

gsmet commented Jul 23, 2020

What I was asking is who will review it?

Personally, I don't know much about Panache and I know nothing about reactive so I won't. Can you find some people who will get that reviewed and merged before CR1?

@Sanne
Copy link
Member

Sanne commented Jul 24, 2020

I can have a look, but "104 files changes" .. gosh I only have an hour I can spare today :)

@Sanne
Copy link
Member

Sanne commented Jul 24, 2020

@DavideD could you have a look as well? If you could focus on checking if the Hibernate Reactive APIs seem used correctly, I'll focus on integration risks.

Sanne
Sanne reviewed Jul 24, 2020
View reviewed changes
...rc/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/AbstractJpaOperations.java Outdated
}

public Uni<Void> persist(Mutiny.Session em, Object entity) {
if (!em.contains(entity)) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this check? And why should it not be persisted otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why is this specific variation of the persist method having this check, but the other methods won't have the same semantics?

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose is to not call persist if it's already persisted. The ORM version has the same check. What happens if I call persist on an already persisted entity, otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

The other variations of persist ultimately call this, so it's fine.

Copy link
Member

Choose a reason for hiding this comment

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

What happens if I call persist on an already persisted entity, otherwise?

Nothing, but at least we have a chance to verify the transaction is alive, extend timeouts, and things like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the javadoc wasn't entirely clear about the semantics here.

Sanne
Sanne reviewed Jul 24, 2020
View reviewed changes
extensions/panache/hibernate-reactive-panache/deployment/pom.xml
</plugins>
</build>

<profiles>
Copy link
Member

Choose a reason for hiding this comment

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

Don't we normally have integration tests in the /integration-testsdirectory, in the root of the Quarkus source tree?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are for the hot-reload. They mirror what we have for ORM with Panache, except that one could run without docker because H2, which we don't support ATM for reactive. I could move them to the existing IT, but that'd be less symmetrical and I still hope we will support H2 for reactive at some point and get rid of this docker requirement.

Sanne
Sanne reviewed Jul 24, 2020
View reviewed changes
...ployment/src/test/java/io/quarkus/hibernate/reactive/panache/test/DuplicateIdEntityTest.java

@Test
void shouldThrow() {
fail("A BuildException should have been thrown due to duplicate entity ID");
Copy link
Member

Choose a reason for hiding this comment

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

So Panache can't deal with composite IDs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC this detects the case where you have multiple @Id annotations but no matching @IdClass which is always a blunder.

@evanchooly
Copy link
Member

simple enough. shouldn't conflict with what I have in motion. fwiw, I'm working on reducing the number of those enhancers/visitors which will reduce the maintenance burden there.

@FroMage
Copy link
Member Author

FroMage commented Sep 1, 2020

Added a new method to guard against future copying of null parameter names, and rebased to resolve HR conflicts.

@FroMage
Copy link
Member Author

FroMage commented Sep 1, 2020

Fixed the damn formatting.

@FroMage
Copy link
Member Author

FroMage commented Sep 1, 2020

All the failures are unrelated:

2020-09-01T11:48:45.8896837Z Resulted in: org.jboss.resteasy.spi.UnhandledException: io.fabric8.kubernetes.client.KubernetesClientException: Failure executing: GET at: http://localhost:40435/api/v1/namespaces/test/pods.

Let's restart.

@stuartwdouglas
Copy link
Member

Removing the dump must have been an oversight.

@FroMage
Copy link
Member Author

FroMage commented Sep 2, 2020

Rebased to get rid of kubernetes test failing.

@jaikiran
Copy link
Member

jaikiran commented Sep 3, 2020

Failure in the IncompletePostTestCase is being tracked at #11839

@FroMage
Panache Reactive
1f6371d
@FroMage
HR: conditionally add the producers
5cec9f2 
To make sure we don't add producers that depend on something that ORM didn't add
@FroMage
junit5-mockito: support mocking Mutiny method default values
6b0a48a
@FroMage
junit5-mockito: Do not mock the proxy classes but rather their superc…
60221f5 
…lass
@FroMage
PanacheMock: made synchronized
f33134c
@FroMage
Fixed AlphaObserver test by making the mocked method public
52fb685 
Mockito doesn't know how to mock package-private methods. This only
works if we're mocking the ClientProxy class instead of the real bean
class.
@FroMage
Added AsmUtilCopy to ArC and use it to create signatures in _ClientPr…
daafa61 
…oxy classes
@FroMage
Make the Kotlin MongoDB with Panache tests pass
a3834ed
@FroMage
Factored all visitParameter into AsmUtil to better avoid missing para…
74d8b71 
…meter names
@FroMage
Copy link
Member Author

FroMage commented Sep 15, 2020

Let's rebase.

@stuartwdouglas stuartwdouglas merged commit 796808c into quarkusio:master Sep 17, 2020
@stuartwdouglas stuartwdouglas added this to the 1.9.0 - master milestone Sep 17, 2020
@FroMage
Copy link
Member Author

FroMage commented Sep 17, 2020

Yay!

@Sanne
Copy link
Member

Sanne commented Sep 17, 2020

Wow! very nice

@loicmathieu
Copy link
Contributor

Cool ! I need to test this !

@jaikiran jaikiran mentioned this pull request Sep 18, 2020
Merged
@gavinking
Copy link

Great!

@yuhaibohotmail
Copy link

@FroMage ,this PR has include in 19.CR1, Could you provide some documents for this PR?

@yrodiere yrodiere mentioned this pull request Sep 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sanne Sanne Sanne left review comments

@cescoffier cescoffier cescoffier left review comments

@DavideD DavideD DavideD left review comments

@gavinking gavinking gavinking left review comments

@gsmet gsmet gsmet left review comments

@loicmathieu loicmathieu loicmathieu approved these changes

@emmanuelbernard emmanuelbernard Awaiting requested review from emmanuelbernard

Assignees
No one assigned
Labels
area/core area/dependencies Pull requests that update a dependency file area/panache
Projects
None yet
Milestone
1.9.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet