quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.57k stars 2.63k forks source link

Simplify projection of a single field when using Panache with Hibernate #28844

Open DavideD opened 1 year ago

DavideD commented 1 year ago

With the current version of Panache with Hibernate, if we want to project a field from an entity, we need to do this:

import io.quarkus.runtime.annotations.RegisterForReflection;

@RegisterForReflection 
public class PersonName {
    public final String name; 

    public PersonName(String name){ 
        this.name = name;
    }
}

// only 'name' will be loaded from the database
PanacheQuery<PersonName> query = Person.find("status", Status.Alive).project(PersonName.class);

It seems a lot of work for returning a single field.

It would be nice ot have a way to specify a field:

PanacheQuery<String> query = Person.find("status", Status.Alive).project("name");

or, maybe,

PanacheQuery<String> query = Person.find("status", Status.Alive).project(Person.attribute("name", String.class));

For comparison, using HIbernate one can do:

session.createQuery("select name from Fruit ...")

This issue applies to both Panache with Hibernate ORM and Panache with Hibernate Reactive

Maybe we should also consider what happens when a user run a query like this:

List<String> distinctNames = Person.find("select distinct p.name from Person p").???.list();

One idea:

List<String> names = Person.distinct().find("status", Status.Alive).project("name", String.class).list()

WORKAROUND: I haven't tested but it seems that this will work:

List<String> names = Person
    .find("select distinct name from Person where ...")
    .project(String.class)
    .list()
quarkus-bot[bot] commented 1 year ago

/cc @FroMage, @loicmathieu

FroMage commented 1 year ago

Can't you already write:

PanacheQuery<String> query = Person.find("select name from Person where status = ?1", Status.Alive);
DavideD commented 1 year ago

No, the signature of .find is

<T extends PanacheEntityBase> PanacheQuery<T> find(String query,  ... )

So it needs to be an entity class

mkouba commented 1 year ago

I once tried to introduce a more flexible PanacheQuery#project() method but was told that it's basically useless. The truth is that for your particular sample the usage would look like query.project("name", row -> row[0].toString()) which is not very ellegant :shrug:

DavideD commented 1 year ago

No, I'm suggesting something different. At the moment, when we use .project(...) underneath we are creating an HQL the looks like (using the person example):

select new PersonName(name) from Person

This seems unnecessary and force us to have a new class just for a field. We should have something that allows the generation of the following HQL instead:

select name from Person

Personally, I don't want to create a new class every time I need a couple of fields from the entity.

The truth is that for your particular sample the usage would look like query.project("name", row -> row[0].toString()) which is not very ellegant

This doesn't work well with Person.findAll().project("name", row -> row[0].toString())). I can't tell where the different fields of Person will be in a row.

I think this would make more sense for SQL queries or more generic queries. But with Panache we query over an entity , so we know that row is actually on object of the type of the entity (Person in the example or PersonName when we use a projection).

mkouba commented 1 year ago

This seems unnecessary and force us to have a new class just for a field.

Yes, I know and I agree.

This doesn't work well with Person.findAll().project("name", row -> row[0].toString())). I can't tell where the different fields of Person will be in a row.

Well, in this API the row was actually not a "full row of an entity" but the row of the selected fields, i.e. the resulting query is something like select name from Person and the the row is an array of length 1 - therefore row[0] maps to the name.

DavideD commented 1 year ago

To be fair Hibernate already handle all these scenarios.

If one select a single field, it will return a list of the type of the field. Otherwise, it will return a List<Object[]>.

I think we should do the same:

List<String> names = Person.findAll().project("name").list();
List<Object[]> custom = Person.findAll().project("name", "status").list();

People can then handle the list the way they prefer.

mkouba commented 1 year ago

List names = Person.findAll().project("name").list();

But how do you derive the type of name?

DavideD commented 1 year ago

I'm not sure yet

DavideD commented 1 year ago

I mean, we can pass a parameter somewhere

FroMage commented 1 year ago

No, the signature of .find is

<T extends PanacheEntityBase> PanacheQuery<T> find(String query,  ... )

So it needs to be an entity class

That signature was a mistake, I've wanted to drop that constraint for a long time. We should really drop it.

Serkan80 commented 1 year ago

It would have been nice if the the all args constructor requirement was not necessary for projections because this way we could directly use the models generated from OpenApi.

loicmathieu commented 1 year ago

@Serkan80 it's a per design requirement as it's a way to select fields from the database and not retrieve all columns.

@FroMage maybe we need an asReturnType() that will replace the parameterized class, this would enable more use cases without broken everything.

I think the signature was not a mistake as it covers most use cases, and Panache purpose was to cover most use cases not all. When you look at the active record pattern, the Person class is responsible for persisting/managing a Person in a database not arbitraty classes.

It's easy to acces the entity manager and load a single field from an entity.

DavideD commented 1 year ago

What's the difference between asReturnType() and project()?

loicmathieu commented 1 year ago

What's the difference between asReturnType() and project()?

List<String> names = Person.find("select name from Person").asReturnType(String.class).list();

asReturnType() changes the parameterized type of the PanacheQuery but not the query itself (where project chage the query to DTO projection query).

FroMage commented 1 year ago

That's OK for PanacheQuery I guess, if a bit too verbose, but in this case we could achieve the same with:

List<String> names = Person.list("select name from Person");
// now I'm even tempted to make the `from Person` part optional ;)
List<String> names = Person.list("select name");

I've never had the time to test this, but I really wonder what it would cause to drop the constraint.

DavideD commented 1 year ago

List names = Person.list("select name from Person");

I like this approach, but having both .list and .find seems unnecessary. At first glance, it seems that changing the signature will give us the flexibility we need to implement this and I don't see any benefit in having <T extends PanacheEntityBase> PanacheQuery<T> find(String) over this <T> PanacheQuery find(String):

  1. If no return type and no select is specified, we can assume it's going to be Person
  2. If projection is needed, one can use .project
  3. If there is a select clause and no project, we could return what hibernate returns (I think it will work in most cases)

Maybe there is a use case I haven't considered?

// now I'm even tempted to make the from Person part optional ;) List names = Person.list("select name");

I guess? In this situation I would prefer to have something like this:

List<String> names = Person.select("name");

Or, if quarkus generates the JPA Static model automatically:

List<String> names = Person.select(Person_.name);
loicmathieu commented 1 year ago

If we remove the type parameter, people will need to make cast or use method type hints. This will not be a good idea (there is already places where type inference didn't work well and it already feels akward).

@DavideD

I like this approach, but having both .list and .find seems unnecessary.

list() is a shorthand for .find().list().

PanacheQuery find(String)

Here you just declare a parameter type without using it. People will get a raw PanacheQuery wich, in most cases, they will cast to PanacheQuery<Entity>. So you will degrade the 80% experience for a niche 20% use case (or less).

Again, I think the best is to allow to change the type parameter and honor the select clause if any. We can also special case the project method for integral wrappers, string and object[].

DavideD commented 1 year ago

If we remove the type parameter,

It's not about removing it, but there's no benefit in extending PanacheEntityBase (I think). Usually, people wants the entity type or something that maps the select clause. Object can also work if for, whatever reason, there's not an appropriate type to map the return value.

Anyway, I'm not particularly interested to this, it's fine by me to call an extra method if it makes me return the type I expect/need.