spring-projects / spring-data-commons

Spring Data Commons. Interfaces and code shared between the various datastore specific implementations.
https://spring.io/projects/spring-data
Apache License 2.0
778 stars 675 forks source link

SpelQueryContext errors on unbalanced quotes in SQL statement comments #2943

Open roookeee opened 1 year ago

roookeee commented 1 year ago

Given the following Kotlin @Repository query for spring-data-jdbc:

    @Query(
        """
        -- don't do this
        SELECT 1
        """
    )
    fun test(): Int

We get the following error after upgrading from Spring Boot 2.x to 3.x:

java.lang.IllegalArgumentException: The string <
        -- don't do this
        SELECT 1
        > starts a quoted range at 15, but never ends it.
    at org.springframework.data.repository.query.SpelQueryContext$QuotationMap.<init>(SpelQueryContext.java:341) ~[spring-data-commons-3.1.3.jar:3.1.3]
    at org.springframework.data.repository.query.SpelQueryContext$SpelExtractor.<init>(SpelQueryContext.java:209) ~[spring-data-commons-3.1.3.jar:3.1.3]
    at org.springframework.data.repository.query.SpelQueryContext.parse(SpelQueryContext.java:113) ~[spring-data-commons-3.1.3.jar:3.1.3]
    at org.springframework.data.repository.query.SpelQueryContext$EvaluatingSpelQueryContext.parse(SpelQueryContext.java:172) ~[spring-data-commons-3.1.3.jar:3.1.3]
    at org.springframework.data.jdbc.repository.query.StringBasedJdbcQuery.processSpelExpressions(StringBasedJdbcQuery.java:163) ~[spring-data-jdbc-3.1.3.jar:3.1.3]
    at org.springframework.data.jdbc.repository.query.StringBasedJdbcQuery.execute(StringBasedJdbcQuery.java:140) ~[spring-data-jdbc-3.1.3.jar:3.1.3]
    at org.springframework.data.repository.core.support.RepositoryMethodInvoker.doInvoke(RepositoryMethodInvoker.java:136) ~[spring-data-commons-3.1.3.jar:3.1.3]
    at org.springframework.data.repository.core.support.RepositoryMethodInvoker.invoke(RepositoryMethodInvoker.java:120) ~[spring-data-commons-3.1.3.jar:3.1.3]

I know it's pretty unusual to have comments in production queries, but if the query becomes long (e.g. 50 lines), adding a Java / Kotlin comment above said query is too non-local, our use case comments a specific condition which is hard to understand without such a local comment.

Nevertheless, this is a regression from Spring Boot 2.x which should be fixed or documented to not work anymore.

EDIT: This will probably also error in /* */ SQL comments.

mp911de commented 1 year ago

Thanks for reporting the issue. We should consider comment blocks when looking for quotes. While it is a regression for the JDBC module, this has never worked for SpelQueryContext.

dev-jonghoonpark commented 5 months ago

@mp911de @schauder I think adding a method to the SpelExtractor class to filter comments like below would solve it. Do you mind if I modify it?

SpelExtractor(String query) {

    Assert.notNull(query, "Query must not be null");

    query = filteringComments(query);

    Map<String, String> expressions = new HashMap<>();
    Matcher matcher = SPEL_PATTERN.matcher(query);
    ...
}
dev-jonghoonpark commented 5 months ago

Gentle ping to @schauder, Hi! Can I write a PR with this approach?

mp911de commented 5 months ago

We require some sort of comment processing (single-line and multi-line comments) to pre-process the query, or better, to ignore sections. In some cases, comments should be sent to the database. Commons isn't opinionated on the actual database technology, so we should provide a facility to configure comments. The actual comment syntax should be specified in the actual module that uses SpEL processing.

Going forward, SpelQueryContext will be deprecated as part of #3050 and replaced by ValueExpression support which is mostly a refactored copy of SpelQueryContext.

dev-jonghoonpark commented 5 months ago

Thank you for being very specific. I hope you have a nice day.

I will try to contribute at another opportunity.