spring-projects / spring-data-commons

Spring Data Commons. Interfaces and code shared between the various datastore specific implementations.
https://spring.io/projects/spring-data
Apache License 2.0
778 stars 675 forks source link

Add support for operators on auto-extracted Querydsl predicates [DATACMNS-846] #1308

Closed spring-projects-issues closed 5 years ago

spring-projects-issues commented 8 years ago

Jan Pavtel opened DATACMNS-846 and commented

Add support for all Querydsl operators. Suppose we have entity with fields

class Person {
  long age;
  String name;
}

we would like to filter persons by age

/persons/?age.lt=33

@RequestMapping(value = "/persons/", method = GET)
public Page<Person> getPersons(@RequestParam(value = "pageNumber", defaultValue = "0") int pageNumber,
    @RequestParam(value = "pageSize", defaultValue = "20") int pageSize,
    @RequestParam(value = "sorted", defaultValue = "", required = false) String sorted,
    @QuerydslPredicate(root = Asset.class) Predicate predicate) throws Exception {

    PageRequest pageRequest = new PageRequest(pageNumber, pageSize, sortInterpreter.process(sorted));
    return personRepository.findAll(predicate, pageRequest);
}

No further details from DATACMNS-846

spring-projects-issues commented 8 years ago

Oliver Drotbohm commented

Unless we find very good reasons — and I have a hard time coming up with one — I don't think this is going to be added. There's two major reasons for that.

  1. It potentially creates ambiguities with nested properties. With every keyword we could basically shadow a property on the model in the first place.
  2. It requires much more knowledge about the query API in the client. Clients effectively have to become aware of Querydsl capabilities, supported keywords etc. That tightly couples the client to the server implementation.

With the QuerydslBinderCustomizer callback, we already have an API in place to customize the way values are bound to predicates. So clients only have to know about the fields the documents expose (which they have to have anyway). I think this is a good middle ground between the rather static exposure of query methods and the more flexible needs of clients to express dynamic filters

spring-projects-issues commented 8 years ago

Jan Pavtel commented

Thank you for quick answer. We already using

QuerydslBinderCustomizer

but I thought more powerful solution would be better. Ad 1) we can add property enableQueryDslOperators (default false) into QuerydslPredicate annotation :

@QuerydslPredicate(root=Person.class, enableQueryDslOperators=true) Predicate predicate
spring-projects-issues commented 7 years ago

Jens Schauder commented

??I thought more powerful solution would be better??

Not really. "More powerful" most of the time also mean: More complex, more chances of conflicts with other features. That's why we are conservative with the features we add. And also often prefer to offer a way to users to build their own solution.

In this case, you can provide your own conversions, right?

Marking this issue as not likely. Going to close it if no new arguments in favor of it are brought forward

spring-projects-issues commented 5 years ago

Dominic Bach commented

Although I disagree with the approach to solving the problem (adding search operators as sub-properties) and think that LDAP-esque operators might be more sensible (not to mention more powerful as they could potentially allow nesting), I figured I'd respond to Jens Schauder's point concerning leaving implementation to the user.

Unfortunately the QuerydslPredicateBuilder attempts to coerce the search arguments prior to invoking the bindings for them. As a result it is not possible to define any sort of custom operator-parsing mechanism on the argument using QuerydslBinderCustomizer (except for String types). This means search filters for types such as dates can only have a single static operator defined on them while it would be more useful to define multiple operators such as greater than and lower than

spring-projects-issues commented 5 years ago

Oliver Drotbohm commented

The PathBinder interface returned by QuerydslBindings exposes an ….all(…) method, that allows you to consume all values provided for a given parameter. I.e. something like ?age=25&age=35 can be consumed that way and used to construct between expressions.

I'll close this as won't fix as – as indicated in my first comment – we don't have plans to invent a request parameter query language

spring-projects-issues commented 5 years ago

Dominic Bach commented

Oliver Drotbohm

When you say "we don't have plans to invent a request parameter query language", do you mean that it doesn't fit the vision for Spring Data Commons and if I were to create a pull request with an implementation it wouldn't be looked at, or could I propose an implementation for said functionality?

I've already created an implementation of a query capabilities for Querydsl in a project that allows me to do something like

 

/persons?age=or(between(18,30),in(14,16))

however due to the visibility level of classes used by QuerydslPredicateBuilder and the fact that it isn't a bean, I had to extend multiple classes and make reflective calls to private methods, which isn't great.

If you're open to reviewing my proposition I'd love to be able to share.

 

 

spring-projects-issues commented 5 years ago

Oliver Drotbohm commented

Yes, it's exactly what we don't want to do as we're not in the business of specifying a request parameter API on the one hand, and at least 15 different flavors of something like that already existing in the wild. Maybe Petar Tahchiev can comment as I know they built something similar