Sort with type parameter

[njr-11]

To help evaluate the proposal to have Sort with a type parameter #430, I created this pull to demonstrate what that would look like. As expected, it spills over into Pageable, which is constructed from Sorts, and then to Page and Slice which require a Pageable, although it gets more complicated at this point because Page and Slice might not return the same type (a repository method that is annotated @Query and uses query language can return a different type than the entity with the attributes that are the sort criteria), meaning that now Page and Slice methods that return Pageable need a way of returning that Pageable as one for the entity class rather than the result type.

To get rid of warnings and avoid the need to suppress them, we need to eliminate usage of variable arguments (...) in a number of places. I first tried replacing that with Iterable<Sort<MyEntityClass>>, but it wasn't a very distinguishable as a special parameter type, so I added a Sorts class for that purpose. Initially constructing Sort instances with static methods also becomes a bit awkward because the entity class isn't naturally supplied yet, requiring usage like Sort.<MyEntityClass>asc("myAttributeName") or it could have been done asSort.asc(MyEntityClass.class, "myAttributeName") (This pull first went with the former, then switched to a different approach). Using the static metamodel avoids that problem. Preserving the type on Pageable was another problem. I moved where sorts are supplied to a Pageable to the initial static method, from which the entity type is inferred from the Sort instances, and that helps in some cases, but in other cases there is still a need to clarify the type with <MyEntityClass>, which could be annoying to the user. This was later changed to using a separate Order<MyEntityClass> to collect parameterized sorts and created a parameterized page request from that.

[otaviojava]

Instead of breaking the whole API, I would create a wrapper of this sort.

Also, I would create it as an extension in a vendor first.

[njr-11]

Also, I would create it as an extension in a vendor first.

The whole reason I am trying this out is because Gavin pointed out that Hibernate already has the ability to parameterize their equivalent of Sort with the entity type. So I want to explore what it would look like in Jakarta Data.

Instead of breaking the whole API, I would create a wrapper of this sort.

You make that sound so easy, but in practice I do not see how what you suggest is even possible. What the exploration under this pull has already shown is that Sort is tightly coupled to Pageable, and in turn to Slice, and Page, and so forth. I think it was important to illustrate that as we evaluate whether or not it's a good idea.

[otaviojava]

Also, I would create it as an extension in a vendor first.

The whole reason I am trying this out is because Gavin pointed out that Hibernate already has the ability to parameterize their equivalent of Sort with the entity type. So I want to explore what it would look like in Jakarta Data.

Nice, I mean, implementation is based on a Jakarta Data implementation itself. I hope that I can start to help with the static model at the end of February.

You make that sound so easy, but in practice I do not see how what you suggest is even possible. What the exploration under this pull has already shown is that Sort is tightly coupled to Pageable, and in turn to Slice, and Page, and so forth. I think it was important to illustrate that as we evaluate whether or not it's a good idea.

It is not trivial, but we can do it with a wrapper, I would start with:

interface ParametrizedSorted<T> extends Supplier<Sort>{
}

Mainly because we want to use if static model.

[njr-11]

It is not trivial, but we can do it with a wrapper, I would start with:

interface ParametrizedSorted<T> extends Supplier<Sort>{
}

Mainly because we want to use if static model.

That isn't something I can prototype for you because I don't have an understanding of how that would fit into Pageable and the existing API. If you want to write a pull of your own that illustrates your idea and shows how it fits in, go for it.

[KyleAure]

I'm not sure the type safety in this case is worth it.
It seems to me that the static metamodel already gives us a pretty strong coupling between entity and attribute, such that, the following code seems natural to write:

employees.findByYearHired(2024, Limit.of(5), _Employee.lastName.asc(), _Employee.firstName.asc());

Also I didn't even know that this was possible or what the meaning of it is in java:

Sorts.<Employee> of(Sort.asc("id"))

It seems unreasonable for us to require users to write code this way, and worse if they aren't using an IDE they might not even see the type safety warning until they compile anyway.

If we do go down this route, however, I think we need a new name for Sorts.of().
The word sorts has an alternative meaning in English that refers to a general collection, and not one that is sorted.
It would be natural (yet weird) to say "I would like to buy long sorts of pants."

[njr-11]

Excellent comments Kyle - thanks!
I'm not sure either if the extra type safety is going to be worth the trade-off, but I want to be sure it is something we have at least made a good effort on when considering it, so I have added in all of your feedback.

Sorts.<Employee> of(Sort.asc("id"))

It seems unreasonable for us to require users to write code this way, and worse if they aren't using an IDE they might not even see the type safety warning until they compile anyway.

Good point - I had been wondering if that would be too unfamiliar. I switched the approach to follow the style of providing the class instead:

Pageable<Employee> pageRequest = Pageable.of(Employee.class).page(1).size(20).sortBy(
                                   Sort.asc("lastName"),
                                   Sort.asc("firstName"),
                                   Sort.asc("id"));

In the above case, .of(Employee.class) is the additional code that assigns a type if you prefer to supply string values or constants.

If we do go down this route, however, I think we need a new name for Sorts.of(). The word sorts has an alternative meaning in English that refers to a general collection, and not one that is sorted. It would be natural (yet weird) to say "I would like to buy long sorts of pants."

That's another good point: Sorts.of could be misread as "types of". I switched it to Sorting.by(...).

List<Employee> found = findByYearHired(2024, Sorting.by(Sort.asc("lastName"),
                                                        Sort.asc("firstName"),
                                                        Sort.asc("id")));

In the above case, Sorting.by( is the additional code that enables the type to be inferred from the repository method signature List<Employee> findByYearHired(int yearHired, Sorting<Employee> sortBy) if you prefer to supply string values or constants.

Here are some other examples where the static metamodel is used instead:

Pageable<Employee> pageRequest = Sorting.by(_Employee.lastName.asc(),
                                            _Employee.firstName.asc(),
                                            _Employee.id.asc()),
                                        .page(1)
                                        .size(20);

In the above case, this ends up being just as concise as currently, but does involve starting from the `Sorting.by` rather than `Pageable.of...`.

List<Employee> found = findByYearHired(2024, Sorting.by(_Employee.lastName.asc(),
                                                        _Employee.firstName.asc(),
                                                        _Employee.id.asc()));

The above very closely matches the corresponding scenario with string values/constants. Sorting.by( is still needed to supply multiple Sort instances because the variable args Sort... approach isn't possible anymore.

[gavinking]

Sorry for not responding sooner. So here's what the API looks like in Hibernate. First, the usage:

// finder method
repo.getBooksByTitle("%Hibernate%", desc(Book_.isbn))

// query method returning an entity type
repo.findBooksByTitle("%Hibernate%", first(10), asc(Book_.isbn))
repo.findBooksByTitle("%Hibernate%", first(10), asc(Book_.isbn), desc(Book_.publicationDate))

// query method returning a projection
repo.findBookAttributesByTitle("%Hibernate%", first(10), asc(1), asc(2))

where the query methods are declared as follows:

@Find
List<Book> getBooksByTitle(String title, Order<Book> ord);

@HQL("where title like :title")
List<Book> findBooksByTitle(String title, Page page, Order<? super Book>... ordering);

@HQL("select title, isbn from Book where title like :title")
List<Object[]> findBookAttributesByTitle(String title, Page page, Order<Object[]>... ordering);

A couple of observations:

1. My Page class is not generic, but OTOH it doesn't do everything that Jakarta Data's Pageable stuff does.
2. My Order is the equivalent of your Sort.

[gavinking]

[Of course, you're also allowed to write asc(Book.class, "isbn"), but that's sort of "clearly worse".]

[gavinking]

Oh and if you're concerned about the stupid and useless "heap pollution" warning, this is also allowed:

@HQL("where title like ?1")
repo.findBooksByTitle("%Hibernate%", first(10), asList(asc(Book_.isbn), desc(Book_.title)))

List<Book> findBooksByTitle(String title, Page page, List<Order<? super Book>> ordering);

[njr-11]

Oh and if you're concerned about the stupid and useless "heap pollution" warning, this is also allowed:

Thanks for responding. Yes, most of the experimentation I've been doing under this pull is aimed at trying to figure out how to spare customers from that annoying warning and the other warning you get when trying to use the repository method. Your strategy of using Arrays.asList to avoid the warning is very similar to the Sorting.by approach that I tried - they basically do the same thing in allowing multiple to be specified without the warning. Maybe Order.by would be an even better name.

@otaviojava @gavinking @KyleAure will it be possible for all of you to make it to the February 7th Jakarta Data meeting so that we can all discuss/decide whether or not to add the type parameter?

[gavinking]

Your strategy of using Arrays.asList to avoid the warning is very similar to the Sorting.by approach that I tried - they basically do the same thing in allowing multiple to be specified without the warning. Maybe Order.by would be an even better name.

Yep, I agree, they're the same.

Another possibility would be to always declare the methods as:

@Find
List<Book> getBooksByTitle(String title, Sort<Book> ordering);

But let you call it as:

getBooksByTitle("Jakarta Data", _Book.isbn.asc())

or:

getBooksByTitle("Jakarta Data", order(_Book.title.asc(), _Book.isbn.asc()))

Where the static method order() is defined as follows:

	@SafeVarargs
	public static <T> Sort<T> order(Sort<T>... orders) {
		return .... ;
	}

That is, it aggregates multiple Sorts into a single one.

This is quite clean, it seems to me.

[otaviojava]

I'm not sure the type safety in this case is worth it. It seems to me that the static metamodel already gives us a pretty strong coupling between entity and attribute, such that, the following code seems natural to write:

I have the same concern as well.

Anyway, could we at least don't break the whole core code doing it?

I mean, why not create an adapter or wrapper to Sort instead of breaking the whole API?

Let's create a ParametizedSort interface and go with it.

I agree that is not trivial, but it is not that hard.

[njr-11]

Anyway, could we at least don't break the whole core code doing it?

I mean, why not create an adapter or wrapper to Sort instead of breaking the whole API?

Let's create a ParametizedSort interface and go with it.

I agree that is not trivial, but it is not that hard.

My experience of first attempting to make a more isolated change is that I was unable to accomplish it because these interfaces are all tied together. If you have a way to achieve it in a more isolated way, please submit a pull of your own to demonstrate your idea.

What is currently under this pull is the best I have to offer.

At the next meeting, I would like us all to decide between the approach I have demonstrated under the pull, versus the approach you are suggesting (assuming you can demonstrate it as a complete and working solution), versus not doing anything at all and leaving the specification as is. Or maybe someone else will have other ideas that have not been explored yet.

[otaviojava]

At the next meeting, I would like us all to decide between the approach I have demonstrated under the pull, versus the approach you are suggesting (assuming you can demonstrate it as a complete and working solution), versus not doing anything at all and leaving the specification as is. Or maybe someone else will have other ideas that have not been explored yet.

IMHO: I like the Parametized approach at static metadata, but it should not break what we have. So if it does not break what we have is a good candidate, otherwise, we should think more. Jakarta Data is not a race to include as much features as possible, thus, it is fine if we don't include at the version 1.0.0 and bring as a best approach.

[njr-11]

This pull is ready now. The first commit is the same as what was shown on the Jakarta Data call. The second and third commits include the corresponding updates to the JavaDoc and spec doc. The fourth commit includes the suggestion which Gavin made on the call to add the more concise approach for appending generic sorts to a page request: Pageable.ofSize(20).desc("price").asc("productName").asc("id");

[KyleAure]

👏🏼 Thanks @njr-11 for putting this all together. Just a few typos and one clarification needed.

[KyleAure]

Might need to expand on this a bit.
Does ignoring case here mean we ignore the case of the property or the returned results?

• if we call ascIgnoreCase("id") will we find and sort by id,Id, ID, and/or iD?
• if we call ascIgnoreCase("firstName") will we ignore case when sorting strings like (Kacy, Kyle, kim) -> (Kacy, kim, Kyle)

[njr-11]

It's the latter. I was hoping to rely on the link to explain it better. I'll see what else I can add to these.

[njr-11]

I added the clarification here:
09dece6

[KyleAure]

👍🏼 Looks good to me

[gavinking]

Excellent, this looks great!