jakartaee / data

Data-API
Apache License 2.0
101 stars 27 forks source link

Parameter-based Automatic Queries as a full alternative to Query by Method Name #857

Open njr-11 opened 3 weeks ago

njr-11 commented 3 weeks ago

The parameter-based Automatic Queries pattern already provides an alternative to much of what Query by Method Name offers for static queries, but some parts are lacking which we should address. Applications ought to be able to choose to fully utilize this pattern and not need to rely on Query by Method Name in order to have a solution for static queries.

The most notable piece that is lacking is the ability to request conditions other than equality. One way to address that would be with a single Is annotation that accepts an enumerated value for the condition to apply. For example,

@Find
@OrderBy(_Product.PRICE)
@OrderBy(_Product.NAME)
@OrderBy(_Product.ID)
Page<Product> search(@By(_Product.NAME) @Is(Like) String pattern,
                     @By(_Product.PRICE) @Is(LessThan) float max,
                     PageRequest pageRequest);

The above directly corresponds to the following Query by Method Name method that could currently be used:

Page<Product> findByNameLikeAndPriceLessThanOrderByPriceAscNameAscIdAsc(String pattern,
                                                                        float max,
                                                                        PageRequest pageRequest);

Note that everything that is needed for the former is already in the Jakarta Data specification except for the mechanism for specifying the condition to be something other than equals. So the enhancement here has a direct equivalence to the Query by Method Name and achieves it with a minor addition that fits right in to an existing specification pattern.

gavinking commented 2 days ago

I definitely 100% understand where this proposal is coming from, and I want to emphasize that I don't hate it, but, after reflection, my preference is still for #546.

@Find
@OrderBy("price, name, id")
Page<Product> search(@By("name like ?1") String pattern,
                     @By("price < ?2") float max,
                     PageRequest pageRequest);

This option is:

  1. significantly less verbose, and easier on the eyes,
  2. much more flexible (it can be easily extended to accommodate more things), and
  3. can be made completely typesafe.

Furthermore:

We know for a fact that we're going to be able to count on IDE support for this stuff (the IntelliJ folks have been super responsive here).

gavinking commented 2 days ago

it can be easily extended to accommodate more things

For example, we get this for free:

@Find
Thing thing(@By("lower(code) like lower(?1)") String code`)

Whereas with @Is, I'm really not sure how you would express it.

beikov commented 1 day ago

Just to throw another idea into the pit, how about we introduce special value types per predicate instead of annotations?

@Find
Page<Product> search(@By(_Product.NAME) LikePattern namePattern,
                     @By(_Product.PRICE) ComparisonValue<Float> maxPrice,
                     PageRequest pageRequest);

That would also allow some runtime flexibility e.g. use a different comparison operator without having to duplicate the whole method.

graemerocher commented 1 day ago

I agree with @gavinking introducing overly complex annotation-based DSLs doesn't seem to be a wise direction

gavinking commented 1 day ago

Just to throw another idea into the pit, how about we introduce special value types per predicate instead of annotations?

@Find
Page<Product> search(@By(_Product.NAME) LikePattern namePattern,
                     @By(_Product.PRICE) ComparisonValue<Float> maxPrice,
                     PageRequest pageRequest);

That would also allow some runtime flexibility e.g. use a different comparison operator without having to duplicate the whole method.

This is a very interesting idea. I definitely think LikePattern is a really excellent alternative to @Pattern.

Nice one @beikov.

njr-11 commented 1 day ago

Just to throw another idea into the pit, how about we introduce special value types per predicate instead of annotations?

@Find
Page<Product> search(@By(_Product.NAME) LikePattern namePattern,
                     @By(_Product.PRICE) ComparisonValue<Float> maxPrice,
                     PageRequest pageRequest);

That would also allow some runtime flexibility e.g. use a different comparison operator without having to duplicate the whole method.

@beikov - Yes, that's an interesting approach for dynamically specifying comparisons. The point of this issue was to cover static, upfront definition of the comparisons. We have a separate issue #829 that is looking into dynamic patterns and your idea should be considered there rather than under this issue.

otaviojava commented 1 day ago

I liked this ComparisonValue mainly because we could use it in both situations, static and also dynamic queries.

I need to go deep on this next week and open a PR to make it easier to see this.

njr-11 commented 1 day ago

it can be easily extended to accommodate more things

For example, we get this for free:

@Find
Thing thing(@By("lower(code) like lower(?1)") String code`)

Whereas with @Is, I'm really not sure how you would express it.

It is not the goal to be able to express everything you can do in query language. That would result in an overly complex solution as is being cautioned against in some earlier comments. Instead the goal is to cover the same sort of basic capability new users will want such as what you get from Query by Method Name, except without the need to learn and rely on special syntax and parsing of method names.

The particular capability does actually have a Query by Method Name equivalent and so it's good to bring it up. Note that the query language solution, even the reduced one you give as an example, requires knowing there is a special lower function that you can take advantage of, applying to both the supplied value and the value in the datastore within a query to achieve a case insensitive comparison. That is asking a lot of users for something so basic (case insensitive compare on a single entity attribute). The query language solution is not intuitive or easy for beginning users.

It will be more clear once I have a chance to write it up in a PR, but the Is annotation does cover this particular scenario. The enumerated values that go into the annotation value make it clear and intuitive to users exactly what is available, making it easy to choose from,

@Find
List<Thing> match(@By(_Thing.CODE) @Is(LikeIgnoreCase) String code)

There are no hard-coded strings where you can make mistakes typing things or where you don't know what is valid to put there in the first place. There is no query language to learn. It (will be) well-defined in the API. The enumerated values are all isolated to one place within the annotation, so it avoids cluttering the rest of the spec. This will also be a great place to put an example and description of each of the defined options. It is conducive to autocomplete while writing code. Just pick from the available list.

I know some people have hesitations. And I think there are some misconceptions that this is going to expand into some complex model that tries to do everything query language does. That is not the point at all. It is only aiming to achieve some of the very core and basic things that Query by Method Name gives you, but in a way that is easier and more obvious (and type-safe) for beginning users.

njr-11 commented 1 day ago

Regarding query language fragments, I do think it is a good topic to consider and explore, but has a different goal and serves a different subset of users than what this issue is proposed for. In any case, I'll aim to keep discussion of query fragments to the #546 issue that is open for that. Once we have both of these proposals more fully developed, then we can assess whether it will make sense to have both of them in the spec, just one, neither, or other ideas that come up.

KyleAure commented 1 day ago

Sorry just getting caught up again. Here are my 2 cents on what has been discussed so far.

Seeing as how Jakarta Query is being proposed as a new spec for Jakarta EE 12 I would be hesitant to consider implementing query fragments until we know how Jakarta Data and Query are going to interoperate.

I think the biggest issue we need to address in EE 12 is the deprecation / removal of Query by Method Name. In this case, I agree with Nathan that we need to have simple, easy to code, and easy to understand alternative. In the past, we have pushed those scenarios into the parameter-based automatic queries and left the more complex queries to JDQL. The more complex we make parameter-based automatic queries the less useful it becomes and we will force users into JDQL and increase the learning curve of Jakarta Data.

An @Is or @Does annotation seems to be a good solution IMO.