HHH-19364 - Introduce QuerySpecification

[sebersole]

Initial work on a proper solution to creating "query builders".

Initial commit focuses on HQL-based builders. Still need to add criteria-based, though maybe not for 7.0.

A few notes -

• See note in DynamicSelectionHqlImpl#createQuery and DynamicMutationHqlImpl#createQuery wrt copied code. Need to address that duplication.
• Not sure QueryInterpretationCache#cacheHqlInterpretation is really worth it.
• Class<T> mutationTarget argument for QueryProducer#createDynamicMutation - ran into compiler errors without it, but would be nice to remove it.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

https://hibernate.atlassian.net/browse/HHH-19364

[gavinking]

Also need something here to replace:

KeyedResultList<R> getKeyedResultList(KeyedPage<R> page);

But just putting it on this interface doesn't work at all, because there's no way to bind arguments to query parameters.

So the solution has to be to split KeyedPage up so that the keyDefinition is no longer part of the KeyedPage object. I already convinced them to make a similar change in the Data spec itself (before 1.0). So this is something I had been considering doing anyway. [The current definition of the KeyedPage object still sorta reflects the previous design the Data group had before I got involved.]

So I guess it would mean adding this method:

DynamicSelection<T> setKeyDefinition(List<Order<? super T>> keyDefinition);

which sets up the order and the restrictions required. This implies changing KeyBasedPagination.paginate() to set up parameters instead of just using the keys directly.

Now what would this look like from the user point of view? I guess it would be:

KeyedResultList<Person> firstPage =
        session.createTailoredSelection("from Person where name like :name", Person.class)
                .setOrder(Order.asc(Person_.ssn))  // not setKeyDefinition()
                .createQuery()
                .setParameter("name", name)
                .setPage(Page.first(5))  // not setKeyedPage()
                .getKeyedResultList();

KeyedResultList<Person> nextPage =
        nextsession.createTailoredSelection("from Person where name like :name", Person.class)
                .setKeyDefinition(Order.asc(Person_.ssn))
                .createQuery()
                .setParameter("name", name)
                .setKeyedPage(firstPage.getNextPage())
                .getKeyedResultList();

Note that setKeyedPage() now binds the query parameters we created during the call to setKeyDefinition().

So, OK, fine, what is the blast radius here:

1. Remove keyDefinition from KeyedPage
2. Add setKeyDefinition() to this new API, and change the implementation of KeyBasedPagination.paginate() to use parameter objects
3. Replace SelectionQuery.getKeyedResultList(KeyedPage) with setKeyedPage(KeyedPage) which does the parameter bindings and getKeyedResultList() which constructs the `KeyedResultList.
4. Update Hibernate Processor.

I think that is all more or less OK. The API is arguably somewhat less ergonomic, but it's also more consistent, and it goes in a direction I had already been playing with in my head.

[beikov]

We could bind the keyset values through our JPA Criteria value() API or simply bind the parameters automatically when invoking createQuery(), so the ergonomics could stay the same.

[gavinking]

We could bind the keyset values through our JPA Criteria value() API or simply bind the parameters automatically when invoking createQuery(), so the ergonomics could stay the same.

So Steve and I had talked about doing it something approximately like this:

KeyedResultList<Person> firstPage =
        session.createTailoredSelection("from Person where name like :name", Person.class)
                .createQuery(Page.first(5).keyedBy(Order.asc(Person_.ssn)))
                .setParameter("name", name)
                .getKeyedResultList();

KeyedResultList<Person> nextPage =
        nextsession.createTailoredSelection("from Person where name like :name", Person.class)
                .createQuery(firstPage.getNextPage())
                .setParameter("name", name)
                .getKeyedResultList();

That's still an option, certainly, but I guess I feel like it sits in a bit of an uncomfortable middle ground between the other two approaches, being neither one nor the other.

The big problem I have with that is it would not be legit to call .getKeyedResultList() on a Query which had not been created in a very specific way. Whereas with the approach I proposed above, you could essentially (perhaps with some restrictions) call getKeyedResultList() on any query with an order.

Of course we could solve that "big problem" by introducing KeyedSelectionQuery as a subtype of SelectionQuery and perhaps that's worth considering.

I guess the question there really is: which direction do we want to take KeyedPage?

• Do we want it to carry the key definition, or
• do we want to decouple it like in the Data spec?

I have not really been able to make up my mind about that one.

[gavinking]

Actually, there's another option, which I had thought about in the context of the spec, but which didn't really make sense for repositories, but might indeed be the right thing here. The idea is to have KeyedPage carry around the query parameters.

So the usage might look like this:

KeyedResultList<Person> firstPage =
        session.createTailoredSelection("from Person where name like :name", Person.class)
                .setOrder(Order.asc(Person_.ssn))
                .createQuery()
                .setParameter("name", name)
                .setPage(Page.first(5))
                .getKeyedResultList();

KeyedResultList<Person> nextPage = 
        firstPage.getNextPage()
                .getKeyedResultList(nextsession);

And of course you would not even need to use setOrder(), and could just write:

KeyedResultList<Person> firstPage =
        session.createSelectionQuery("from Person where name like :name order by ssn", Person.class)
                .setParameter("name", name)
                .setPage(Page.first(5))
                .getKeyedResultList();

KeyedResultList<Person> nextPage = 
        firstPage.getNextPage()
                .getKeyedResultList(nextsession);

This way, we would not need to expose anything about key-based pagination on the new API, all we would need is SelectionQuery.getKeyedResultList().

I think this is a good way forward.

[beikov]

The idea is to have KeyedPage carry around the query parameters.

I don't think this will work for the very common use case of HTTP endpoints, where you have to serialize/deserialize this "cursor" somehow. From looking at the code, it feels like KeyedResultList is something that is actively connected to a query, which would be very heavyweight to serialize and from a security PoV should not be deserialized.

Of course we could solve that "big problem" by introducing KeyedSelectionQuery as a subtype of SelectionQuery and perhaps that's worth considering.

A dedicated KeyedSelectionQuery sounds more appropriate to me.

[sebersole]

Or how about:

interface DynamicSelection {
    ...

    SelectionQuery createQuery();
    KeyedResultList getKeyedResultList(KeyedPage keyedPage);
}

[gavinking]

though perhaps the Limit and Page forms are overkill.

We don't have Limit in Hibernate APIs. That's just setMaxResults() for us.

And setPage() is perfectly fine where it is right now. It's just a convenient way to call setMaxResults()/setFirstResult().

[sebersole]

Sure, but those comments are more about KeyedPage / KeyedResultList.

What I absolutely do not want is to have this stuff on SelectionQuery and have the expectation that it mutates the internal state of that SelectionQuery. That internal state of SelectionQuery ought to be immutable once created; and to me that includes the SQM tree.

However we achieve that.

[sebersole]

Yes, yes, I know we cannot revert SelectionQuery from being mutable in the short term because of the methods previously added there until we can remove those previously added methods.

But we can proceed with that expectation.

[gavinking]

Alright so at this point I'm pretty happy with this:

var firstPage =
	session.createSelectionQuery("from Person where dob > :dob order by ssn", Person.class )
		.setParameter( "dob", LocalDate.of(1970, 2, 1) )
		.setPage( Page.first( 10 ) )
		.getKeyedResultList();
firstPage.getResultList()
	.forEach(p -> ... );

var secondPage =
	session.createSelectionQuery( "from Person where dob > :dob order by ssn", Person.class )
		.getKeyedResultList( firstPage.getNextPage() );
secondPage.getResultList()
	.forEach(p -> ... );

which I actually already have implemented at a POC level.

[hibernate-github-bot]

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

[gavinking]

Suggested change

	MutationQuery createQuery();
	@Override
	MutationQuery createQuery();

[gavinking]

Suggested change

	SelectionQuery<T> createQuery();
	@Override
	SelectionQuery<T> createQuery();