Hibernate Reactive's Panache persist methods should return the persisted entity

[gastaldi]

Description
In the PanacheEntityBase from the hibernate-reactive-panache, it would be nice if the persist() and persistAndFlush() returned the persisted entity in the Uni object.

Implementation ideas
Change the method signature from Uni<Void> to Uni<T>

[ghost]

/cc @DavideD, @FroMage, @Sanne, @gavinking, @gsmet, @loicmathieu, @yrodiere

[gastaldi]

Same for the PanacheRepositoryBase

[DavideD]

Can you show some code examples that will benefit from this change?

[gastaldi]

@DavideD sure, here is one example:

    private Uni<Extension> createExtension(io.quarkus.dependencies.Extension ext, PlatformRelease platformRelease) {
        Supplier<Uni<? extends Extension>> createNewExtension = () -> {
            Extension newExtension = new Extension();
            newExtension.groupId = ext.getGroupId();
            newExtension.artifactId = ext.getArtifactId();
            newExtension.name = ext.getName();
            newExtension.description = ext.getDescription();
            return newExtension.persistAndFlush()
                    .onItem().transform(x -> newExtension);
        };
        return Extension.findByGroupIdAndArtifactId(ext.getGroupId(), ext.getArtifactId())
                .onItem().ifNull().switchTo(createNewExtension)
                .onItem().transformToUni(extension ->
                        ExtensionRelease.findByExtensionAndVersion(extension, ext.getVersion())
                                .onItem().ifNull()
                                .switchTo(() -> {
                                    ExtensionRelease extensionRelease = new ExtensionRelease();
                                    extensionRelease.extension = extension;
                                    extensionRelease.version = ext.getVersion();
                                    extensionRelease.metadata = toJsonNode(ext.getMetadata());
                                    extensionRelease.platforms.add(platformRelease);
                                    return extensionRelease.persistAndFlush()
                                            .onItem().transform(x -> extensionRelease);
                                })
                                .onItem()
                                .transform(extensionRelease -> {
                                    // Add release to extension
                                    extension.releases.add(extensionRelease);
                                    return extension.persistAndFlush();
                                }).onItem().transform(x -> extension)
                );
    }

AFAIU this enhancement would get rid of the onItem().transform(...) snippets

[gavinking]

Even after staring at that code for 5 minutes I've no idea what you're trying to do there.

But I speculate that perhaps this issue can be resolved by trying to clean up your use of the Mutiny APIs, making use of the new operations we added to Mutiny. For example, using chain(), call(), and replaceWith() instead of .onItem().transformToUni() for everything.

I might be wrong.

[gastaldi]

Even after staring at that code for 5 minutes I've no idea what you're trying to do there.

Welcome to my world :)

This example is a One-to-Many relationship between Extension and ExtensionRelease (One Extension can contain many ExtensionReleases), so I query for Extension and if it doesn't exist I perform an INSERT. Same with ExtensionRelease and the relationship added in the last transform method.

[DavideD]

I think in Hibernate Reactive one could convert this type of code:

Extension extension = new Extension();
       session.  
         .persist( extension)
         .chain( v -> doSomething(extension))
         .chain( v -> session.flush());

to something like

session
    .persist( new Extension(...) )
    .chain( Type::doSomething )
    .chain( v-> session.flush() )

or this:

Extension extension = new Extension(...) 
return session
    .withTransaction( tx -> 
        session
            .persist(  extension )
            .chain( v -> doSomethingInTx( extension )
            .invoke( v -> extension )
    );

to this:

return session
    .withTransaction( tx -> 
        session
           .persist(  new Extension(...)  )
           .chain( Type::doSomethingInTx)
    );

[DavideD]

Actually, the doSomethingInTx doesn't make much difference.

This is a simpler use case for Hibernate Reactive:

Extension extension = new Extension(...) 
return session
    .withTransaction( tx -> 
        session
            .persist(  extension )
            .invoke( v -> extension )
    );

to this:

return session
    .withTransaction( tx -> 
        session
           .persist(  new Extension(...)  )
    );

[gavinking]

@DavideD

Your third example is a case for replaceWith(), not invoke(), I believe.

(It's one of the last things we requested, remember.)

[gavinking]

For example:

Extension extension = new Extension(...) 
return session
    .withTransaction( tx -> 
        session
            .persist( extension )
            .replaceWIth( extension )
    );

[DavideD]

I see, thanks.

You still need the additional .replaceWith and an instance of the extension beforehand though.

By the way, I don't have strong opinions either way, but I think it helps to write down some examples as part of the issue.

[gavinking]

Also @DavideD:

1. We don't use the v -> idiom anymore, because it's bad. Use () -> instead.
2. For calling flush() I think it's better style to use call() than chain().

[gavinking]

You still need the additional .replaceWith and an instance of the extension beforehand though.

Well I think that's fine personally. I mean, what's actually wrong with:

return session.withTransaction( tx -> {
    Extension extension = new Extension(...);
    return session.persist(extension)
        .replaceWith(extension);
} );

This compares perfectly favorably to the equivalent non-reactive code:

return session.withTransaction( tx -> {
    Extension extension = new Extension(...);
    session.persist(extension);
    return extension;
} );

[DavideD]

Just one last point for completeness.
What about the Hibernate Reactive Stage.Session API? A user won't have the nice Mutiny methods in that case.

[DavideD]

What about the Hibernate Reactive Stage.Session API? A user won't have the nice Mutiny methods in that case.

I suppose that's the way it is.
Thanks @gavinking

[FroMage]

This is a RFE for Quarkus's "HR with Panache", so you don't need to change the HR API, this is about the Panache layer.

Originally I made persist return Uni<Void> because the blocking equivalent returns void, but it turns out that it doesn't hurt and leads to more idiomatic reactive pipelines to make it return the item you want to persist, by making it part of the pipeline rather than require a local variable to be declared, which is always a bit tricky with reactive pipelines.

I think it's an acceptable enhancement and doesn't cost anything. It's not like Uni<Void> is more useful than Uni<T> either.

[gavinking]

2. For calling flush() I think it's better style to use call() than chain().

Excuse me, I should say the proper style is too use call() for any void-like method, including persist(), remove(), etc.

[gavinking]

What about the Hibernate Reactive Stage.Session API? A user won't have the nice Mutiny methods in that case.

Well, here's where I document the mapping:

http://hibernate.org/reactive/documentation/1.0/reference/html_single/#_apis_for_chaining_reactive_operations

For sure it's not quite as nice on the Stage API, but it seems like we are currently pushing people toward Mutiny anyway.

[loicmathieu]

@FroMage there is another request for this on #16921 (I closed it as duplicate).

So, do we switch the return type (this is binary compatible but not source compatibe) to Unit<T> ?

[FroMage]

Yes, please.

[edeandrea]

What about the non-reactive flavor of panache which currently returns void? Shouldn't the behavior there be similar?

Also

public Fruit addFruit(@Valid Fruit fruit) {
    return fruitRepository.persist(fruit);
}

Is much more elegant and simple than

public Fruit addFruit(@Valid Fruit fruit) {
    fruitRepository.persist(fruit);
    return fruit;
}

[gastaldi]

@edeandrea agreed, that could be changed too

[edeandrea]

@gastaldi would you like me to open a separate issue for it?

[loicmathieu]

@gastaldi @edeandrea I'm not against it but I'm pretty sure we already discuss this and dismissed it.
I would like to hear what's @FroMage think about this one.

[gastaldi]

@gastaldi would you like me to open a separate issue for it?

I'm curious to know what @FroMage thinks about it first. I recall seeing some discussions around it as @loicmathieu mentioned

[edeandrea]

I'm curious to know what @FroMage thinks about it first. I recall seeing some discussions around it as @loicmathieu mentioned

I don't think he's a fan :)

[FroMage]

Well, I don't think it's very useful for the non-reactive cases, because we don't usually chain the calls, and this example is really trivial and not common in real code. But it wouldn't hurt I guess.

[MartinX3]

Is it possible to backport it to 1.13.x? :)

[loicmathieu]

Is it possible to backport it to 1.13.x? :)

I'd rather not, this is already a backport to 2.x as it is already branched, and it can potentially break some code due to method generic return type change so we cannot add it in a micro version.

[edeandrea]

@FroMage / @loicmathieu / @gsmet / @gastaldi after updating to 2.0.0.CR3 I noticed that the method signature on PanacheRepositoryBase within quarkus-hibernate-reactive-panache changed to public default Uni<Entity> persist(Entity entity) but in PanacheEntityBase the public static Uni<Void> persist(Object firstEntity, Object... entities) method (and the other persist variants) still return Uni<Void>.

Is this by design? Why do they not return Uni<T> like they do in PanacheRepositoryBase? It seems only the (non-static) persist() method returns Uni<T>.

This means if I want to showcase the same use case in both the repository & active record patterns, my code now needs to be different.

Active record

@POST
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public Uni<Fruit> addFruit(@Valid Fruit fruit) {
	return Panache.withTransaction(() ->
		Fruit.persist(fruit)
			.replaceWith(fruit)
	);
}

Repository

@POST
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public Uni<Fruit> addFruit(@Valid Fruit fruit) {
	return Panache.withTransaction(() -> this.fruitRepository.persist(fruit));
}

Furthermore, I am not able to properly mock the non-static persist() method on PanacheEntityBase.

[loicmathieu]

Entity and repository should offers the same API and they do see https://github.com/quarkusio/quarkus/blob/main/extensions/panache/hibernate-reactive-panache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/PanacheEntityBase.java#L55 and https://github.com/quarkusio/quarkus/blob/main/extensions/panache/hibernate-reactive-panache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/PanacheRepositoryBase.java#L52

Now the persist method that takes a varargs still returns Uni, we could have return the array of persisted entity but we didn't choose to do so as use cases are less attractive for the various persist variant.

The two exemples you show should works with CR3.

[edeandrea]

What I'm saying is my examples should look like the following (i.e. I shouldn't have to chain .replaceWith on the return of .persist(fruit), but it doesn't.

Active record

@POST
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public Uni<Fruit> addFruit(@Valid Fruit fruit) {
	return Panache.withTransaction(() -> Fruit.persist(fruit));
}

Repository

@POST
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public Uni<Fruit> addFruit(@Valid Fruit fruit) {
	return Panache.withTransaction(() -> this.fruitRepository.persist(fruit));
}

I can't use the non-static persist() method because I am unable to mock it in my tests using PanacheMock. PanacheMock can only mock the static methods on PanacheEntityBase and Mockito can not mock the panache methods.

PanacheMock.doReturn(Uni.createFrom().item(new Fruit(1L, "Grapefruit", "Summer fruit")))
  .when(Fruit.class).persist();

does not work and the mocking does not happen.

[loicmathieu]

@edeandrea what you describe as nothing to do with the changes I made, this is a mocking issue, please open an issue with a reproducer but Mockito should be able to mock your method, or PanacheMock, one of the two.

[edeandrea]

Mockito can not mock it because the mock is a class, not an instance (i.e. you can't do Mockito.when(Fruit.class)... nor Mockito.doReturn(...).when(Fruit.class)...). I submitted #17697 for the mocking issue.

All I'm trying to say is that the behavior of

fruitRepository.persist(fruit) (returns Uni<Fruit>) is now different than Fruit.persist(fruit) (returns Uni<Void>)

[loicmathieu]

No Fruit.persist(fruit) returns Uni<Fruit> , you can look at https://github.com/quarkusio/quarkus/blob/main/extensions/panache/hibernate-reactive-panache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/PanacheEntityBase.java#L55

[edeandrea]

That reference you made is not to the correct method. That reference would be for fruit.persist() (non-static & no input argument), not Fruit.persist(fruit) (https://github.com/quarkusio/quarkus/blob/main/extensions/panache/hibernate-reactive-panache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/PanacheEntityBase.java#L744)

Fruit.persist(fruit) returns Uni<Void>

[loicmathieu]

OK, you mis-use the API, if you want to persist a single entity you should use the instance method not the static one. As I already said, the static one taken a varags cannot return a Uni<Fruit> as it takes a varags, so an array of Fruit.

So the correct usage should be

@POST
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public Uni<Fruit> addFruit(@Valid Fruit fruit) {
	return Panache.withTransaction(() -> fruit.persist()));
}

[edeandrea]

Ok thanks. Then my ticket (#17697) is blocking me from doing so and forcing me to use Fruit.persist(fruit) :)