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.

Sign up for GitHub

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

Jump to bottom

Introduce QueryEnhancerSelector to configure which QueryEnhancerFactory to use #3527

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Introduce QueryEnhancerSelector to configure which QueryEnhancerFactory to use #3527

wants to merge 4 commits into from
+1,053 −686

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Jun 27, 2024 •
edited
Loading

This is a draft for configuring a QueryEnhancerSelector that selects a QueryEnhancerFactory based on a DeclaredQuery:

@EnableJpaRepositories(queryEnhancerSelector = MyQueryEnhancerSelector.class)

class MyQueryEnhancerSelector extends QueryEnhancerSelector.DefaultQueryEnhancerSelector {
	public MyQueryEnhancerSelector() {
		super(QueryEnhancerFactories.fallback(), DefaultQueryEnhancerSelector.jpql());
	}
}

This change requires decoupling of DeclaredQuery (the declaration aspect) and the introspected part (IntrospectedQuery) to avoid the logical cycle of introspecting a query upon DeclaredQuery creation.

Closes #3622

@mp911de mp911de added the type: enhancement A general enhancement label Jun 27, 2024
schauder
schauder requested changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

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

No major concerns, just some nitpicking here and there.

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryEnhancerFactory.java Outdated

/**
* Creates a new {@link QueryEnhancer} for the given {@link DeclaredQuery}.
* Creates a new {@link QueryEnhancer} for the given {@link IntrospectedQuery}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it say IntrospectedQuery? The API still only mentions DeclaredQuery

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java Outdated
@@ -1,5 +1,5 @@
/*
* Copyright 2018-2024 the original author or authors.
* Copyright 2024 the original author or authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it does matter in any real sense of the word, but we should leave the copyright date as is.

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java
* @author Jens Schauder
* @author Diego Krupitza
* @since 2.0.3
* @author Mark Paluch
Copy link
Contributor

Choose a reason for hiding this comment

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

We should leave the authors intact, even when the class changes completely.

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/DefaultDeclaredQuery.java
/**
* @author Mark Paluch
*/
class DefaultDeclaredQuery implements DeclaredQuery {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a candidate for a record to me

Copy link
Member Author

Choose a reason for hiding this comment

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

I deliberately do not want to use records to avoid performance penalties on GraalVM. Once these are sorted out, we can switch to records.

...a-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryConfiguration.java
*
* @author Mark Paluch
*/
public class JpaQueryConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a record

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not like records being public API.

.../src/main/java/org/springframework/data/jpa/repository/support/JpaRepositoryFactoryBean.java Outdated
private @Nullable EntityManager entityManager;
private EntityPathResolver entityPathResolver;
private EscapeCharacter escapeCharacter = EscapeCharacter.DEFAULT;
private JpaQueryMethodFactory queryMethodFactory;
private @Nullable Function<BeanFactory, QueryEnhancerSelector> queryEnhancerSelector;
Copy link
Contributor

Choose a reason for hiding this comment

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

The name is misleading. queryEnhancerSelectorSource of course we could go with queryEnhancerSelectorSelector ;-)

.../src/main/java/org/springframework/data/jpa/repository/support/JpaRepositoryFactoryBean.java Outdated
*
* @param queryEnhancerSelector must not be {@literal null}.
*/
public void setQueryEnhancerSelector(Class<? extends QueryEnhancerSelector> queryEnhancerSelector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

queryEnhancerSelectorType?

@mp911de mp911de linked an issue Sep 25, 2024 that may be closed by this pull request
Provide an option to disable JSqlParserQueryEnhancer #2989
Closed
@mp911de mp911de mentioned this pull request Sep 25, 2024
Closed
@mp911de mp911de removed a link to an issue Sep 25, 2024
Provide an option to disable JSqlParserQueryEnhancer #2989
Closed
@mp911de mp911de added the has: breaking-change An issue that is associated with a breaking change. label Sep 25, 2024
@mp911de mp911de force-pushed the main branch 2 times, most recently from 52ee55f to 61e6d36 Compare October 9, 2024 07:35
@mp911de mp911de mentioned this pull request Dec 12, 2024
Open
@mp911de
Prepare issue branch.
8b435cc
@mp911de
Hacking.
0ee6c66 
Introduce QueryEnhancerSelector to EnableJpaRepositories.

Also, split DeclaredQuery into two interfaces to resolve the inner cycle of query introspection while just a value object is being created.

Introduce JpaQueryConfiguration to capture a multitude of configuration elements.
@mp911de
Post-Rebase
debdeba
@mp911de
Copy link
Member Author

mp911de commented Dec 12, 2024

I addressed a few review comments and introduced EntityQuery as additional interface. Likely, there's a better name since queries can return projections or aggregations when using native SQL. In any case, we signal that only EntityQuery allows count query derivation.

I think this change still requires some refinement before I can turn it into a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schauder schauder schauder requested changes

@christophstrobl christophstrobl Awaiting requested review from christophstrobl

Assignees
No one assigned
Labels
has: breaking-change An issue that is associated with a breaking change. type: enhancement A general enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor DeclaredQuery to decouple the query definition from its introspected state
2 participants
@mp911de @schauder