-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parameter-based Automatic Queries as a full alternative to Query by Method Name #857
Comments
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:
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). |
For example, we get this for free: @Find
Thing thing(@By("lower(code) like lower(?1)") String code`) Whereas with |
Just to throw another idea into the pit, how about we introduce special value types per predicate instead of annotations?
That would also allow some runtime flexibility e.g. use a different comparison operator without having to duplicate the whole method. |
I agree with @gavinking introducing overly complex annotation-based DSLs doesn't seem to be a wise direction |
This is a very interesting idea. I definitely think Nice one @beikov. |
@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. |
I liked this I need to go deep on this next week and open a PR to make it easier to see this. |
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. 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 It will be more clear once I have a chance to write it up in a PR, but the @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. 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. |
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. |
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 |
…tomatic query Signed-off-by: Nathan Rauh <[email protected]>
Alright, so after last night's discussion I realized that I simply had not understood a distinction that some people were making here, that is, what was meant by "static". To me, "static" meant, approximately something like:
whereas "dynamic" meant:
I now realize that @njr-11 is using the term to mean something more like:
And it occurs to me that that this is probably because we're working from very different implementations. In my implementation, I don't attempt to compile a parameter-based query method to a JPQL string (which would then be parsed to an AST and then rendered to SQL). Instead I just jump directly to creating the AST via the JPA So a parameter-based query method like this: @Find // @Pattern is a Hibernate extension
List<Book> byTitle(@Pattern String title, Order<Book> order); Compiles to this: @Override
public List<Book> byTitle(String title, Order<Book> order) {
var _builder = session.getFactory().getCriteriaBuilder();
var _query = _builder.createQuery(Book.class);
var _entity = _query.from(Book.class);
_query.where(
title==null
? _entity.get(Book_.title).isNull()
: _builder.like(_entity.get(Book_.title), title)
);
var _orders = new ArrayList<org.hibernate.query.Order<? super Book>>();
for (var _sort : order.sorts()) {
_orders.add(by(Book.class, _sort.property(),
_sort.isAscending() ? ASCENDING : DESCENDING,
_sort.ignoreCase()));
}
try {
return session.createSelectionQuery(_query)
.setOrder(_orders)
.getResultList();
}
catch (PersistenceException exception) {
throw new DataException(exception.getMessage(), exception);
}
} Notice that this query is already "dynamic" in at least one sense, since:
So to me, any change like, for example, replacing Now, that said, I would argue that this sort of implementation concern should NOT be driving the design of the API! Indeed, we run a serious risk of getting stuck in a local optimum here. We're already going to add dynamic restrictions, and so implementors are going to have to be able to handle dynamic query construction sí o sí. And the precise position of the line between what is "static" and "dynamic" in Nathan's sense does not need to be reflected in our APIs, because that is going to be implementation-dependent anyway. These "static" queries are an optimization in IBM's implementation of Jakarta Data. It's an implementation detail. I'm choosing not to perform this optimization because I know that Hibernate itself is capable of performing similar optimizations at a later stage and in a different way. Instead we should be designing the API from the point of view of what is most elegant from the point of view of the user. OK, I'm going to stop here, and then post a follow-up comment. |
So, when we first started writing code example using parameter-based query methods we all immediately ran into the problem of not being able to do When I did that, I came within a hair's breadth of also adding At this point we have seen ~ four different proposed approaches:
Option 3Of these it's now clear to me that option 3, due to @beikov, is the one which is much more in line with the original spirit of parameter-based query methods. It has continued to grow on me the more I think about it. Of course it has the disadvantage that the caller has to explicitly construct a This is quite clearly the option which is easiest on the eyes. Option 2Option 2 is the strictly most-powerful approach because you can call arbitrary JDQL/JPQL functions and operators and thus do all sorts of things which are simply not possible with the other options. It's also nice in that it doesn't grow the API surface at all (the This option has the disadvantage that implementations not based on annotation processing can't validate the This approach does not get in the way of re-expressing a parameter-based query method as a static JDQL/JPQL string, which is more comfortable for some implementors. Option 0Option 0 is in some sense the simplest, and easiest to understand. But it leads to somewhat of an explosion of annotation, and I don't think any of us is pushing it. It's strictly less-powerful than options 2 and 3. On the other hand, vendor-specific extensions are actually very easy to accommodate into the framework it lays out, making it in some sense quite extensible. In its defense, it's easy on the eyes. This option doesn't get in the way of re-expressing a parameter-based query method as a static JDQL/JPQL string, which is more comfortable for some implementors. (See long discussion above.) Option 1Option 1 fixes the explosion of annotations implied by Option 0, but at the cost of verbosity. It's no more powerful than option 0, and doesn't comfortably accommodate vendor extensions. Together, perhaps, with the much more powerful option 2, I would say it's clearly the least easy on the eyes. Like options 0 and 2, this approach doesn't get in the way of re-expressing a parameter-based query method as a static JDQL/JPQL string, which is more comfortable for some implementors. |
That's a nice, thorough overview and analysis of the differences between the options that have been proposed. Some of the shortcomings of Option 1 might be addressable with some alterations, and I'll explore some of that here.
We could reduce some (admittedly not all) of the verbosity with shortened names. For example, the longest of the examples given
The way I initially proposed it with the constants in an enumeration seemed nice at the time because it limits users to valid operations defined by the spec. I wasn't thinking at all about vendors that wanted to extend it. But that would actually be easy to accommodate by getting rid of the
This one is a matter of opinion. In my opinion it is the most easy on the eyes. It reads naturally like a short phrase, is obvious what is does, and there is excellent symmetry when you have multiple parameters, enabling you to more quickly process and understand multiple at once. Options 0 and 3 do not have that symmetry and are visually all over the place with different special parameter types needing to be considered and understood in different ways.
I can add onto this the same will be true for JDBC-based implementations that could pre-generate fixed SQL commands, and in general for any data store type that has a query language that accepts parameters. I'm not really sure what sort of implementations will be developed for this spec over time, but both those that already exist and those that have yet to be created, will be interested in having what they can already do for Query by Method Name and Parameter-based Automatic Query that only use equality comparisons, to also work for the other common comparisons that will be added to Parameter-based Automatic Query. |
About option two, why not allow @Find
List<Book> books(@Query("title like ?1") String title,
@Query("publicationDate > ?2") LocalDate startDate
@Query("type in ?3") List<Type> types) Why should we have a new annotation to archive the same goal? This point was not clear to me. |
@otaviojava |
When I say new, I mean in the proposal. For example, we already have a Query annotation that executes JPQL. We don't need to define a new proposal to By; instead, we use the same Query annotation to keep its proposal, but in a different context, in this case, the parameter. |
@otaviojava I guess I just don't think that On the other hand, |
I agree with Gavin that
is very readable, whereas,
does not read well at all. I don't know that I like the overloading of the I haven't thought through query fragments enough yet so I'm also a little worried about the possibility they cause complexity and mistakes when they aren't the most basic scenarios that we currently have examples of. One that immediately comes to mind is how they will be used when intermixed with other parameters. I could see someone writing,
or maybe they would write it as,
I'm not looking forward to trying to keep all of that straight. |
Here's an even worse variant of the above where the repository writer decided they prefer to have their method accept the maximum range value prior to the minimum and so they swap the numbers thinking we will figure it out,
|
@njr-11 @gavinking I am fine using Thus, we don't need another annotation to archive this. |
The more I think about basic usage examples, the more I'm convinced that the best solution is A simple example here is returning a list of businesses in a city with a particular rating or better (5 is best, 1 is worst). The repository developer, knowing about the data they are writing the repository for, is aware that city names will not always be in a consistent case. Here is how this works out using the 4 options (0 - 3) described earlier.
Usage for options 0 to 2 is the same:
Usage for option 3:
Note that the above requires using Beyond that, option 3 also lets you do things that at first glance look correct, but aren't:
First, the user needs to be aware of the need to include the Please don't take this as an argument against having |
I mean I see this as something that's just quite clearly an error. The JDQL fragments must have a single parameter in them, not two parameters, not zero parameters.
The bright-line rule is: exactly one parameter. Simple. You would write this code as: @Find
books(String author,
@By("yearPublished > ?2") int minYear,
@By("yearPublished < ?3") int maxYear,
String language,
@By("title like ?5") String pattern) Which is perfectly fine. |
And again, the "worse variant" also has two parameters, which is again wrong and easily detected. |
OK, fine, so it seems clear that there's no consensus here; nor does consensus look likely to be achievable, since we've been circling this for some time now, with no apparent progress at all. Therefore, we should not do this in Jakarta Data 1.1. Let's focus on the question of |
In the meantime, we can all experiment in our implementations, and gain practical experience. |
Thanks for clarifying that. It wasn't obvious to me what all the intended limitations were for the query fragments. The example here is invalid then, but the question of how to handle the mixture of parameter types still applies. It looks like you are thinking that the query language parameter number will always match the method parameter position. We would need to include that requirement so users don't do what I did in one the examples and number according to only the query fragment parameters. |
Yes, been doing that and will continue to do so with the various improvements we've been thinking up while discussing the options. Hopefully having the PR out there illustrates the idea and helps out others who want to try as well. |
Honestly I had not thought especially hard about this yet.
I need time to think it through further. (These options are not mutually-exclusive.) |
I understand the we can give a huge power in a annotation with a String, but. How safe this API is? In this case, I would go more to annotations, such as:
I am not against the power, but it would greater if you also care about the safe part of the API as well. |
Depends on the implementation. In our implementation we're able to completely syntax- and type- check JDQL queries at compile time, and so we can easily check these fragments too. But not every implementation is going to do that. The point is, it's at least no worse than |
The parameter-based Automatic Queries pattern already provides an alternative to much of what Query by Method Name offers for the repository author to define queries upfront, 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 places.
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,The above directly corresponds to the following Query by Method Name method that could currently be used:
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.
There has been some confusion over how this differs from other new capability like Restrictions and new Special parameter types (Pattern/Range) with which the repository user can customize different types of conditions. These are two very different usage patterns. In some cases, the nature of the application involves a repository user needing the flexibility to compute different conditions at run time, such as in response to an end user defining different search conditions. But in other cases, the repository author wishes to provide specific capability and to externalize it to the repository user in the most simplistic and direct way, eliminating as much misuse and error conditions as possible.
When the repository author has a set of requirements that can be encoded into the repository method definition in advance, they ought to be able to do so, in order to abstract the complexity away from the repository user, who can then use the repository in the simplest way, without concern for complexities that the repository author has already taken care of.
A brief example of this is a repository for a library that wants to identify overdue books in order to remind the borrower to return them. The repository author might write the following method, encoding the LESS_THAN condition for the dueDate to identify overdue books, and appropriately naming the method so that the repository user knows exactly what it is for,
The repository user can easily understand that they should invoke the method with today's date to find the books that are overdue,
If the repository author had instead written the method without encoding the LESS_THAN requirement, and instead only encoding that it needs to compare the dueDate in some way, then it might look something like this,
The burden is then on the repository user to figure out what type of range to supply in order to make the method do what its name says it will do.
One right answer is that they can use
Range.before
, sending in the current date to make it return all past values to finds all overdue books,The other right answer is that they can use
Range.to
, sending in 1 less than the current date, also making it return all past values to find all overdue books,There is (unwanted) flexibility to send in some other Ranges to the method, making it possible to do things that contradict what its name says it will do,
There are multiple problems here:
A best practice should be that the repository author always encodes in advance all of the restrictions that they know will remain constant across all usage of the method. Currently, the only ways to do that are Query by Method Name (but that pattern is discouraged and expected to go away) and Query Language (advanced users only). This issue is opened so that we have a simple recommended way for newer and non-advanced users. It should be straightforward, clear, and easily discoverable which restrictions are available.
The text was updated successfully, but these errors were encountered: