querydsl / querydsl

Unified Queries for Java
https://querydsl.com
Apache License 2.0
4.71k stars 869 forks source link

request for change "IllegalArgumentException: eq(null) is not allowed. Use isNull() instead" #25

Closed AndreTheHunter closed 12 years ago

AndreTheHunter commented 12 years ago

When using com.mysema.query.types.expr.SimpleExpression.eq(Expression<? super T> right) where right is set to null an IllegalArgumentException is thrown:

java.lang.IllegalArgumentException: eq(null) is not allowed. Use isNull() instead

This is by design (see https://bugs.launchpad.net/querydsl/+bug/720078) however it would be better to check if right is null and use isNull() accordingly. e.g.

public BooleanExpression eq(T right) {
    return right == null ? isNull() : eq(new ConstantImpl<T>(right));
}

see SimpleExpression.java:124

At the moment I have to do this null checking in my code every time I want to use .eq(...). This is cumbersome which defeats the purpose of Querydsl's fluent design.

timowest commented 12 years ago

We decided to keep it like on purpose for more explicit query construction. Querydsl mimics SQL in many ways and keep null checks separate from value equality checks is one of those features.

Do you use Querydsl mainly for explicit query construction or something more generic?

AndreTheHunter commented 12 years ago

I use Querydsl for explicit query construction for JPA as part of the Spring Data project. The DAOs perform queries based on method parameters where null is often a legitimate value. Example:

public Slice getSlice(final Core core, final String sliceCd) {
    return repo.findOne(byCoreAndCd(core, sliceCd));
}

public boolean exists(final Core core, final String sliceCd) {
    return repo.exists(byCoreAndCd(core, sliceCd));
}

private Predicate byCoreAndCd(final Core core, final String sliceCd) {
    final QSlice slice = QSlice.slice;
    return eq(slice.core, core).and(eq(slice.cd, sliceCd));
}

protected static <T> BooleanExpression eq(final SimpleExpression<T> path, final T right) {
  return right == null ? path.isNull() : path.eq(right);
}

In this example a sliceCd can quite legitimately be null and should be matched as such. Having to use the eq method "breaks" some of the fluent nature of Querydsl.

I can understand if this can't be changed for consistency's sake, otherwise please provide some mechanism to extend the Querydsl annotation processor (I'm sorry if there is already such a feature that I am unaware of).

timowest commented 12 years ago

You can achieve an extension like this : http://source.mysema.com/static/querydsl/2.2.3/reference/html/ch03s02.html#d0e1403

AndreTheHunter commented 12 years ago

Thanks, I'll have a look. Unfortunately http://source.mysema.com/ has been down all day...

timowest commented 12 years ago

Sorry, there were some DNS issues. These have been fixed now.

timowest commented 12 years ago

Andre, did you have a chance to try out the query extensions?

AndreTheHunter commented 12 years ago

I could not get it working for the ideal solution. Two solutions I found was to:

1) Create a @QueryDelegate for each entity type is very verbose and not very flexible between projects

2) Create a @QueryDelegate for Object then use

new QObject(QSlice.slice.cd.getMetadata()).eq2(Object)

does not work for entity paths and is not type safe

I think the ideal solution would be to configure the annotation processor to allow the QTypes to be extended from another base class.

@Generated("com.mysema.query.codegen.EntitySerializer")
public class QSlice extends EasyEntityPathBase<Slice> {
...
}

Where EasyEntityPathBase would then override the eq(...) method.

I'll continue to investigate a solution. I'm happy to close this issue and continue the discussion on the Google Group (https://groups.google.com/forum/#!forum/querydsl)