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

Ability to define query restrictions dynamically #829

Open
njr-11 opened this issue Aug 22, 2024 · 24 comments
Open

Ability to define query restrictions dynamically #829

njr-11 opened this issue Aug 22, 2024 · 24 comments
Labels
enhancement New feature or request
Milestone

Comments

@njr-11
Copy link
Contributor

njr-11 commented Aug 22, 2024

Replaces #825

Users will sometimes need to define queries dynamically instead of statically in response to what they see happen at run time. For example, a user specifies various customizable search options and in response, the application dynamically adds restrictions to a query to filter for the matching results.

An application ought to be able to define the same kind of query restrictions dynamically that it can already define statically (with patterns like Query by Method Name).

Query restrictions should make use of the Jakarta Data Static Metamodel and generally be agnostic to the database as much as possible, other than the same sort of limitations that Query By Method Name and the other static patterns have on which operations certain classes of NoSQL databases are incapable of.

The Jakarta Data Static Metamodel will be expanded to represent conditions that are possible per attribute type. These conditions can be supplied as an extra repository method parameter, similar to what is currently possible with Order.by/Sorts.

Example with one static and one dynamic restriction:

Page<Product> page1 = products.search(namePattern,
                                      _Product.price.lessThan(100.0),
                                      Order.by(_Product.price.desc(),
                                               _Product.id.asc()));

For multiple dynamic restrictions,

One possible way suggested by Otavio in 825

List<Product> list = products.search(
                         namePattern,
                         _Product.price.greaterThan(min)
                             .and(_Product.price.lessThan(max))
                             .and(_Product.color.equal(BLUE)
                                 .or(_Product.color.equal(GREEN))),
                         Order.by(_Product.name.asc(),
                                  _Product.id.asc()));

Another possible way (slightly more wordy, possibly more readable)

List<Product> list = products.search(
                         namePattern,
                         Require.all(_Product.price.greaterThan(min),
                                     _Product.price.lessThan(max),
                                     Require.any(_Product.color.equal(BLUE),
                                                 _Product.color.equal(GREEN))),
                         Order.by(_Product.name.asc(),
                                  _Product.id.asc()));

Or we could think of another way to cover and/or/not. That part can be figured out later. The basic idea here will be to add the restrictions to the static metamodel and decide how type safe to make them. For example, requiring Number for numeric attributes, or possibly more precise, or just Object for everything, ...

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

                         _Product.price.greaterThan(min)
                             .and(_Product.price.lessThan(max))
                             .and(_Product.color.equal(BLUE)
                                 .or(_Product.color.equal(GREEN))),

This to me is highly ambiguous. Conventionally, and has a higher precedence than or, but it's not at all clear what the precedence of the operators is here. Best to make it completely explicit as in your second example.

@hantsy
Copy link

hantsy commented Aug 24, 2024

For dynamically building the query criteria, I would like to consider the JPAStreamer and DotNet Core Entity Framework as references.

An example of JPASteamer home page, https://jpastreamer.org/

jpaStreamer.stream(User.class)
    .filter(User$.age.equal("22"))
    .sorted(User$.name.reversed())
    .skip(10)
    .limit(5)
    .forEach(System.out::println); 

Especially since Java has a great Stream API for filtering, transforming, and aggregating.

Use generated metamodels to assist in implementing the Stream API.

The Repository concept is a container for a collection of entities, should be streamable, and can be converted to Stream seamlessly.

products.stream().filter(Product$.name.like("").and(...))
.map(result -> another POJO)
.collect()/forEach/toList

In the implementation, translate it and execute it on the DB driver API level(Jdbc or NOSQL drivers).

@gavinking
Copy link
Contributor

@hantsy JPAstreamer is very cute and I don't hate it at all. It does indeed have a sort-of approximately similar scope to JDQL, in that it looks like it works well for queries with no joins or cartesian products. So it looks like in principle you could establish a welldefined mapping between the two things.

But on the other hand I'm not sure how cleanly it really fits with the notion of repositories and query methods. It's a sort-of different paradigm. If you want to use JPAstreamer for your queries, I'm not clear on why you would want to have a repository to begin with.

Anyway, I think it's something we can discuss. If we're already proposing to introduce restrictions like _Product.price.greaterThan(min) it might not be that much more work to make them also work with a stream-like API.

My main concern that we would maybe end up with a worse JPAstreamer with no support for e.g. group by. It's not clear that having a worse JPAstreamer would improve the ecosystem. [And I dunno how the person or people behind JPAstreamer would feel about us wading into this area.]

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

So I finally found some time to think more clearly about this stuff, and to spend some time designing a restrictions API for Hibernate 7. Note that our implementation of Jakarta Data works by generating very trivial code that just calls public APIs of Hibernate, and so if Data has restrictions, then Hibernate needs something equivalent.

What I came up with is something that to me feels extremely simple and elegant, and rather powerful.

  • Similar to our prototype, there's an interface Restriction with a lot of static factory methods.
  • Ignoring logical operations, there's essentially one implementation of Restriction, which is just an ordered pair of a JPA SingularAttribute and a Range. It just says "this attribute is constrained to this range".
  • The Range interface also has a bunch of static factory methods for different kinds of ranges. There's ten different built-in implementations, things like Pattern, Value, ValueList, and Interval. In principle users can define their own kinds of Range.
  • One can of course construct a Restriction in a generic fashion by calling Restriction.restrict(attribute, range), and that's completely type safe.

Now, the lovely thing about this is that a repository method parameter may accept:

  • either a Restriction<Entity>, in which case it's completely generic and can do anything, or
  • a Range<AttributeType>, in which case it applies any kind of restriction to the named attribute.

So I could write:

@Find List<Book> books(Restriction<Book> restriction);

and call it like:

List<Book> books = library.books(Restriction.startsWith(Book_.title, "Jakarta");
List<Book> books = library.books(Restriction.like(Book_.title, "Hibernate%");
List<Book> books = library.books(Restriction.in(Book_.id, List.of(1, 2, 3));

Or I can write:

@Find List<Book> books(Range<String> title);

and call it like:

List<Book> books = library.books(Range.prefix("Jakarta");
List<Book> books = library.books(Range.pattern("Hibernate%");
List<Book> books = library.books(Range.valueList(List.of(1, 2, 3));

Notice that there's not a single annotation anywhere. Nor is there any construct representing any kind of operator! There's no EQUALS enum value, there's just a kind of Range that represents a single value.

This solution is extremely expressive, highly customizable, extremely programmable, and completely type-safe.

It's also a really quite tiny amount of code, since I just made Restriction and Range know how to turn themselves into JPA criteria objects. (This particular aspect would not quite work for us.)

Anyway, Jakarta Data doesn't have to go down this path, but I'm throwing it out there as an option. I think it's both simpler and more powerful than what we have right now.

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 6, 2025

I think the first pattern, @Find List<Book> books(Restriction<Book> restriction); matches exactly with what we have already added to Jakarta Data for restrictions, so I'll focus on the second pattern.

  • a Range<AttributeType>, in which case it applies any kind of restriction to the named attribute.
@Find List<Book> books(Range<String> title);
List<Book> books = library.books(Range.valueList(List.of(1, 2, 3));

I got confused by the above combination, but I think the latter one is just a typo and you meant to have String values in the list to match up with the Range type parameter, like this,

List<Book> books = library.books(Range.valueList(List.of("1", "2", "3"));

Doing this without an annotation worked because it assumes -parameters is used to obtain the method parameter name, title. I assume the intent would be to allow @By(_Book.TITLE) if the application is not built with that compiler option.

In any case, the usefulness of this pattern with Range/EmptyRange/FullRange/Interval/LowerBound/UpperBound/NotNull/Pattern/Value/ValueList/CaseInsensitiveValue seems to be when the repository method author wants to enforce that the repository user must add query restrictions on specific predefined entity attributes (the names encoded in to the method parameter names) instead of any combinations of restrictions on entity attributes that the repository user wants. For example, a repository can have a method that requires its user to query on restrictions of the book title and length,

@Find List<Book> booksWithTitleAndLength(Range<String> title, Range<Integer> length);
...
List<Book> books = library.booksWithTitleAndLength(Range.containing("Jakarta EE"),
                                                   Range.closed(100, 500));

But if they want restrictions on a different combination, like title and author, then they need another repository method for that.

None of what I have stated above is arguing for or against the proposal. It is only exploring it at this point.

@gavinking
Copy link
Contributor

gavinking commented Jan 7, 2025

I think the first pattern, @Find List<Book> books(Restriction<Book> restriction); matches exactly with what we have already added to Jakarta Data for restrictions, so I'll focus on the second pattern.

When you want completely generic restrictions, it's exactly the same from the point of view of the user. But from the point of view of of how this is modeled and represented and how it's implemented in Java code it is, in my opinion, significantly more elegant than what we have right now.

I got confused by the above combination

Yes, sorry, that last example is wrong, of course.

Doing this without an annotation worked because it assumes -parameters is used to obtain the method parameter name, title. I assume the intent would be to allow @By(_Book.TITLE) if the application is not built with that compiler option.

Yes of course.

The idea is that:

  • Whereas we currently allow a parameter of a @Find method like String title, I also allow Range<String> title, and the client can pass in Range.pattern("%Java%") or whatever.
  • Similarly, whereas we currently allow int age, I also allow Range<Integer> age, and the client can pass in Range.greaterThan(17), or whatever.

Range/EmptyRange/FullRange/Interval/LowerBound/UpperBound/NotNull/Pattern/Value/ValueList/CaseInsensitiveValue

Note that those are all package-private implementation classes, I don't think I said that explicitly.

the repository user must add query restrictions on specific predefined entity attributes (the names encoded in to the method parameter names) instead of any combinations of restrictions on entity attributes that the repository user wants.

Correct. And we know 100% for sure that this is something people often want to do:

  • We know they want to be able to let the caller restrict strings by a pattern. And what is a pattern, really? It's a range of strings, nothing more nor less! For me as a caller of the method it's super-nice to be able to choose to express this range as startsWith() or contains() instead of using weird "metacharacters".
  • Similarly, imagine the common problem of restricting a field of type LocalDate. It's very common for a UI to allow the user to specify a start date, an end date, both, or neither. Passing null to a repo method startDate or endDate parameter doesn't work here, so you would need to specify four overloads of the method! Or, you could have a Range<LocalDate> parameter, and let the caller pass in Range.greaterThanOrEqual(), Range.lessThanOrEqual(), Range.closed(), or Range.full().

Compare:

@Find 
List<Records> records(long tenant, Range<LocalDate> creationDate);

with something like:

@Find 
List<Records> records(long tenant);
@Find 
List<Records> records(long tenant, 
        @By(_Record.CREATION_DATE) @Is(GREATER_THAN_OR_EQUAL) LocalDate startDate);
@Find 
List<Records> records(long tenant, 
        @By(_Record.CREATION_DATE) @Is(LESS_THAN_OR_EQUAL) LocalDate endDate);
@Find 
List<Records> records(long tenant, 
        @By(_Record.CREATION_DATE) @Is(LESS_THAN_OR_EQUAL) LocalDate startDate,
        @By(_Record.CREATION_DATE) @Is(GREATER_THAN_OR_EQUAL) LocalDate endDate);

The solution with Range is not only much easier for the repository author, it's also nicer for the caller, who can construct the Range far away from the call to the repo method.

Now, of course, you might object that "well, nobody would write code like that, they would just use Restriction instead". And that's true. But that's also less convenient for the caller who:

  • now needs Restriction.between(Record_.creationDate, startDate, endDate) instead of just Range.closed(startDate, endDate), and
  • might need to invent their own DateRange class if they plan on constructing it far from the repository method invocation.

Now, I get it, these benefits might not seem all that compelling to you (though to me I find them quite convincing). And so if I were proposing a more complex solution, I can imagine that some people might think it just not worth the extra complexity. But I'm emphasizing here that this proposal is fundamentally simpler. You get this extra flexibility for negative cost.

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 7, 2025

Range/EmptyRange/FullRange/Interval/LowerBound/UpperBound/NotNull/Pattern/Value/ValueList/CaseInsensitiveValue

Note that those are all package-private implementation classes, I don't think I said that explicitly.

I saw that when I was trying write an example. I think that works for your implementation where you are interpreting them internally and able to return JPA Predicate off of your API. In a spec where the API is separate from a provider implementation, there would still need to be a way for the provider to obtain from the Range (or Restriction) information about what it represents. Currently, we are using Operator to supply some of that information. I don't think we currently have any cases where an application actually uses Operator - it is intended for the Jakarta Data provider. With respect to the Restriction/Range patterns, I don't have any particular attachment to having an Operator class, and if we find it better to use something else, I'm fine with that. But I'm also not sure how it is actually replaced in your proposal though when separating the API from the Jakarta Data provider implementation. We won't be able to have a toPredicate method that returns a JPA class. It seems that we would need some other abstraction in between.

  • We know they want to be able to let the caller restrict strings by a pattern. And what is a pattern, really? It's a range of strings, nothing more nor less! For me as a caller of the method it's super-nice to be able to choose to express this range as startsWith() or contains() instead of using weird "metacharacters".

This point is misleading. It implies that the pattern that uses "metacharacters" does not offer startsWith. It does offer startsWith and contains,

library.books(_Book.title.startsWith("Jakarta EE"));
library.books(_Book.title.contains("Jakarta EE"));

versus the Range proposal,

library.booksTitled(Range.startsWith("Jakarta EE"));
library.booksTitled(Range.contains("Jakarta EE"));

Your point here would be more valid stating a user would like to use startsWith and contains without using metacharacters.

  • Similarly, imagine the common problem of restricting a field of type LocalDate. It's very common for a UI to allow the user to specify a start date, an end date, both, or neither. Passing null to a repo method startDate or endDate parameter doesn't work here, so you would need to specify four overloads of the method! Or, you could have a Range<LocalDate> parameter, and let the caller pass in Range.greaterThanOrEqual(), Range.lessThanOrEqual(), Range.closed(), or Range.full().

This one I don't agree with at all. You have switched from comparing Range versus Restriction to instead comparing Range versus the @Is annotation, but for a scenario that involves computing the restrictions at run time. That is not the purpose of the @Is annotation at all, so of course no one should use it for that. If you want to compare a scenario of computing restrictions at run time, we need to compare Range versus Restriction. Here is what that would look like,

Range:

@Find 
List<Record> records(long tenant, Range<LocalDate> creationDate);

tenantInfo.records(tenantId, Range.greaterThanOrEqual(date));
tenantInfo.records(tenantId, Range.lessThanOrEqual(date));
tenantInfo.records(tenantId, Range.closed(firstDate, lastDate));
tenantInfo.records(tenantId, Range.full(LocalDate.class));

Restriction:

@Find 
List<Record> records(long tenant, Restriction<Record> restrictions);

tenantInfo.records(tenantId, _Record.creationDate.greaterThanEqual(date));
tenantInfo.records(tenantId, _Record.creationDate.lessThanEqual(date));
tenantInfo.records(tenantId, _Record.creationDate.between(firstDate, lastDate));
tenantInfo.records(tenantId, null);

These are about the same. In both cases, I would say that ignoring/skipping/full-ranging the Range or Restriction parameter is awkward and we should look into better ways of allowing that if needed. Probably there should be a Restrict.ignore() or something with a better name rather than passing in null, but we haven't added that yet.

If we go further with the example where the user wants to add restrictions on more entity attributes, then the greater flexibility of the Restriction pattern gives an advantage,

tenantInfo.records(tenantId, Restrict.all(_Record.description.notNull(),
                                          _Record.description.contains(searchText),
                                          _Record.creationDate.greaterThanEqual(date)));

With Restriction, the same repository method can be reused for flexible search criteria, whereas Range will require additional repository methods for every different combination of entity attributes possible in flexible search criteria.

This is not to say we can't have Range as well. It isn't a replacement for Restriction and it isn't a replacement for @Is or Query by Method Name. Range does have some value on its own, which seems to be that it is a semi-flexible pattern that doesn't require the static metamodel or hard coding String names.

It should be noted that it might be an option for someone who doesn't want to type a static metamodel class to do a static import, and then you have,

tenantInfo.records(tenantId, creationDate.greaterThanEqual(date));

but that won't be desirable in all cases, especially if there are multiple entity classes to use with an overlap in attribute names.

@gavinking
Copy link
Contributor

gavinking commented Jan 7, 2025

I think that works for your implementation where you are interpreting them internally and able to return JPA Predicate off of your API. In a spec where the API is separate from a provider implementation, there would still need to be a way for the provider to obtain from the Range (or Restriction) information about what it represents.

Yes, right, those types might need to be exposed as an SPI. In fact it's just about a perfect use-case for sealed types.

This point is misleading. It implies that the pattern that uses "metacharacters" does not offer startsWith. It does offer startsWith and contains.

You're misunderstanding, what I'm saying is that:

  • In the @Is(LIKE) String title approach, the caller is forced to use a given format (%, _ metacharacters, or whatever).
  • With Range<String>, the caller gets to decide how they want to express the range of allowed strings. I actually find this quite a lot better, since I much prefer to write contains(word) compared to '%' + word + '%'.

This one I don't agree with at all. You have switched from comparing Range versus Restriction to instead comparing Range versus the @is annotation, but for a scenario that involves computing the restrictions at run time. That is not the purpose of the @is annotation at all, so of course no one should use it for that.

What I'm saying is that Range is far, far better than @Is and makes @Is completely unnecessary. It's more elegant for both the repository author, and for the caller of the repository method.

This is not to say we can't have Range as well. It isn't a replacement for Restriction

Correct. It's a replacement for @Is.

@gavinking
Copy link
Contributor

gavinking commented Jan 7, 2025

The "competition" here is between:

@Find 
List<Records> records(long tenant, 
        @By(_Record.CREATION_DATE) @Is(LESS_THAN_OR_EQUAL) LocalDate startDate,
        @By(_Record.CREATION_DATE) @Is(GREATER_THAN_OR_EQUAL) LocalDate endDate);

And:

@Find 
List<Records> records(long tenant, Range<LocalDate> creationDate);

But by providing Range, we also by side-effect make Restriction almost trivial because it's reduced to just a package of a Range and an Attribute.

@gavinking
Copy link
Contributor

(One of my favorite tricks is to provide very simple but generic primitives and build up more complex higher-semantic-level things out of them. Range is that kind of very simple/generic primitive.)

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 7, 2025

What I'm saying is that Range is far, far better than @Is and makes @Is completely unnecessary. It's more elegant for both the repository author, and for the caller of the repository method.

I don't know why there is discussion of @Is under an issue titled "Ability to define query restrictions dynamically" because that isn't what the @Is annotation is for at all. So of course it isn't going to be useful for these scenarios which all relate to defining criteria at runtime. The correct comparison to make for defining criteria at runtime is comparing Range against Restriction.

If you want to discuss @Is then we need to look at a completely different set of scenarios, such as the library example under the description of issue 857 which is making a direct comparison with Range (although the Range method names used are the ones written by Otavio rather Hibernate). I'm not going to repeat that here because discussion of it belongs under that issue instead.

The patterns for @Is and Range/Restriction are complementary, not in competition with each other. Best practice for writing a repository method should be to define all fixed sorting and/or restrictions upfront (involving -parameters/@By, @Is, and @OrderBy as needed), then give it the most meaningful name based on the fixed aspects, then use Sort/Order and Restriction/Range parameters to allow for additional criteria to be determined at runtime, as desired.

@gavinking
Copy link
Contributor

gavinking commented Jan 7, 2025

I don't know why there is discussion of @Is under an issue titled "Ability to define query restrictions dynamically" because that isn't what the @Is annotation is for at all.

OK, Nathan, stop. Take a step back. I'm asking you to clear your mind of your preconceptions and of the way you have been thinking about these problems up until now. In your head there are two distinct things here, and they call for completely separate solutions. I fundamentally disagree with this.

And so by insisting that we conceptualize these things separately, you're asking me to concede the main part of my argument. Not so fast!

I've been trying to push you guys in the direction of thinking that even though there might sorta be two things, that they can actually be solved more elegantly by one more unified thing. I know you like @Is, but I insist that Range does everything @Is can do but better and more elegantly. And at the same time it makes Restriction much simpler.

The point is that Range kills two birds with one stone (and perhaps more birds in future).

@gavinking
Copy link
Contributor

I know you like @Is, but I insist that Range does everything @Is can do but better and more elegantly.

And of course, if I'm wrong and you're right, and we ultimately figure out that Range and @Is are addressing actually-different problems, it's easy to add @Is.

What's not so easy is to retrofit Range to the Restriction API if we don't do it now in this release. Or at least, it might be possible to retrofit it, but we would not benefit from the simplifications it enables.

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 7, 2025

I've been trying to push you guys in the direction of thinking that even though there might sorta be two things, that they can actually be solved more elegantly by one more unified thing. I know you like @Is, but I insist that Range does everything @Is can do but better and more elegantly.

When asked to consider Range as the solution to #857 (which is what @Is was proposed to cover), I find that it is not an elegant solution, nor does it solve the problem. It ends up doing something else and only restricting the attribute name and not the comparison. The repository method ends up named generically, does not correspond to the intent of the repository author, and requires extra thought and code for the repository user to use it. When I'm asked to consider Range as the solution to such scenarios, I don't want it at all. If Range is presented as having a different purpose, then I can see some value in it.

@gavinking
Copy link
Contributor

It ends up doing something else and only restricting the attribute name and not the comparison.

What possible legitimate purpose might I, as a repository author, have in wanting to restrict the kind of comparison that the client can perform?

  • I have no motivation in terms of typesafety.
  • I have no motivation in terms of security. The same data is available. And it's not as if these repositories are HTTP endpoints or something.
  • I have no motivation in terms of clarity/simplicity/readability/maintainability.

Why would I as a repository author possibly want to let the client restrict a date by > but not by < or between? I just can't think of one. That just makes my repository less useful!

I mean, sure, previous inferior repository frameworks did force people to do it that way, and so we've all certainly seen lots of examples like that. But that doesn't mean there's a good reason to do it like that, or that it's what people would choose to do if we gave them the choice.

But perhaps I'm wrong. Perhaps you can think of some motivation that I've not thought of?

Anyway, this is great. Now we've really arrived at the root of the disagreement. I was struggling to see where the disconnect was. Now I see it.

@gavinking
Copy link
Contributor

gavinking commented Jan 7, 2025

The repository method ends up named generically, does not correspond to the intent of the repository author,

Consider:

var books = library.byTitle(like("%Hibernate%"));  // (1)
var books = library.byTitle(contains("Hibernate"));  // (2)

These are not in my opinion more generic, nor lacking in clear intent than:

var books = library.byTitleLike("%Hibernate%");  // (3)

In fact, examples (1) and (2) express the intent much more clearly, since they express it through the type system.

If I didn't add Like into the method name in example (3), then the caller would have to check the annotations on the method to see if this method accepts a pattern or just a plan string.

Similarly:

var records = archive.byCreationDate(greaterThan(startDate));  // (4)

Expresses its intent, more clearly, in my opinion, than:

var records = archive.byCreationDateAfter(startDate);  // (5)

Again, the type system forces me to express the intent clearly, instead of me having to name my method carefully.

Hell, we (or the user) can even add trivial convenience methods for LocalDate so that example (4) becomes:

var records = archive.byCreationDate(after(startDate));

@njr-11
Copy link
Contributor Author

njr-11 commented Jan 7, 2025

Perhaps you can think of some motivation that I've not thought of?

The following example was in the other issue, mentioned earlier,

overdueBooks = library.booksDueBefore(today);

I can quickly think of others like,

children = people.children(maxChildAge);
potentialDrivers = people.ofDrivingAge(minDriverAge);
candidatesForBonus = employees.highPerformers(minRating, Set.of(Role.SOFTWARE_ENGINEER,
                                                                Role.SOFTWARE_DEVELOPER,
                                                                Role.SOFTWARE_ARCHITECT));

Although I haven't been recording examples that I see, I've run into enough of them to know the pattern is quite useful, and would pretty much always be the starting point I would take when writing a repository method of my own: First, what do I know of for restrictions and sort criteria that cannot change? If there is any, then encode it into the method upfront.

I would say that Query by Method Name actually was actually working against this pattern by forcing less meaningful names that become exponentially more cumbersome to use as the number of restrictions increases linearly and thus discouraging use. Moving away from that and being able to use concise names that are meaningful to the business logic will driver more usage, which will be good because it should be encouraged as a best practice for the repository author to encode upfront everything that cannot change, sparing the repository user from needing to do extra work of figuring out how to use a more generally named method with which Ranges to wrap their values in to get what they really wanted.

I know you'll argue against some of my simple examples: Why not make them more flexible with Range (or even Restriction) and the repository users can do whatever they want with them? In cases, where that is what you want, then yes, go for it. But in cases where there are specific business requirements and usage patterns, the repository author should be able to take advantage of encoding them and exposing things to the repository user more precisely and meaningfully in terms of the model that they have.

@gavinking
Copy link
Contributor

I know you'll argue against some of my simple examples: Why not make them more flexible with Range (or even Restriction) and the repository users can do whatever they want with them?

That's ... um, well, yeah ... pretty much exactly what I think, actually.

I'm kind of a big fan of abstraction, reusability, and composabilty.

Moving away from that and being able to use concise names that are meaningful to the business logic will driver more usage

I do agree with this.

But from this point of view booksDue(before(now())) is every bit as good if not better than booksDueBefore(now()).

it should be encouraged as a best practice for the repository author to encode upfront everything that cannot change

This is the bit where you lose me. I don't really understand what you're trying to achieve here.

Uncharitably, it seems your goal here is to deliberately make your code less generic than it could be. That's, like, the exact opposite of everything I'm aiming for when I write a program .

@gavinking
Copy link
Contributor

gavinking commented Jan 7, 2025

I guess the point is that the reason I changed my mind about repositories is NOT that I liked the fact that I was trading a single generic EntityManager for tens or hundreds of little much less abstract FooEntityManagers.

On the contrary, that's what I still don't like about repositories. (For the exact same reason that I don't write BookList classes anymore like we used to do before Java got generics.)

The reason I changed my mind about repositories was because we figured out a way to achieve a level of typesafety that was impossible with EntityManager. I'm prepared to trade quite a lot of genericity for type safety. (Otherwise I would write all my code in Smalltalk or something.) But when I'm not getting extra type safety, I would prefer to keep the polymorphism.

@montanajava
Copy link

montanajava commented Jan 13, 2025

If you all could humor the following question: For my own understanding, does the following example illustrate the type of problem the API would be designed to address? Assume that the data model for this example GUI pane is comprised of multiple tables.

+------------------------------------------------------+
|                    Sweater Filter                    |
+------------------------------------------------------+
|                                                      |
|                      Colour                          |
|  [ ] Red                     [ ] Green               |
|  [ ] Blue                    [ ] Black               |
|                                                      |
+------------------------------------------------------+
|                       Size                           |
|  [ ] Small                   [ ] Large               |
|  [ ] Medium                  [ ] XL                  |
|                                                      |
+------------------------------------------------------+
|                     Price Range                      |
|  ( ) Below $20                                       |
|  ( ) $20 - $50                                       |
|  ( ) $50 - $100                                      |
|  ( ) Above $100                                      |
|                                                      |
+------------------------------------------------------+
|                   Manufacturer                       |
|  [ ] Brand A                                         |
|  [ ] Brand B                                         |
|  [ ] Brand C                                         |
|  [ ] Brand D                                         |
|                                                      |
+------------------------------------------------------+
|                       Gender                         |
|  ( ) Men                                             |
|  ( ) Women                                           |
|  ( ) Unisex                                          |
|                                                      |
+------------------------------------------------------+
|                  Released (Date)                     |
|  From: [ YYYY-MM-DD * ]                              |
|  To:   [ YYYY-MM-DD ]                                |
|                                                      |
+------------------------------------------------------+
|                  Shipping Method                     |
|  [ ] Free Shipping                                   |
|  [ ] Express Delivery                                |
|  [ ] Standard Shipping                               |
|                                                      |
+------------------------------------------------------+
|                   Group By                           |
|  Group by:                                           |
|    1. [ Color ↓ ]   [ + Add ]                        |
|    2. [ Size ↓ ]                                     |
|    3. [ None ]                                       |
|                                                      |
|  * Drag to reorder priority                          |
|                                                      |
+------------------------------------------------------+
|                     Sorting                          |
|  Order by:                                           |
|    1. [ Price ↓ ]   [ + Add ]                        |
|    2. [ Popularity ↓ ]                               |
|    3. [ None ]                                       |
|                                                      |
|  * Drag to reorder priority                          |
|                                                      |
+------------------------------------------------------+
|                 Sales Features                       |
|                                                      |
|  Discount:                                           |
|    [----O=======================] 50% or greater     |
|                                                      |
|  [ ] Show only Best Sellers                          |
|                                                      |
|  Minimum Reviews:                                    |
|    [ Number Input: __ ]                              |
|                                                      |
+------------------------------------------------------+
| [ Apply Filter ]                           [ Reset ] |
+------------------------------------------------------+


@njr-11
Copy link
Contributor Author

njr-11 commented Jan 13, 2025

For my own understanding, does the following example illustrate the type of problem the API would be designed to address?

@montanajava Yes, that is an excellent example. In Data 1.0, the only part of this that was possible was the Sorting. This issue/feature will make everything else in your example possible, except Group By. I'm not sure I properly understand the meaning of Group By in this example. If it is only meant as a higher level of sorting, then it would be already possible as well, by using Jakarta Data's Sort. If it is anything beyond that, then it's a feature we don't have and isn't solved by this issue. In that case, open a new issue for discussion of it and to go over the use case in more detail.

@montanajava
Copy link

@njr-11 Thanks for your reply. A higher level of sorting is indeed the most common use case I had in mind. While there are arguments for including "group by", I agree it would be best to create a separate issue for that.

I'm really glad to see you confirm the problem domain this issue addresses! It tackles a real-world need, I can assure you.

@hantsy
Copy link

hantsy commented Jan 14, 2025

Besides the fluent APIs to assemble the query criteria, we can port the QueryByExample into data spec. It is included in a few frameworks, such as Quarkus, Micronaunt Data, and Spring Data.

@gavinking
Copy link
Contributor

Besides the fluent APIs to assemble the query criteria, we can port the QueryByExample into data spec.

Big -1. We had this in Hibernate for many, many years, long before any of those other things you mention existed and:

  • It doesn't work properly with primitive types.
  • Nobody used it.
  • We removed it in H6 and nobody complained.

This doesn't belong in the Data spec. If you like, it's fairly trivial to build your own query by example facility using the Restriction API.

njr-11 added a commit to njr-11/data that referenced this issue Jan 23, 2025
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

4 participants