Package changes #1039

Closed
timowest opened this Issue Nov 12, 2014 · 17 comments

Projects

None yet

4 participants

@timowest
Member

Issues

https://github.com/querydsl/querydsl/milestones/4.0.0

Modules

  • querydsl-apt (com.querydsl.apt)
  • querydsl-codegen (com.querydsl.codegen)
  • querydsl-collections (com.querydsl.collections)
  • querydsl-core (com.querydsl.core)
  • querydsl-hibernate-search (com.querydsl.hibernate.search)
  • querydsl-jdo (com.querydsl.jdo)
  • querydsl-jpa (com.querydsl.jpa)
  • querydsl-jpa-codegen (com.querydsl.jpa.codegen)
  • querydsl-lucene3 (com.querydsl.lucene3)
  • querydsl-lucene4 (com.querydsl.lucene4)
  • querydsl-maven-plugin (com.querydsl.maven)
  • querydsl-mongodb (com.querydsl.mongodb)
  • querydsl-scala (com.querydsl.scala)
  • querydsl-spatial (com.querydsl.spatial)
  • querydsl-sql (com.querydsl.sql)
  • querydsl-sql-codegen (com.querydsl.sql.codegen)
  • querydsl-sql-spatial (com.querydsl.sql.spatial)
  • querydsl-sql-spring (com.querydsl.sql.spring)

Contrib modules

  • querydsl-dynamodb (com.querydsl.dynamodb)
  • querydsl-hazelcast (com.querydsl.hazelcast)
@timowest
Member

Idea

How about separating the projection from the query execution

e.g.

query.from(customer).where(customer.firstName.eq("Tom")).list(customer)

into

query.select(customer).from(customer).where(customer.firstName.eq("Tom")).list()

This has the following benefits

  • queries can be written both with select definition in the beginning as well as the end
  • subqueries can be defined via normal Query instances
  • Projectable and SimpleProjectable can be merged into the same class

Disadvantages

  • more verbose syntax in some cases
  • generic parameter in Query interface/class
@timowest timowest added this to the 4.0.0 milestone Nov 12, 2014
@Shredder121
Member

Querydsl-contrib and it's subprojects also needs to be updated in parallel.
That way we don't have package mismatches/confusion.

How does that sound?

@timowest
Member

Yes, true. Good point.

@Shredder121
Member

I don't think that separating the projection is a good call in the way I think you mean (please correct me if I am wrong)

What we do in our projects(with JPA), is before calling .list(), run a .count() to populate pagination.
This means we would not specify a projection until the last moment (as it is now)

Do you mean that we keep the old style

query.from(customer).where(...)
query.count()
query.exists()
query.list(...)
//etc...

and mix in the new style?

query.select(customer).from(customer).where(...).list()

How would you know when to use the new and old style?
I think when you started out in the old style, you might get something like this:

//You start out the way you used to
query.from(customer)

//You will find the select method
query.from(customer).select(customer)

//And then you are confused, since there is no result yet
query.from(customer).select(customer).list(customer)

Then Querydsl won't quite know what to do with the two projections.

My proposal is, since I do like the idea of separating the projection, to include some sort of FluentQuery, looking at Guava's FluentIterable and our CaseBuilder.
This way we keep the original Querying API, which is already familiar.
It might be some more work getting it right, but I think it does away with the disadvantages you mention:

more verbose syntax in some cases

Since the query is still built in the same way we all know and love(and is closer to the original equivalent), we can make the queries more compelling and pleasing to look at.

queryBuilder.select(customer.name)
    .from(customer)
    .where(customer.id.in(
        queryBuilder.select(customer.id)//<-here we are already dealing with a compile time check, and we could make it so that in() subquery serializes to list, and all other serialize to singleresult (so first() or single() as it's called)
        .from(customer)
   ).list()//<- here the user wants the result, while in the one above we serialize it for the in query

generic parameter in Query interface/class

Yes, this is unavoidable, but, if we separate the query, and the querybuilder, we could make the querybuilder generic (so better compile time validation, since at least for Netbeans generic classes are easier to autocomplete than generic methods), because of the way type parameters are substituted.

queryBuilder.select(customer.name)//<-single expression projection typed to String
         .from(customer)
         .list()//<-return type is known, since the type parameter was already known from the generic type

//versus the old
query.from(customer)//<-plain query
         .list(customer)//<-still a plain query, so Netbeans thinks you could type singleresult to a list, because its completely generic return type

Just as your post, it is just an idea, and of course up for discussion, but just to recap my point, mixing old and new query API's (in the same query class) is not a good idea I think.

@Shredder121
Member

Just adding: if you meant we are not keeping the old Querying API, then we could make the new way much better, and we don't need to have a querybuilder.
(I was busy writing this reply over the day, rereading my points, but now I think I definitely misunderstood you)

Generic classes are always better, the only thing I am concerned about is keeping the (right) generic state across method calls, but we'll see about that.
I think we then also need to make a test suite that emulates the different use cases(projection-first, projection-last) to see if it works.

@timowest
Member

I didn't intend to mix the old and new API. But having a single query type would be a simpler choice. Single Query instances are mutable and builders it would also be difficult to come up with a conceptual separation between the old and new API.

A pattern I came up with was

private  <T> JPAQuery<T> from(EntityPath<T> entity) {
    return new JPAQuery<T>(em).from(entity).select(entity);
}

which could be used as

from(customer).where(customer.name.eq("Bob")).list();

or to override the projection

from(customer).where(customer.name.eq("Bob")).select(customer.name).list();

One problem I see with the generic type parameter is that it is misleading when the projection is not yet defined.

@Shredder121
Member
String query = JPAQuery.create(em)//<-This is a difficult one indeed, but I think that static factory methods help in the "I don't (yet) care about the type of the query"
    .from(customer).where(customer.firstName.eq("Bob"))//<- these methods shouldn't be generic, since sources and predicates are independent of the projection
    .select(customer.lastName)//<-I suggest that select resets the generic type to the new projection, where multiple projections reset it to Tuple.
    .single();

This is my first take on the new way. It is indeed misleading to have to declare a generic parameter, although we could fix this with a static factory method.
In my opinion, static factories tell me: "Don't worry too much about the creation details, leave that to me", so I think that is what we are trying to tell people with the (initial) generic parameter.
The generic query returned for JPA could be a JPAQuery<?>, indicating the projection is still unknown. The only problem is that people shouldn't store the initial query, as in:

JPAQuery.create(em)
//extract local variable

JPAQuery<?> query = JPAQuery.create(em)
//querying
query = query.select(customer)//<-generic result lost..
query = query.from(customer)
//etc....

But in return, filters could be applied in a typesafe way (or on any Query<?> for that matter, just a matter of preference)

JPAQuery<Customer> query = JPAQuery.create(em).select(customer).from(customer);
return filterQuery(query).list();

But as i said, we should add a test class so that we can try the syntax for ourselves and see if it works out. (also usable as showcase for end users, to show how you can query)

@timowest
Member

Good point for the syntax demo. I can prepare one this weekend.

@Shredder121
Member

I also tried some of the new syntax.
I have to say that it feels quite nice to have the select type captured while querying.

@timowest
Member

There are some remaining issues. Some methods are both applicable to both queries and subqueries, e.g. exists and notExists. In my example I made it available for subqueries.

@tiarebalbi

I have followed this discussion and i would like to check if has been considered include another operations like delete, update and insert into the Query Builder.
Could be a good option if we think in a fluent query builder. I'm putting this topic to be discussed.

@Shredder121
Member

On second thought is a querybuilder not that easy. I presumed the new way and the old way would coexist and thought we must seperate it in some way. But this is not the case.

If you want to create DMLClauses, you can use a QueryFactory.

@Tibor17
Tibor17 commented Nov 26, 2014

Hi all, pls see this #1053
I would be glad if you could make it like in #1053.

@Shredder121
Member

As I explained in the comment on your issue, I also though of using static factory methods, but the problem is that with static factory methods, you miss some context as in how the query must be built.
I saw only later that you wanted to provide the EntityManager in the list() method, but then we can't do validation early enough.

I think that keeping the templates final is a better validation approach than validating it when it is set.

@Tibor17
Tibor17 commented Nov 26, 2014

@Shredder121
If QueryDSL needs entity manager early, then static methods create(em).from(...) would maybe do it for us. Perhaps from(em, Entity.class) would be enough.

This was referenced Jan 12, 2015
@timowest timowest removed the postponed label Jan 15, 2015
@timowest timowest changed the title from Querydsl 4.0.0 changes to Querydsl 4.0.0 package changes Jan 15, 2015
@timowest timowest changed the title from Querydsl 4.0.0 package changes to Package changes Jan 15, 2015
@timowest
Member

Extracted query / projection separation into #1120

@timowest timowest closed this Jan 22, 2015
@timowest timowest removed the progress label Mar 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment