Try to optionally use JSqlParser for parsing SQL #2409

schauder · 2022-01-17T14:18:24Z

https://github.com/JSQLParser/JSqlParser Seems to be a decent solution to the problem of SQL parsing.
We also have a lot of issue related to SQL and JPQL parsing.

Therefore we want to:

implement everything related to SQL parsing using JSqlParser
use that implementation when JSqlParser is on the classpath.

The downside of having two parser implementations is, that it might get confusing, what is going on for the user.
Therefore we should:

During startup log an INFO message what parser we are using.
If possible define well defined limits of the standard parser like: We can't parse anything with nested parenthesis and mention that information in documentation and in error messages.

DiegoKrupitza · 2022-01-21T08:47:56Z

I think having two different parsers, as you already said, might confuse users. That's why I was thinking to first introduces the new parsers and deprecate the old one.
So far as I can see the old parser could be "translated" into the new framework (I did not went into details but it seems possible).
Therefore if it can really replace the old custom implementation deprecation and later removal should be considered.

schauder · 2022-01-21T09:30:49Z

There are two problems with this:

We also have to parse JPQL, which is a completely different story.
We don't want to require an external library. It is a basic principle we follow to minimise required dependencies to third party libraries.

DiegoKrupitza · 2022-01-22T08:49:32Z

ok this makes sense.
I will try to take a look at this enhancement. I would say I start with something simple such as org.springframework.data.jpa.repository.query.QueryUtils. Since here we do a lot of Regex matching etc just to get and detect certain parts of an SQL query and introducing JSqlParser here would bring a big benefit in terms of fault prevention, readability etc.

It will take some time since right now it is exam season.

We introduced `JSQLParser/JSqlParser` to the project so SQL can be easily parsed with out any hassle. This commit only focuses on the first two methods inside `JSqlParserQueryUtils` that is the JSqlParser counterpart of `QueryUtils`. Related tickets spring-projects#2409

This commit only focuses on sorting methods inside `JSqlParserQueryUtils` that is the JSqlParser counterpart of `QueryUtils`. Related tickets spring-projects#2409

Inside `JSqlParserQueryUtils` it is now possible to create count queries with `createCountQueryFor(...)`. Furthermore, the `getProjection(...)` method utilizes the power of the JSQlParser. Related tickets spring-projects#2409

DiegoKrupitza · 2022-01-22T18:36:17Z

So I now translated the most important methods from org.springframework.data.jpa.repository.query.QueryUtils into their JSqlParser counterpart. All the changes are inside org.springframework.data.jpa.repository.query.JSqlParserQueryUtils.

So far I can see I did not miss anything SQL related but it would be nice if someone could look over it.
Furthermore, it is needed to somehow use JSqlParserQueryUtils over QueryUtils when it is a SQL query. I could not figure out how to do this within the spring-data-jpa ecosystem (prob due to missing project related knowledge). Maybe someone can help me there (or if it is faster maybe hook into my current state and "finish" this part).

DiegoKrupitza · 2022-01-24T08:38:10Z

Although JSqlParser is great for parsing the SQL there are still some limitations. I think this should be included into the final documentation. Due to this "bug" I had to adjust few test cases.

JSQLParser/JSqlParser#1462

schauder · 2022-01-24T08:48:24Z

Furthermore, it is needed to somehow use JSqlParserQueryUtils over QueryUtils when it is a SQL query.
Here is how I would approach this:

For every method in QueryUtils accessed from outside the class, create a new variant, that takes a StringQuery class.
StringQuery combines just two fields: The query string and a flag denoting if it is a native query.
deprecate the old methods that use strings directly.
cascade the change through the code base. This will lead you to all the places where query strings come into the system.
In QueryUtils delegate to JSqlParserQueryUtils for native queries.
extract the actual work of QueryUtils into another class (SimpleParserQueryUtils ???).

HTH

schauder · 2022-01-24T08:56:26Z

Although JSqlParser is great for parsing the SQL there are still some limitations.
As long as it isn't worse then what we use now this shouldn't be to much of a problem.
Thanks for raising an issue with them.

schauder · 2022-01-24T08:57:11Z

So far I can see I did not miss anything SQL related but it would be nice if someone could look over it.
Could you create PR for that?

DiegoKrupitza · 2022-01-24T08:58:14Z

Furthermore, it is needed to somehow use JSqlParserQueryUtils over QueryUtils when it is a SQL query.
Here is how I would approach this:

For every method in QueryUtils accessed from outside the class, create a new variant, that takes a StringQuery class.

StringQuery combines just two fields: The query string and a flag denoting if it is a native query.

deprecate the old methods that use strings directly.

cascade the change through the code base. This will lead you to all the places where query strings come into the system.

In QueryUtils delegate to JSqlParserQueryUtils for native queries.

extract the actual work of QueryUtils into another class (SimpleParserQueryUtils ???).

HTH

Ok thank you! I will take a look at this later! 👍

PS: the first line of your response is still within the "quote" of my message

DiegoKrupitza · 2022-01-24T09:01:59Z

Although JSqlParser is great for parsing the SQL there are still some limitations.
As long as it isn't worse then what we use now this shouldn't be to much of a problem.
Thanks for raising an issue with them.

I think we should clearly state somewhere that parsing errors could still happen, although the query is valid sql and that this could be due to some internals of the JSqlParser. So that the user knows that a possible short term fix could be to use a different query or use the "custom parser".

DiegoKrupitza · 2022-01-24T09:03:00Z

So far I can see I did not miss anything SQL related but it would be nice if someone could look over it.
Could you create PR for that?

Ok I can open a PR later but it is still a draft.

schauder · 2022-01-24T09:09:03Z

Although JSqlParser is great for parsing the SQL there are still some limitations.
As long as it isn't worse then what we use now this shouldn't be to much of a problem.
Thanks for raising an issue with them.

I think we should clearly state somewhere that parsing errors could still happen, although the query is valid sql and that this could be due to some internals of the JSqlParser. So that the user knows that a possible short term fix could be to use a different query or use the "custom parser".

I understand the urge. But really any piece of non-trivial software has bugs and putting disclaimers in the documentation everywhere makes it just hard to read. If you want it documented, a StackOverflow question and answer might be a better option.

For further processing we somehow need to know if the given `StringQuery` is a native query or not, since based on that information we can decide if we use the JSqlParser for parsing or the custom parser. Related tickets spring-projects#2409

`QueryUtils` can now decide at runtime if a given query can be processed using the JSqlParser or the (default) custom parser implementation, that can parse more than just standard SQL queries. JSqlParser is selected if the query qualifies as a native query (which means it does not use any special features such as SpEL and is marked with `nativeQuery=true`). Related tickets spring-projects#2409

DiegoKrupitza · 2022-01-24T22:17:38Z

Furthermore, it is needed to somehow use JSqlParserQueryUtils over QueryUtils when it is a SQL query.
Here is how I would approach this:

For every method in QueryUtils accessed from outside the class, create a new variant, that takes a StringQuery class.

StringQuery combines just two fields: The query string and a flag denoting if it is a native query.

deprecate the old methods that use strings directly.

cascade the change through the code base. This will lead you to all the places where query strings come into the system.

In QueryUtils delegate to JSqlParserQueryUtils for native queries.

extract the actual work of QueryUtils into another class (SimpleParserQueryUtils ???).

HTH

@schauder
Ok that helped a lot, but I am still a bit unsure if my approach "check if JSqlParser is in class path" is as beautiful implemented as it should be.

Nevertheless all the important methods within QueryUtils are not either using JSqlParser or the default implementation (the custom parser).

The PR is actually completed but before I remove the "DRAFT" I would like to get some feedback since its quite a lot of code and I want to keep the quality high.

PR can be found here: #2417

Related tickets spring-projects#2409

…ts`. Since all the testcases in this class will never qualify as a native query for `StringQuery` we can hardcode the `false`. Related tickets spring-projects#2409

Queries often need to be enhanced for example with an "order by" or a "where" clause and `QueryEnhancer` provides a simple inferface that handles all the enhancements. Since native queries can be parsed and processed more efficiently with JSqlParser there is a `QueryEnhancer` implementation using JSqlParser. On the other side there is a default implementation using the custom written parser `DefaultQueryEnhancer`. If a given query cannot be identified as a native one the `DefaultQueryEnhancer` is used. Related tickets spring-projects#2409

It does not make sense to allow difference queries after initialising the implementation of `QueryEnhancer` with a given `DeclaredQuery`. Therefore, the API now does not allow the usage of a different `DeclaredQuery`. Related tickets spring-projects#2409

Since all the methods within `JSqlParserQueryUtils` were only used by `JSqlParserQueryEnhancer` it makes more sense to just implement it there. Additionally we introduced the `JSqlParserUtils` class which is there for operations that a commonly used when working with JSqlParser (for example generating a count function object). Related tickets spring-projects#2409

We introduced `JSQLParser/JSqlParser` to the project so SQL can be easily parsed with out any hassle. This commit only focuses on the first two methods inside `JSqlParserQueryUtils` that is the JSqlParser counterpart of `QueryUtils`. Related tickets #2409

This PR introduces the usage of JSqlParser in spring-data-jpa for native queries. As discussed in spring-projects#2409 the current parser has few SQL related bugs. Closes spring-projects#2409

Introduce a library that makes it possible to parse native SQL statements. See #2409.

See #2409.

Introduce a library that makes it possible to parse native SQL statements. See #2409.

See #2409.

gregturn · 2022-02-23T19:04:52Z

Merged to main. Thanks @DiegoKrupitza !

Introduce a library that makes it possible to parse native SQL statements. See #2409.

See #2409.

schauder added the type: enhancement A general enhancement label Jan 17, 2022

DiegoKrupitza mentioned this issue Jan 24, 2022

Use JSqlParser for parsing SQL #2417

Closed

4 tasks

DiegoKrupitza added a commit to DiegoKrupitza/spring-data-jpa that referenced this issue Jan 27, 2022

Reverting unrelated issue changes.

9c614a8

Related tickets spring-projects#2409

gregturn added a commit that referenced this issue Feb 18, 2022

GH-2409 - Prepare branch

6aede09

gregturn pushed a commit that referenced this issue Feb 22, 2022

Introduce JSqlParser as option for parsing native queries.

993eeb5

Introduce a library that makes it possible to parse native SQL statements. See #2409.

gregturn added a commit that referenced this issue Feb 22, 2022

Polishing.

466ab36

See #2409.

gregturn added a commit that referenced this issue Feb 22, 2022

Polishing.

31173b2

See #2409.

gregturn added a commit that referenced this issue Feb 23, 2022

Polishing.

f48608a

See #2409.

gregturn pushed a commit that referenced this issue Feb 23, 2022

Introduce JSqlParser as option for parsing native queries.

c4d9bc6

Introduce a library that makes it possible to parse native SQL statements. See #2409.

gregturn added a commit that referenced this issue Feb 23, 2022

Polishing.

8579477

See #2409.

gregturn closed this as completed Feb 23, 2022

gregturn pushed a commit that referenced this issue Feb 24, 2022

Introduce JSqlParser as option for parsing native queries.

1fe4cf5

Introduce a library that makes it possible to parse native SQL statements. See #2409.

gregturn added a commit that referenced this issue Feb 24, 2022

Polishing.

1f29463

See #2409.

gregturn mentioned this issue May 16, 2022

New QueryUtils.hasOrderByClause doesn't find subselect/window order by clause #2496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to optionally use JSqlParser for parsing SQL #2409

Try to optionally use JSqlParser for parsing SQL #2409

schauder commented Jan 17, 2022

DiegoKrupitza commented Jan 21, 2022

schauder commented Jan 21, 2022

DiegoKrupitza commented Jan 22, 2022

DiegoKrupitza commented Jan 22, 2022

DiegoKrupitza commented Jan 24, 2022

schauder commented Jan 24, 2022

schauder commented Jan 24, 2022

schauder commented Jan 24, 2022

DiegoKrupitza commented Jan 24, 2022 •

edited

Loading

DiegoKrupitza commented Jan 24, 2022

DiegoKrupitza commented Jan 24, 2022

schauder commented Jan 24, 2022

DiegoKrupitza commented Jan 24, 2022

gregturn commented Feb 23, 2022

Try to optionally use JSqlParser for parsing SQL #2409

Try to optionally use JSqlParser for parsing SQL #2409

Comments

schauder commented Jan 17, 2022

DiegoKrupitza commented Jan 21, 2022

schauder commented Jan 21, 2022

DiegoKrupitza commented Jan 22, 2022

DiegoKrupitza commented Jan 22, 2022

DiegoKrupitza commented Jan 24, 2022

schauder commented Jan 24, 2022

schauder commented Jan 24, 2022

schauder commented Jan 24, 2022

DiegoKrupitza commented Jan 24, 2022 • edited Loading

DiegoKrupitza commented Jan 24, 2022

DiegoKrupitza commented Jan 24, 2022

schauder commented Jan 24, 2022

DiegoKrupitza commented Jan 24, 2022

gregturn commented Feb 23, 2022

DiegoKrupitza commented Jan 24, 2022 •

edited

Loading