-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add configuration option to disable usage of JSqlParser for native queries. #3623
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,29 +17,29 @@ | |
|
||
import org.apache.commons.logging.Log; | ||
import org.apache.commons.logging.LogFactory; | ||
import org.springframework.core.SpringProperties; | ||
import org.springframework.data.jpa.provider.PersistenceProvider; | ||
import org.springframework.lang.Nullable; | ||
import org.springframework.util.ClassUtils; | ||
import org.springframework.util.StringUtils; | ||
|
||
/** | ||
* Encapsulates different strategies for the creation of a {@link QueryEnhancer} from a {@link DeclaredQuery}. | ||
* | ||
* @author Diego Krupitza | ||
* @author Greg Turnquist | ||
* @author Mark Paluch | ||
* @author Christoph Strobl | ||
* @since 2.7.0 | ||
*/ | ||
public final class QueryEnhancerFactory { | ||
|
||
private static final Log LOG = LogFactory.getLog(QueryEnhancerFactory.class); | ||
|
||
private static final boolean jSqlParserPresent = ClassUtils.isPresent("net.sf.jsqlparser.parser.JSqlParser", | ||
QueryEnhancerFactory.class.getClassLoader()); | ||
private static final NativeQueryEnhancer NATIVE_QUERY_ENHANCER; | ||
|
||
static { | ||
|
||
if (jSqlParserPresent) { | ||
LOG.info("JSqlParser is in classpath; If applicable, JSqlParser will be used"); | ||
} | ||
NATIVE_QUERY_ENHANCER = NativeQueryEnhancer.select(QueryEnhancerFactory.class.getClassLoader()); | ||
|
||
if (PersistenceProvider.ECLIPSELINK.isPresent()) { | ||
LOG.info("EclipseLink is in classpath; If applicable, EQL parser will be used."); | ||
|
@@ -48,7 +48,6 @@ public final class QueryEnhancerFactory { | |
if (PersistenceProvider.HIBERNATE.isPresent()) { | ||
LOG.info("Hibernate is in classpath; If applicable, HQL parser will be used."); | ||
} | ||
|
||
} | ||
|
||
private QueryEnhancerFactory() {} | ||
|
@@ -62,15 +61,7 @@ private QueryEnhancerFactory() {} | |
public static QueryEnhancer forQuery(DeclaredQuery query) { | ||
|
||
if (query.isNativeQuery()) { | ||
|
||
if (jSqlParserPresent) { | ||
/* | ||
* If JSqlParser fails, throw some alert signaling that people should write a custom Impl. | ||
*/ | ||
return new JSqlParserQueryEnhancer(query); | ||
} | ||
|
||
return new DefaultQueryEnhancer(query); | ||
return getNativeQueryEnhancer(query); | ||
} | ||
|
||
if (PersistenceProvider.HIBERNATE.isPresent()) { | ||
|
@@ -82,4 +73,56 @@ public static QueryEnhancer forQuery(DeclaredQuery query) { | |
} | ||
} | ||
|
||
/** | ||
* Get the native query enhancer for the given {@link DeclaredQuery query} based on {@link #NATIVE_QUERY_ENHANCER}. | ||
* | ||
* @param query the declared query. | ||
* @return new instance of {@link QueryEnhancer}. | ||
*/ | ||
private static QueryEnhancer getNativeQueryEnhancer(DeclaredQuery query) { | ||
|
||
if (NATIVE_QUERY_ENHANCER.equals(NativeQueryEnhancer.JSQL)) { | ||
return new JSqlParserQueryEnhancer(query); | ||
} | ||
return new DefaultQueryEnhancer(query); | ||
} | ||
|
||
/** | ||
* Possible choices for the {@link #NATIVE_PARSER_PROPERTY}. Read current selection via {@link #select(ClassLoader)}. | ||
*/ | ||
enum NativeQueryEnhancer { | ||
|
||
AUTO, DEFAULT, JSQL; | ||
|
||
static final String NATIVE_PARSER_PROPERTY = "spring.data.jpa.query.native.parser"; | ||
|
||
private static NativeQueryEnhancer from(@Nullable String name) { | ||
if (!StringUtils.hasText(name)) { | ||
return AUTO; | ||
} | ||
return NativeQueryEnhancer.valueOf(name.toUpperCase()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
||
} | ||
|
||
/** | ||
* @param classLoader ClassLoader to look up available libraries. | ||
* @return the current selection considering classpath avialability and user selection via | ||
* {@link #NATIVE_PARSER_PROPERTY}. | ||
*/ | ||
static NativeQueryEnhancer select(ClassLoader classLoader) { | ||
|
||
if (!ClassUtils.isPresent("net.sf.jsqlparser.parser.JSqlParser", classLoader)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For Graal Native, wouldn't we want to keep this check within a field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the check would still work just not be initialized at build time which is not necessary since its done only once and forgotten afterwards. |
||
return NativeQueryEnhancer.DEFAULT; | ||
} | ||
|
||
NativeQueryEnhancer selected = NativeQueryEnhancer.from(SpringProperties.getProperty(NATIVE_PARSER_PROPERTY)); | ||
if (selected.equals(NativeQueryEnhancer.AUTO) || selected.equals(NativeQueryEnhancer.JSQL)) { | ||
LOG.info("JSqlParser is in classpath; If applicable, JSqlParser will be used."); | ||
return NativeQueryEnhancer.JSQL; | ||
} | ||
|
||
LOG.info("JSqlParser is in classpath but won't be used due to user choice."); | ||
return selected; | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* | ||
* Copyright 2024 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.springframework.data.jpa.util; | ||
|
||
import java.lang.annotation.Documented; | ||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
import org.junit.jupiter.api.extension.ExtendWith; | ||
|
||
/** | ||
* Annotation used to exclude entries from the classpath. Simplified version of <a href= | ||
* "https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-tools/spring-boot-test-support/src/main/java/org/springframework/boot/testsupport/classpath/ClassPathExclusions.java">ClassPathExclusions</a>. | ||
* | ||
* @author Christoph Strobl | ||
*/ | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target({ ElementType.TYPE, ElementType.METHOD }) | ||
@Documented | ||
@ExtendWith(ClassPathExclusionsExtension.class) | ||
public @interface ClassPathExclusions { | ||
|
||
/** | ||
* One or more packages that should be excluded from the classpath. | ||
* | ||
* @return the excluded packages | ||
*/ | ||
String[] packages(); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
SpringProperties
falling back to System Properties, wouldn't we want to keep the selection dynamic to allow changing system property values at runtime?More with the rationale of setting the value later and not so much for flipping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we could do it, I'd rather have it set in stone. Allowing to change values at runtime might lead to interesting side effects in its best case, and we'd have to do the lookup every single time.