Skip to content
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

Open
njr-11 opened this issue Oct 2, 2024 · 30 comments
Labels
enhancement New feature or request
Milestone

Comments

@njr-11
Copy link
Contributor

njr-11 commented Oct 2, 2024

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,

@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.

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,

@Find
@OrderBy(_Book.DUE_DATE)
@OrderBy(_Book.BORROWER)
@OrderBy(_Book.TITLE)
List<Book> booksDueBefore(@Is(LESS_THAN) LocalDate dueDate);

The repository user can easily understand that they should invoke the method with today's date to find the books that are overdue,

overdueBooks = library.booksDueBefore(LocalDate.now());

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,

@Find
@OrderBy(_Book.DUE_DATE)
@OrderBy(_Book.BORROWER)
@OrderBy(_Book.TITLE)
List<Book> booksDueBefore(Range<LocalDate> dueDate);

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,

overdueBooks = library.booksDueBefore(Range.before(LocalDate.now()));

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,

overdueBooks = library.booksDueBefore(Range.to(LocalDate.now().minus(Period.ofDays(1))));

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,

booksThatAreNotOverdue = library.booksDueBefore(Range.from(LocalDate.now()));

There are multiple problems here:

  • The repository author is unable to encode/enforce the requirements that it knows it has
  • The repository method is not guaranteed to do what its name says it will do, is vulnerable to mistakes, and can be misused for other purposes
  • The repository user needs to stop and think extra about how to use the method in order to ensure the above
  • The repository user is burdened with writing extra code, which is also more difficult to read when trying to understand the business logic of the application.

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.

@njr-11 njr-11 added the enhancement New feature or request label Oct 2, 2024
@njr-11 njr-11 added this to the 1.1 milestone Oct 2, 2024
@gavinking
Copy link
Contributor

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:

  • It "unifies" @Find with @Query in a way I find elegant, whereas introducing stuff like @Is(Like) keeps them separate and in fact doubles down on the divergence.
  • It's super low impact / high leverage: @By and @OrderBy already accept strings so this much of the proposal has actually zero impact on the APIs, and can be prototyped in implementations today!

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
Copy link
Contributor

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
Copy link

beikov commented Oct 25, 2024

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
Copy link
Contributor

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

@gavinking
Copy link
Contributor

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
Copy link
Contributor Author

njr-11 commented Oct 25, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

njr-11 commented Oct 25, 2024

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
Copy link
Contributor Author

njr-11 commented Oct 25, 2024

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
Copy link
Contributor

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.

njr-11 added a commit to njr-11/data that referenced this issue Nov 5, 2024
@gavinking
Copy link
Contributor

gavinking commented Nov 7, 2024

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:

the query has a fixed set of restrictions and sorting criteria

whereas "dynamic" meant:

the set of restrictions, and the sorting criteria are determined at runtime, by the caller of the repository.

I now realize that @njr-11 is using the term to mean something more like:

a static JPQL query string can be inferred at compilation time / startup time

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 Criteria API. It helps, of course, that in Hibernate the Criteria API and the JPQL AST are the exact same objects.

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:

  • I do something to elegantly handle the possibility of a null argument (this can be suppressed by annotating the parameter @NotNull or @Nonnull), and
  • the order is dynamic.

So to me, any change like, for example, replacing @Pattern with a Pattern class that can specify case sensitivity "dynamically" is a completely straightforward change. Which meant that the possibility of this raising hairiness on the implementation side simply had not even occurred to me. And I guess that's why I wasn't fully understanding some of the arguments which were being put forward. My bad, I should have asked for definitions.

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.

@gavinking
Copy link
Contributor

gavinking commented Nov 7, 2024

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 like or < or in. This problem was so acute that I even added an annotation @Pattern to Hibernate so that you could at least handle searching by a pattern.

When I did that, I came within a hair's breadth of also adding @From (or @LowerBound, whatever) to also handle the case of <. (Really, I actually started writing the annotation.) But then something held me back. I felt like I would be going down a path I would regret. I felt that it was going to be a wart on the side of the whole parameter-based query method facility, and ultimately an abuse of the Java language. So I stopped, and started asking myself about other ways to do it.

At this point we have seen ~ four different proposed approaches:

  1. Using annotations.

    @Find 
    List<Book> books(@Pattern String title, 
                     @From LocalDate publicationDate, 
                     @In List<Type> type)
  2. Using one annotation. Current version of this idea from @njr-11 is a single annotation and an enum.

    @Find 
    List<Book> books(@Is(LIKE) String title, 
                     @Is(GREATER_THAN_OR_EQUAL_TO) LocalDate publicationDate
                     @Is(IN) List<Type> type)
  3. Reusing @By and JPQL expressions.

    @Find 
    List<Book> books(@By("title like ?1") String title, 
                     @By("publicationDate > ?2") LocalDate startDate
                     @By("type in ?3") List<Type> types)
  4. Introducing Pattern and Range.

    @Find 
    List<Book> books(Pattern title, 
                     Range<LocalDate> publicationDate, 
                     List<Type> type)

Option 3

Of 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 Pattern or Range object, but this disadvantage must be weighed against the extra flexibility that the caller has in terms of being able to specify case-sensitivity, escaping, etc. And the fact that you have to explicitly distinguish between patterns and literal strings adds a tiny bit of extra type safety.

This is quite clearly the option which is easiest on the eyes.

Option 2

Option 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 @By and @OrderBy annotations already exist). In some sense I don't really care all that much whether the spec blesses this option or not, because we can support it in our implementation right now anyway. And we could even view it as an independent proposal which could be blessed alongside one of the other options, rather than as competing with them.

This option has the disadvantage that implementations not based on annotation processing can't validate the @By annotation at compile time. But, I mean, this problem already exists with @Query so I'm not sure how strongly this really counts against it. That's probably just something we should accept as an intrinsic limitation of the runtime-based solution. (Annotation processor-based solutions have their own intrinsic limitations!)

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 0

Option 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 1

Option 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.

@gavinking gavinking reopened this Nov 7, 2024
@njr-11
Copy link
Contributor Author

njr-11 commented Nov 7, 2024

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.

Option 1 fixes the explosion of annotations implied by Option 0, but at the cost of verbosity.

We could reduce some (admittedly not all) of the verbosity with shortened names. For example, the longest of the examples given @Is(GREATER_THAN_OR_EQUAL_TO) would be equally well understood and just as readable if we shortened it to, @Is(GREATER_THAN_EQUAL) or even @Is(GREATER_THAN_EQ). Some of the other constant names such as LIKE are already very short and others could benefit from being made more concise.

It's no more powerful than option 0, and doesn't comfortably accommodate vendor extensions.

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 Op enum and moving the String constants directly onto Is. This will be simpler and more concise, and even easier for users to discover and see what is available. And it will allow vendors, who for example want to define their own constant so they can easily offer extensions like @Is(ROUNDED_TO) price if they want to do that. Thanks for bringing this up. I'll look into updating the example proposal to allow this.

Together, perhaps, with the much more powerful option 2, I would say it's clearly the least easy on the eyes.

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.

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.

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.

@otaviojava
Copy link
Contributor

About option two, why not allow Query annotation at the parameter once it is the JPQL?

@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.

@gavinking
Copy link
Contributor

Why should we have a new annotation

@otaviojava @By isn't a new annotation; it's the same annotation we already have.

@otaviojava
Copy link
Contributor

Why should we have a new annotation

@otaviojava @By isn't a new annotation; it's the same annotation we already have.

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.

@gavinking
Copy link
Contributor

@otaviojava I guess I just don't think that @Query reads very nicely in that context. And it's not a "query", it's just an expression. Well, a predicate, I suppose.

On the other hand, @Find books @By("title like ?1") reads very naturally to me.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 7, 2024

I agree with Gavin that

@Find
books(@By("title like ?1") String pattern)

is very readable, whereas,

@Find
books(@Query("title like ?1") String pattern)

does not read well at all.

I don't know that I like the overloading of the By annotation value, which is supposed to be a field name that is ideally obtained from the static metamodel (or the special ID constant). But we should definitely not reuse @Query there. If we add query fragments, we will also want to consider a separate annotation such as @Where which also reads nicely and aligns with JDQL.

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,

@Find
books(String author,
      @By("yearPublished BETWEEN ?2 AND ?3") int minYear,
      int maxYear,
      String language,
      @By("title like ?5") String pattern)

or maybe they would write it as,

@Find
books(String author,
      @By("yearPublished BETWEEN ?1 AND ?2") int minYear,
      int maxYear,
      String language,
      @By("title like ?3") String pattern)

I'm not looking forward to trying to keep all of that straight.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 7, 2024

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,

@Find
books(String author,
      @By("yearPublished BETWEEN ?3 AND ?2") int maxYear,
      int minYear,
      String language,
      @By("title like ?5") String pattern)

@otaviojava
Copy link
Contributor

otaviojava commented Nov 8, 2024

@njr-11 @gavinking I am fine using Byas well.

Thus, we don't need another annotation to archive this.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 8, 2024

The more I think about basic usage examples, the more I'm convinced that the best solution is @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.

  1. Using annotations.

    @Find 
    List<Business> inCityRatedAtLeast(@Pattern(ignoreCase = true) String city,
                                      @From int rating)
  2. Using one annotation.

    @Find
    List<Business> inCityRatedAtLeast(@Is(IGNORE_CASE) String city,
                                      @IS(GREATER_THAN_EQ) int rating)

    FYI - I just thought of ANY_CASE as a more concise name for IGNORE_CASE. I'm thinking to switch the proposal to use that

  3. Reusing @By and JPQL expressions.

    @Find
    List<Business> inCityRatedAtLeast(@By("lower(city) = lower(?1)") String city,
                                      @By("rating >= ?2") int rating)
  4. Introducing Pattern and Range.

    @Find 
    List<Business> inCityRatedAtLeast(Pattern city,
                                      Range<Integer> int rating)

    Option 3 is no doubt the most flexible to the repository user and concise to write, but the repository writer didn't want to give extra flexibility. They want to shield the repository user from unwanted complexity, from opportunities for mistakes, and make usage more concise for the repository user.

Usage for options 0 to 2 is the same:

found = businesses.inCityRatedAtLeast("Rochester", 4);

Usage for option 3:

found = businesses.inCityRatedAtLeast(Pattern.like("Rochester").ignoreCase(), Range.of(4, 5));

Note that the above requires using like with a value that isn't a pattern at all. It will match the full value. That works. But it is awkward and shouldn't be a recommended approach, let alone the designed/only approach for Find with ignore case.

Beyond that, option 3 also lets you do things that at first glance look correct, but aren't:

found = businesses.inCityRatedAtLeast(Pattern.like("Rochester"), Range.of(2, 4));

First, the user needs to be aware of the need to include the .ignoreCase(). If they ever forget to do it, or don't fully read the documentation of the repository method and don't realize they should do it, they will be missing some results.
Second, the method name implies it will be returning results with the specified rating or higher. But it doesn't actually do that. Maybe the user didn't know the correct highest value, or maybe they thought the higher value didn't matter at all because it says AtLeast. There is more work for the repository method author to do in documenting how to properly use the method in order to get the correct results out of it, and more burden on the repository user to read and understand that documentation when using the method. The unwanted and unavoidable flexibility adds complexity, introduces opportunities for unintended usage and mistakes, and ultimately makes things harder.

Please don't take this as an argument against having Pattern and similar classes. There are times when the flexibility is very useful and where it will make a lot of sense. There are also times where it doesn't seem like the right solution, such as in trying to replace the ability of Query by Method Name to let the repository author define the most commonly used restrictions to make the method behave exactly as they want, making things simpler and more concise for the repository user (apart from the ugly long method names that it needed to do this). We can offer a really good solution here if we preserve the beneficial aspect while solving the problems, something that I believe the @Is annotation does extremely well.

@gavinking
Copy link
Contributor

gavinking commented Nov 8, 2024

I could see someone writing,

  @Find
  books(String author,
        @By("yearPublished BETWEEN ?2 AND ?3") int minYear,  //ERROR: @By may specify at most one parameter
        int maxYear,
        String language,
        @By("title like ?5") String pattern)

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.

I'm not looking forward to trying to keep all of that straight.

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.

@gavinking
Copy link
Contributor

Here's an even worse variant of the above

And again, the "worse variant" also has two parameters, which is again wrong and easily detected.

@gavinking
Copy link
Contributor

gavinking commented Nov 8, 2024

The more I think about basic usage examples, the more I'm convinced that the best solution is @Is.

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 Restriction, i.e. #829, where consensus does look easily-achievable, and leave this thorny issue for a future more-mature version of the spec. We should not try to set something in stone before we're sure that what we're doing is the right thing.

@gavinking
Copy link
Contributor

In the meantime, we can all experiment in our implementations, and gain practical experience.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 8, 2024

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.

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.

@njr-11
Copy link
Contributor Author

njr-11 commented Nov 8, 2024

In the meantime, we can all experiment in our implementations, and gain practical experience.

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.

@gavinking
Copy link
Contributor

gavinking commented Nov 8, 2024

It looks like you are thinking that the query language parameter number will always match the method parameter position.

Honestly I had not thought especially hard about this yet.

  • One option is to, yes, say that the query parameter number lines up with the position of the method parameter.
  • One option is to use a named parameter :param which lines up with the name of the method parameter.
  • A third option would be to allow JDBC-style parameters ? without a number or a name.
  • Perhaps we could even consider tolerating stuff like @By("yearPublished > ") int minYear but this isn't great if you want to do something like @By("lower(pattern) like lower(?1)") String title), so I'm not sure it's a good idea.

I need time to think it through further.

(These options are not mutually-exclusive.)

@otaviojava
Copy link
Contributor

@find
books(String author,
@by("yearPublished BETWEEN ?2 AND ?3") int minYear,
int maxYear,
String language,
@by("title like ?5") String pattern)

I understand the we can give a huge power in a annotation with a String, but. How safe this API is?
Now we have an annotation for two parameters...

In this case, I would go more to annotations, such as:

@Find 
List<Business> inCityRatedAtLeast(@Pattern(ignoreCase = true) String city,
                                  @From int rating, @To int maxYear );

//Maybe using the types that we talked about it:

List<Business> inCityRatedAtLeast(Pattern<Business> String city,
                                  Range<Business> year);


var businesses = repository.inCityRatedAtLeast(Pattern.contains("salvador").ignoreCase(), Range.between(2023, 2025);
//or
var businesses = repository.inCityRatedAtLeast(_Business.city.contains("salvador").ignoreCase(), _Business.year.between(2023, 2025));

I am not against the power, but it would greater if you also care about the safe part of the API as well.

@gavinking
Copy link
Contributor

How safe this API is?

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 @Query. In some impls, it's probably easier to check these small expressions upfront than it is to fully check a @Query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants