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

Use Hibernate Reactive's persistAll in Panache #21840

Merged
merged 1 commit into from
Dec 1, 2021
Merged

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Dec 1, 2021

The one thing I am not sure of is that when using this, we are not checking if the session contains each entity as the simple persist method does (and as the previous - erroneous - implementation implicitly did)

Fixes: #21772

@geoand geoand requested a review from Sanne December 1, 2021 10:09
@geoand
Copy link
Contributor Author

geoand commented Dec 1, 2021

cc @DavideD

Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

In general I would consider it an error to attempt to persist and object which is in the session already; I'm not sure why the existing persist method is lenient about it. @FroMage ?

@geoand
Copy link
Contributor Author

geoand commented Dec 1, 2021

In general I would consider it an error to attempt to persist and object which is in the session already;

I agree, but I thought I would bring it up with you folks to get an expert opinion

@Sanne
Copy link
Member

Sanne commented Dec 1, 2021

I'm wondering also why a "persist(Stream)" is exposed, that might confuse people leading them to have the wrong idea about memory requirements of this operation.

@geoand
Copy link
Contributor Author

geoand commented Dec 1, 2021

I agree on that also. I didn't want to change anything on that front however, since I don't have the context of why it was exposed in the first place

@Sanne
Copy link
Member

Sanne commented Dec 1, 2021

@geoand sure - let's treat those as unrelated changes and wait for @FroMage 's opinion. (This PR looks good as-is)

@Sanne Sanne added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 1, 2021
@geoand geoand merged commit 6b88429 into quarkusio:main Dec 1, 2021
@quarkus-bot quarkus-bot bot added this to the 2.6 - main milestone Dec 1, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 1, 2021
@geoand geoand deleted the #21772 branch December 1, 2021 13:33
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.

Panache reactive shouldn't use Uni.combineAll in a persist
2 participants