Specifications applies conditions in the wrong order [DATAJPA-959]

[spring-projects-issues]

Triqui Galletas opened DATAJPA-959 and commented

This may sound stupid but I think the method Specifications.and(final Specification<T> other) should be written a bit differently.
These two lines should be swapped:

Predicate otherPredicate = other == null ? null : other.toPredicate(root, query, builder);
Predicate thisPredicate = spec == null ? null : spec.toPredicate(root, query, builder);

The call to other.toPredicate should happen after the call to spec.toPredicate.
I'm not sure if there is a better way, but I use to have query.distinct(), query.join() and query.fetch() calls inside the toPredicate methods in some specifications, and in some cases I need to know if a join, for instance, has been already applied or not. The only way to know is if the toPredicate methods for every specification is called in the same order they were created.

For example I build a dynamic specification where I have this:

public class mandatorySpecification {
    toPredicate(...) {
        Fetch child = root.fetch("child", JoinType.LEFT);
        return cb.equal(child.get("width"), 1024);
    }
}

And then I have some 'ifs' where I add more specifications that need to reference a property from that join.

public class optionalSpecification {
    toPredicate(...) {
        Fetch child = getFetchOrJoin(root, "child");
        return cb.equal(child.get("height"), 768);
    }
}

My method getFetchOrJoin uses root.getJoins() and root.getFetches() to retrieve the already existing join, but it doesn't work if I add the first specification first and the second second. It only works if I add the second specification first and the first second.

This doesn't work:

spec.and(mandatorySpecification());
if (something) {
    spec.and(optionalSpecification());
}

This works:

if (something) {
    spec.and(optionalSpecification());
}
spec.and(mandatorySpecification());

---

Affects: 1.10.1 (Hopper SR1)

[spring-projects-issues]

Oliver Drotbohm commented

A Specification has to be implemented in a way that the commutativity of the AND operation is preserved. So if the order of the application matters, that's a bug in the Specification implementation. In your particular example, I'd suggest to guard the join creation of your first Specification the same way, your second one does

[spring-projects-issues]

Triqui Galletas commented

The problem is not when using the AND to add a condition, it's when using it to modify the query (adding a join or a fetch).
Also the first Specification is ok, but the second one needs to get the join created in the first one to apply the condition, but the join does not exists yet. The call to getFetchOrJoin in the second Specification returns null, case the first one hasn't been applied yet.
In my example, the first Specification decides and knows which join to apply (join or fetch, inner or left). The second Specification needs to retrieve to join in order to get the Path to the attribute for which a condition is going to be applied. Since the join doesn't exist yet, the second Specification will never work.
This is what happens when the code is executed:

Fetch child = getFetchOrJoin(root, "child"); // this throws an exception when the join is not found
cb.equal(child.get("height"), 768);  // otherwise it would throw a NulPointerException here
child = root.fetch("child", JoinType.LEFT); // These two lines should be executed before the lines above
cb.equal(child.get("width"), 1024); // only way to do it is move the specifications with join/fetch after any other specification so they are executed before.

And I think

[spring-projects-issues]

Triqui Galletas commented

I added a comment some days ago. I don't know whether that's enough or I have to use the feedback button (I just saw it today). So I write this just in case. Please, ignore this if it wasn't needed

[spring-projects-issues]

Oliver Drotbohm commented

I think I understood your scenario but that doesn't change anything regarding the requirement for commutativity. I can't actually follow your "The problem is not when using the AND to add a condition…". That's what the ticket is about, isn't it? That the order of which specification is added to which changes the result, right? If you're using a canonical specification only, the problem can't really appear.

So again. You need to implement your specifications in a commutative way

[spring-projects-issues]

Triqui Galletas commented

I understand your point, and I agree with you. But there are two things I think you are not considering properly (please, take no offence, my english is not so fluent and it might sound rude even though that's not my intention).

First, if you need to add joins and some other stuff which does not "return" a predicate, the only way I know to do it is like this:

Specifications<Object> spec = Specifications.where(new Specification<Object>() {
	@Override
	public Predicate toPredicate(Root<Object> root, CriteriaQuery<?> query, CriteriaBuilder cb) {
		query.distinct(true);
		root.join("test", JoinType.LEFT);
		return null;
	}
});

Maybe there is a better way and that would solve all my problems.

• This I think is the problem: root.join and query.distinct, for instance, modify the query and won't return a predicate, they probably should be outside the toPredicate() method, but there is no other place to put them. Conditions should be commutative, that's right; but a join, a distinct and some other stuff are not conditions and it's impossible to use them and keep the commutativity of the AND operation.

Second. If you call root.getJoins() from inside toPredicate() it will never work unless any specification with a call to root.join() is added after it.
This is what I was saying just before. If you use a call to join inside an specification it won't be commutative anymore, the result will depend of when you add it to your Specifications. It will probably work most of the time anyway, but there are some cases which won't work unless the right order is used.

I've tried to put an example as short as possible. Still it's a bit long, but please have a look at it and let me know what would be the right way to write it (I can't just put everything into one specification because they come from different places):

public Specification<Object> b(final Integer status) {
	Specifications<Object> spec = Specifications.where(new Specification<Object>() {
		@Override
		public Predicate toPredicate(Root<Object> root, CriteriaQuery<?> query, CriteriaBuilder cb) {
			query.distinct(true);
			root.join("test", JoinType.LEFT);
			return null;
		}
	});
	if (status != null) {
		spec = spec.and(new Specification<Object>() {
			@Override
			public Predicate toPredicate(Root<Object> root, CriteriaQuery<?> query, CriteriaBuilder cb) {
				Path<?> child = root.get("test");
				Bindable<?> model = child.getModel();
				if (model == null || model.getBindableType().equals(BindableType.PLURAL_ATTRIBUTE)) {
					for (Join<?, ?> join : root.getJoins()) {
						if (join.getAttribute().getName().equals("test")) {
							child = join;
							break;
						}
					}
				}
				return cb.equal(child.get("status"), status);
			}
		});
	}
	return spec;
}

Please, note that this won't work. But if you add the two specifications in reversed order it works perfectly.

(Maybe I'm insisting too much and wasting too much of your time. As I said, the workaround is easy, so just let me know if I should drop the matter)

[spring-projects-issues]

Jens Schauder commented

Specifications have to be commutative.

In a case like this it might be able to check the root if a join is already present and if not create it, if it is, use it. If that doesn't work you have still the option to write your own logic to combine Specifications as you desire

[pgv]

Thankfully. I finally updated my old project to a new version and now I see that someone smarter fixed this in 2146.
Maybe you should change the resolution to 'fixed'