Integration of QueryDsl library [DATAJPA-8] about spring-data-jpa HOT 13 CLOSED

Timo Westkämper commented

The signature could be something like this

public interface QueryDslSpecificationExecutor<T> {

  List<T> findAll(Predicate... filters);

  List<T> findAll(Predicate filter, OrderSpecifier... order);

  List<T> findAll(OrderSpecifier order);

  List<T> findOne(Predicate... filters);
}

public class QuerydslRepository<T> extends SimpleJpaRepository<T> implements QueryDslSpecificationExecutor<T> {

  public QuerydslRepository(EntityPath<T> entityPath, EntityManager entityManager){
    super(entityPath.getType(), entityManager);
    // ...
  }

}

This covers filtering and ordering.

For example with a domain class User and a query class QUser this could become something like this

QUser user = QUser.user;
QuerydslRepository repository = new QuerydslRepository(user, entityManager);
// active users ordered by last name and first name
List<User> activeUsers = repository.findAll(user.active.eq(true), user.lastName.asc(), user.firstName.as);
// single user
User johnSmith = repository.findOne(user.firstName.eq("John"), user.lastName.eq("Smith"));

from spring-data-jpa.

Oliver Drotbohm commented

I've created a first draft and have pushed the changes here: 98b2a89

Feel free to comment :)

from spring-data-jpa.

Timo Westkämper commented

This looks mostly ok. Here are some comments :

• createPath looks a bit fragile, better take the EntityPath in the constructor

  • The implementation breaks if the Q-prefix is replaced with something else or when the Q-types are generated into some other package.
  • It breaks as well, if the User wants a different variable for root. e.g. new QUser("myUser") or if the uncapitalization creates a different result.

• public T findOne(Predicate predicate) -> public T findOne(Predicate... predicate)

• public List<T> findAll(Predicate predicate) -> public List<T> findAll(Predicate... predicate)

• the path variable in toOrder can be reused, could be an instance variable

from spring-data-jpa.

Oliver Drotbohm commented

Here's the rational for why I've chosen the implementation to be as it currently is:

We need a constructor that only takes the actual domain class as the client should not be aware of other technical dependencies. If used from within our namespace all we have is the repository interface that we can derive the domain class from via reflection. It doesn't seem to be possible to derive a query type from a domain type directly so we have to fallback on applying the conventions to look up the query type. However I will add a constructor taking EntityType<T> as parameter so that one can instantiate new QueryDslJpaRepository(QUser.class, em) although this is probably not the way people mostly will use it.

I am still unsure about the varargs and that's mostly because I don't really get the advantage of simply listing various predicates over explicitly and-concatenating them. Could you given an example where the restriction to a single predicate would cause a hit-the-wall? Especially for findOne(…) i think it's strange from another point of view: as varargs allow handing no Predicate at all this would allow a call to findOne()} (no {{Predicate) which will probably fail almost always except for the case of one entity stored. I'm not aggressively against it, I just don't get the benefit ;).

At toOrder good catch, will apply that.

from spring-data-jpa.

Oliver Drotbohm commented

Here's the updated version: 2d781df

from spring-data-jpa.

Timo Westkämper commented

The constructors look ok now. In most cases the constructor taking the domain class will work as well, in some cases not.

With the explicit and-concatenation you will need to close more parenthesis. I guess in the end it is a matter of taste.

We at Mysema are used to concatenate the filters via varargs :

List<User> users = query.where(
    user.active,
    user.firstName.eq("Foo"),
    user.lastName.eq("Bar"))
  .list(user);

compared to

List<User> users = query.where(
    user.active.and(user.firstName.eq("Foo")).and(user.lastName.eq("Bar")))
  .list(user);

It is just syntactically lighter, no other benefits. But you are right that it doesn't make so much sense for findOne.

One way to make the constructor taking only the domain class safer is to lookup the Q-class and taking the static field with the same type as the class. The Querydsl code generation does some escaping to the default instance if it clashes with a property name.

With this change also the constructor taking the domain class will be fairly safe to use

from spring-data-jpa.

Timo Westkämper commented

The parenthesis counting gets even worth when nesting boolean expressions :

List<User> users = query.where(    
    user.firstName.eq("Foo").or(user.lastName.eq("Bar")),
    user.validUntil.lt(date1).or(user.validUntil.gt(date2)))
  .list(user);

compared to

List<User> users = query.where(  
    .and(user.firstName.eq("Foo").or(user.lastName.eq("Bar")))
    .and(user.validUntil.lt(date1).or(user.validUntil.gt(date2))))
  .list(user);

You will see the differences clearly when you use Querydsl in fluent style instead of creating the predicates via variables.

Sorry for being so persistent on this issue, I just want to be sure you understand the difference

from spring-data-jpa.

Timo Westkämper commented

There was a mistake in my last code snippet. Here is the fixed version

List<User> users = query.where(  
    user.firstName.eq("Foo").or(user.lastName.eq("Bar")))
    .and(user.validUntil.lt(date1).or(user.validUntil.gt(date2))))
  .list(user);

from spring-data-jpa.

Timo Westkämper commented

I looked at the new commits in the Querydsl branch. Here are some comments

• In the loop where you search for the default path entity path you need to take the first static (!) field with the same type

e.g.

public class QUser {

  public final QUser friend;

  public static final QUser user = new QUser("user"); // this one

}

• The Q class for inner classes is <package>.<prefix><outerclass>_<inner class>

e.g.

com.example.Outer.Inner becomes com.example.QOuter_Inner

Otherwise this looks good

from spring-data-jpa.

Oliver Drotbohm commented

Thanks for the hints! Just fixed both issues: 960bf09

from spring-data-jpa.

Stevo Slavić commented

Will this at least preliminary work-in-progress support make it into the spring data jpa v1.0 M2?

from spring-data-jpa.

Oliver Drotbohm commented

It's scheduled for M2, so yes :)

from spring-data-jpa.

Stevo Slavić commented

Asking because there were "rumors" :) it will be available in M1 already

from spring-data-jpa.