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

Properly handle native queries with SpEL #3109

Closed
wants to merge 3 commits into from
Closed

Conversation

gregturn
Copy link
Contributor

@gregturn gregturn commented Aug 11, 2023

The following repository definition:

public interface EmployeeRepository extends JpaRepository<Employee, Long> {

	@Query(value = "select * from #{#entityName}", nativeQuery = true)
	List<Employee> custom();
}

...wil fail.

We have code that will explicitly switch it from native to non-native, and hence force it into one of the query parsers, which will cause it to fail.

However, it's not as simple as simply retaining the nativeQuery value. It also causes the QueryEnhancerFactory to kick in before parsing the SpEL expression inside. Tools like JSqlParser will fail. Thus, we need to detect it's SpEL nature sooner and properly mark it, so it's handled correctly.

This PR alters the visibility of a couple private items, but only makes them package-private.

Native queries with SpEL expressions should be properly parsed instead of converted to non-native queries.

See #3096
@gregturn gregturn added type: regression A regression from a previous release in: repository Repositories abstraction labels Aug 11, 2023
@gregturn gregturn added this to the 3.1.3 (2023.0.3) milestone Aug 11, 2023
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, this PR isn't fitting to our existing code. AbstractStringBasedJpaQuery already creates a ExpressionBasedStringQuery that we IMO should render properly to avoid downstream issues.

Adding more context to a DeclaredQuery factory adds more dependencies that aren't within the actual scope of the query wrapper.

We should rather eagerly render the query so that downstream components work with the already resolved expressions.

* @param method is a {@link JpaQueryMethod} that has the related metadata
* @return a {@literal DeclaredQuery} instance even for a {@literal null} or empty argument.
*/
static DeclaredQuery of(@Nullable String query, Boolean nativeQuery, @Nullable JpaQueryMethod method) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method signature introduces a cycle as JpaQueryMethod uses DeclaredQuery already. Why don't we resort to EntityInformation as parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tweaked that method signature to instead pass in JpaEntityMetadata.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 6, 2023

Generally, this PR isn't fitting to our existing code. AbstractStringBasedJpaQuery already creates a ExpressionBasedStringQuery that we IMO should render properly to avoid downstream issues.

The problem is that our current flow of processing a query simply doesn't get that far before it tries to extract the QueryEnhancer, which causes JSqlParser to break on the SpEL expression.

Screenshot 2023-09-06 at 4 44 35 PM

I'm actually trying to get in front of the call to QueryEnhancerFactory.forQuery("select * from #{#entityName}") and slide in an ExpressionBaseQueryParser. So yes, it is a little hacky. Let me re-read the code a little closer, and maybe I can restructure things such that we do this overall process sooner, or at least delay finding the QueryEnhancer for a given query until AFTER we have established what type of query it is.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 7, 2023

@mp911de I drafted an alternative solution over at #3145 that is much less intrusive. What do you think?

@gregturn gregturn closed this Sep 12, 2023
@gregturn gregturn deleted the issue/gh-3096 branch September 12, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: repository Repositories abstraction type: regression A regression from a previous release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nativeQuery "SELECT * FROM [...]" in parent interface throws grammar exception
2 participants