Introduce API to rewrite JPQL/native query for Sort and PageRequest

[mp911de]

Right now, we face several tickets where we run into issues when trying to rewrite a query to append ORDER BY fields and pagination.

The initial idea of providing a query parser introduces a lot of complexity to parse JPQL queries and more complexity when parsing native SQL queries as we are not in full control of each dialect nor it makes sense to have a full parser that considers the complete SQL specification.

To address the mentioned issues (see #2079, #2045, #1895, and others), we should investigate whether we can provide a callback API where the calling code can handle the actual rewrite. Calling a @Query annotated method accepting either Sort or Pageable arguments, we would activate the query rewrite callback for rewriting the query. The code path utilizing QueryUtils would be entirely skipped so that the rewrite responsibility is within the calling code.

As starting point, we could have a @QueryRewrite annotation accepting a Java class that implements the callback API, something along the lines of:

@FunctionalInterface
interface QueryRewriter {
    String rewrite(String query, Sort sort);

    default rewrite(String query, Pageable pageable) {
        return rewrite(query, pageRequest.getSort());
    }
}

to be used as:

interface MyRepository extends CrudRepository<Foo, String> {
    @Query(…, nativeQuery = true)
    @QueryRewrite(MyQueryRewriter.class)
    List<Foo> myFindAllMethod(Pageable pageable);
}

Following that idea, one could also write:

interface MyRepository extends CrudRepository<Foo, String>, QueryRewriter {
    @Query(…, nativeQuery = true)
    @QueryRewrite(QueryRewrite.Repository.class)
    List<Foo> myFindAllMethod(Pageable pageable);

    default String rewrite(String query, Sort sort) {
        return …;
    } 
}

where QueryRewrite.Repository.class can be a marker to indicate that the actual repository interface contains the actual method implementation.

[gregturn]

@mp911de,

My current implementation can handle:

public interface UserRepository extends Repository<User, Integer> {

	@Query(value = "select u.* from User u", nativeQuery = true)
	@QueryRewrite(TestQueryRewriter.class)
	List<User> findByNativeQuery(String param);

	@Query(value = "select u.* from User u")
	@QueryRewrite(TestQueryRewriter.class)
	List<User> findByNonNativeQuery(String param);

	@Query(value = "select u.* from User u")
	@QueryRewrite(TestQueryRewriter.class)
	List<User> findByNonNativeSortedQuery(String param, Sort sort);

	@Query(value = "select u.* from User u")
	@QueryRewrite(TestQueryRewriter.class)
	List<User> findByNonNativePagedQuery(String param, Pageable pageable);
}

static class TestQueryRewriter implements QueryRewriter {
	@Override
	public String rewrite(String query, Sort sort) {
		return query.replaceAll("u", "user");
	}
}

It supports native/standard, sort params, and pageable params. Because of the way it works, there is no easy way to grab a hold of an existing repository and inject it into the code that generates the SQL statements. So I really don't see a way to do that extension where the repository is ALSO the rewriter.

Basically, JpaQueryFactory appears to be the hook for all query creation, and it doesn't have access to repository beans. Down the road, if we figured out how to get it access, that might be possible. But right now, I have implemented it where the AbstractStringJpaQuery class uses this to transform @QueryRewrite(TestQueryRewriter.class) into an instance...

@Nullable
protected QueryRewriter findQueryRewriter(JpaQueryMethod method) {

	Class<? extends QueryRewriter> queryRewriter = method.getQueryRewriter();

	if (queryRewriter == null) {
		return null;
	}

	try {
		return (QueryRewriter) ((Constructor<?>) queryRewriter.getDeclaredConstructor()).newInstance();
	} catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) {
		System.out.println(e);
		return null;
	}
}

It does a little reflection (which may be an issue for AOT) to take the classname and instantiate an instance using an empty constructor.

[mp911de]

The general instantiation mechanism should leverage BeanFactory.getBean(…)/AutowireCapableBeanFactory.createBean(…)/BeanUtils.instantiate(…) as existing utilities for bean creation. As the code is deeply nested, probably a bean instantiator abstraction (BeanInstantiator extends Function<Class<T>, T>) would make sense as we don't have a BeanFactory in CDI arrangements.

Paging @odrotbohm

[odrotbohm]

If that rewrite is for manually declared queries only, could the rewriter type just be an additional attribute in the @Query annotation. I don't quite get the example around @QueryRewrite(QueryRewrite.Repository.class). That QueryRewrite.Repository type doesn't exist, does it? Shouldn't pointing to the repository work just fine if it implements QueryRewriter in the first place?

That said, I am a bit hesitant with this one for the following reasons:

• Reusing the rewriter for multiple query methods seems hard because implementations will have to guess the actual method executed by inspecting the query to get context.
• Getting the String query passed in basically means having to parse it at least partially. Even in our experience, augmenting the JPQL query strings has been a source of problems, and I wonder if putting that into users hands is just creating a different problem than just writing fragments to implement those queries manually.

I can't quite see a significant advantage in this over implementing a fragment, potentially extracting shared code. Especially if we exposed a flavor of SimpleJpaRepository.readPage(…) so that generally reading pages becomes simple. I guess I'd even wait for that until we see folks presenting actual implementations and evaluate those. A lot of the tickets you linked to could be solved using that approach, and are overdoing even the declared query approach. For example, what would the rewrite logic for #2400 look like, and what would it compare against if the execution was programmed out manually?

[mp911de]

I don't quite get the example around @QueryRewrite(QueryRewrite.Repository.class). That QueryRewrite.Repository type doesn't exist, does it? Shouldn't pointing to the repository work just fine if it implements QueryRewriter in the first place?

You're right, the example is a bit overly complicated, @QueryRewrite(MyRepository.class) would work.

Reusing the rewriter for multiple query methods seems hard because implementations will have to guess the actual method executed by inspecting the query to get context.

Adding an attribute to @Query would be also an alternative instead of introducing another annotation.

Rewriting queries provides a bit of simplification over a fragment. Assembling a parameterized query for pagination along with a count query requires quite some preparation. Query rewriting would allow folks to specify a token within the query (SELECT … %orderby%) and replace that token (query.replaceAll("%orderby%", actualSort)) and react to a query without necessarily parsing the entire query.

Indeed a lot of the tickets above could be solved by providing an explicit count query or by providing a fragment.

[gregturn]

Reusing the rewriter for multiple query methods seems hard because implementations will have to guess the actual method executed by inspecting the query to get context.

For the record, this is simply an artifact of test method simplification. In production, I'd be VERY resistant to reusing a QueryRewriter, but instead prefer a custom one for every method.

The general instantiation mechanism should leverage BeanFactory.getBean(…)/AutowireCapableBeanFactory.createBean(…)/BeanUtils.instantiate(…)

To get the BeanFactory down into the class hierarchy where the QueryRewriter is created seemed a bit invasive. I can tinker with it more.

Getting the String query passed in basically means having to parse it at least partially. Even in our experience, augmenting the JPQL query strings has been a source of problems, and I wonder if putting that into users hands is just creating a different problem than just writing fragments to implement those queries manually.

Maybe I don't follow. I thought the purpose of this proposal was to offer a last-minute escape where the user can tweak the finalized query at the last minute. So instead of adding more fragments and "things", we could instead do all the parsing needed, and then use a QueryRewriter right before the SQL goes to the entityManager.

[gregturn]

@mp911de @odrotbohm

• I have updated this PR so that it now uses a BeanFactory-based approach.
• I have increased the number of tests so we should have a nice, comprehensive suite of verification.
• I have actually made it so you can use either @QueryRewrite(YourQueryRewriter.class) or @Query(value="", queryRewriter=YourQueryRewriter.class).
• I implemented support for independent beans in the app context that implement QueryRewriter OR a repository that implements the same interface. And I have the test cases to prove.

If you have any final comments about which way you'd prefer things, I can trim out the stuff we don't want and proceed to merge.

[gregturn]

The plan, going forward, is to drop @QueryRewrite and instead introduce another attribute to @Query. This will make it easier for the community to discover this new feature without even digging into reference documentation.

We must also extend support for CDI.

[gregturn]

Depends on: spring-projects/spring-data-commons#2614

[gregturn]

spring-projects/spring-data-commons#2614 is deemed unnecessary. Overriding JpaRepositoryFactory.setBeanFactory() provides the perfect hook for us to assign QueryRewriterProvider with a BeanFactory-based version (QueryRewriterBeanFactoryProvider).