-
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
Introduce QueryEnhancerSelector
to configure which QueryEnhancerFactory
to use
#3527
base: main
Are you sure you want to change the base?
Conversation
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.
No major concerns, just some nitpicking here and there.
|
||
/** | ||
* Creates a new {@link QueryEnhancer} for the given {@link DeclaredQuery}. | ||
* Creates a new {@link QueryEnhancer} for the given {@link IntrospectedQuery}. |
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.
Why does it say IntrospectedQuery
? The API still only mentions DeclaredQuery
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright 2018-2024 the original author or authors. | |||
* Copyright 2024 the original author or authors. |
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.
I don't think it does matter in any real sense of the word, but we should leave the copyright date as is.
* @author Jens Schauder | ||
* @author Diego Krupitza | ||
* @since 2.0.3 | ||
* @author Mark Paluch |
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.
We should leave the authors intact, even when the class changes completely.
/** | ||
* @author Mark Paluch | ||
*/ | ||
class DefaultDeclaredQuery implements DeclaredQuery { |
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.
Looks like a candidate for a record to me
* | ||
* @author Mark Paluch | ||
*/ | ||
public class JpaQueryConfiguration { |
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.
Could be a record
private @Nullable EntityManager entityManager; | ||
private EntityPathResolver entityPathResolver; | ||
private EscapeCharacter escapeCharacter = EscapeCharacter.DEFAULT; | ||
private JpaQueryMethodFactory queryMethodFactory; | ||
private @Nullable Function<BeanFactory, QueryEnhancerSelector> queryEnhancerSelector; |
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.
The name is misleading. queryEnhancerSelectorSource
of course we could go with queryEnhancerSelectorSelector
;-)
* | ||
* @param queryEnhancerSelector must not be {@literal null}. | ||
*/ | ||
public void setQueryEnhancerSelector(Class<? extends QueryEnhancerSelector> queryEnhancerSelector) { |
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.
queryEnhancerSelectorType
?
52ee55f
to
61e6d36
Compare
This is a draft for configuring a
QueryEnhancerSelector
that selects aQueryEnhancerFactory
based on aDeclaredQuery
:This change requires decoupling of
DeclaredQuery
(the declaration aspect) and the introspected part (IntrospectedQuery
) to avoid the logical cycle of introspecting a query uponDeclaredQuery
creation.Closes #3622