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

SpelQueryContext errors on unbalanced quotes in SQL statement comments #2943

Open
roookeee opened this issue Sep 27, 2023 · 5 comments
Open
Assignees
Labels
type: bug A general bug

Comments

@roookeee
Copy link

roookeee commented Sep 27, 2023

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 27, 2023
@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 27, 2023
@mp911de mp911de self-assigned this Sep 27, 2023
@mp911de
Copy link
Member

mp911de commented Sep 27, 2023

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.

@mp911de mp911de assigned schauder and unassigned mp911de Sep 27, 2023
@dev-jonghoonpark
Copy link

dev-jonghoonpark commented Jun 4, 2024

@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
Copy link

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

@mp911de
Copy link
Member

mp911de commented Jun 11, 2024

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
Copy link

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

I will try to contribute at another opportunity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants