Use JSqlParser for parsing SQL

[DiegoKrupitza]

This PR introduces the usage of JSqlParser in QueryUtils. As discussed in #2409 the current parser has few SQL related bugs. In this PR we focus on the most "important" functions of QueryUtils.

This PR is currently just a draft and its main purpose is to get a review of the current state.

Closes: #2409

• You have read the Spring Data contribution guidelines.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.
• You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

[schauder]

Thanks for this PR. I highly appreciate it.

I found a few things that we should change before we merge it though.
Please take a look at my comments.

[mp911de]

My suggestion is to leave QueryUtils where they are (untouched) and instead, introduce a new API defined by an interface. One implementation resorts to using QueryUtils and the other one would use JSqlParser. Part of the motivation is that a lot of methods in QueryUtils are true utilities that do not suffer from any parsing-related drawbacks. Only methods that detect aliases, try to rewrite queries are affected by severe limitations. By introducing a proper abstraction we turn implicit concepts into explicit ones.

[DiegoKrupitza]

My suggestion is to leave QueryUtils where they are

@mp911de
Just to make it more clear. Are you talking about the original QueryUtils (which is now renamed to DefaultQueryUtils) or the new QueryUtils class that either sends the request to DefaultQueryUtils or JSqlParserQueryUtils depending on the existence of JSqlParser and the given query.

[mp911de]

The original QueryUtils as renaming and moving of the class to DefaultQueryUtils create a lot of noise in terms of changes without actually improving the current state.

[DiegoKrupitza]

The new introduced API for all the "enhancing" of a query (that would require to parse and modify the query) is now called QueryEnhancer.
There are two implementations:

1. DefaultQueryEnhancer
2. JSqlParserQueryEnhancer

The first one (DefaultQueryEnhancer) forwards all the request to QueryUtils (the original one)
The second one (JSqlParserQueryEnhancer) forwards all the request to JSqlParserQueryUtils (the new parser only for native ones).

Additionally there is a factory class QueryEnhancerFactory that selects the right QueryEnhancer based on the DeclaredQuery provided and if JSqlParser is in the classpath.

QueryEnhancer has few "default" implementations for String where it is automatically forwarded to QueryUtils.

I hope this is now a cleaner solutions. @mp911de @schauder

[mp911de]

The factory approach makes a lot of sense to be created for a query object. The actual factory implementation would already decide whether to use JSqlParser or the existing approach so no need to carry that fact to the API. Also, creating QueryEnhancer with a query object should allow to not accept another query object in e.g. applySorting. I think we don't need JSqlParserQueryUtils as these implementation details can be part of the JqlParser QueryEnhancer.

I think that toOrders can remain also fully in QueryUtils as there is no parsing involved. Other than that, the design approach is highly appreciated and the only thing that we need to sort out is how or where to configure which QueryEnhancerFactory to use. Let's not worry about configuration, for now, we will eventually get there.

[DiegoKrupitza]

Also, creating QueryEnhancer with a query object should allow to not accept another query object in e.g. applySorting

This is now fixed.

I think we don't need JSqlParserQueryUtils as these implementation details can be part of the JqlParser QueryEnhancer

This is now fixed.

I think that toOrders can remain also fully in QueryUtils as there is no parsing involved

This is now fixed.

@mp911de

[DiegoKrupitza]

This is now the update "design"

[mp911de]

@DiegoKrupitza care to squash your commits into one or at least a few ones so that we can get rid of merge commits?

[DiegoKrupitza]

@DiegoKrupitza care to squash your commits into one or at least a few ones so that we can get rid of merge commits?

Done! @mp911de

[gregturn]

@DiegoKrupitza I'll be shepherding in the last bits of your valiant PR.

I spotted that QueryEnhancer.getExistsQueryString` has no test case, so I wrote one, and strangely got back this:

	private static final String QUERY = "select u from User u";
	@Test
	void queryEnhancerGetExistsQueryShouldReturnBackExistsQuery() {

		QueryEnhancer enhancer = getEnhancer(new StringQuery(QUERY, true));
		String existsQueryString = enhancer.getExistsQueryString("USER", "firstname",
				Collections.singletonList("firstname"));

		assertThat(existsQueryString).isEqualTo("SELECT count(firstname) FROM USER AS x WHERE x.firstname = :firstname");
	}

The thing returns back a COUNT query. Is this what's expected with crafting an EXISTS query in JPQL? (Having a hard time finding what's proper here).

[DiegoKrupitza]

@DiegoKrupitza I'll be shepherding in the last bits of your valiant PR.

That is wonderful to hear!

I spotted that QueryEnhancer.getExistsQueryString` has no test case

That is true. I somehow forgot to add a testcase for this method. The DefaultQueryEnhancer implementation of QueryEnhancer uses directly the already existing QueryUtils.getExistsQueryString(entityName, countQueryPlaceHolder, idAttributes). So I do not think a test for DefaultQueryEnhancer.getExistsQueryString(...) is needed. For the JSqlParserQueryEnhancer a test case would be a good idea.

The thing returns back a COUNT query. Is this what's expected with crafting an EXISTS query in JPQL? (Having a hard time finding what's proper here).

When I implemented the JSqlParser version of getExistsQueryString(String entityName, String countQueryPlaceHolder, Iterable<String> idAttributes) I focused on the already existing method QueryUtils.getExistsQueryString(String entityName, String countQueryPlaceHolder, Iterable<String> idAttributes) that was previously used. So the JSqlParser version basically reflects the same logic as the existing (QueryUtils) one, but just uses JSqlParser components so no Regex and String formatting is required.

As far as I understand QueryUtils.getExistsQueryString(...), creates a COUNT query to check for existence. It even uses the following constant to create the final query.
public static final String COUNT_QUERY_STRING = "select count(%s) from %s x";

I now even checked if QueryUtils and the JSqlParserQueryEnhancer return the same values on your provided test case and they do.

QueryUtils.getExistsQueryString("USER", "firstname", Collections.singletonList("firstname")); 
// =  select count(firstname) from USER x WHERE x.firstname = :firstname

QueryEnhancer enhancer = getEnhancer(new StringQuery(QUERY, true));
String existsQueryString = enhancer.getExistsQueryString("USER", "firstname", Collections.singletonList("firstname"));
// =  SELECT count(firstname) FROM USER AS x WHERE x.firstname = :firstname

PS: I just realised that the method getExistsQueryString(...) does not really depend on the actually query that is provided to the QueryEnhancerFactory. Maybe this can be quite confusing and should be extracted somewhere else?

@gregturn

[DiegoKrupitza]

Is this what's expected with crafting an EXISTS query in JPQL

I think a COUNT query is not a bad idea here, since we only work with SQL here (when native queries are used) and not JPQL. Additionally as far as I know there are not many ways to check with pure SQL for existence. (Beside performance optimized SQL queries see here)

[DiegoKrupitza]

I just realised that QueryEnhancer.getExistsQueryString(...) it not really used. On the other side SimpleJpaRepository.existsById(ID id) uses the QueryUtils.getExistsQueryString(...) implementation. Since the QueryEnhancer.getExistsQueryString(...) is actually not directly used it would make sense to delete this.

Note: SimpleJpaRepository.existsById(ID id) does not create a StringQuery (since this would be a waste of time).

[DiegoKrupitza]

@gregturn since the changes are now in main, can I close this PR?

[schauder]

This is fixed by 1fe4cf5