-
Notifications
You must be signed in to change notification settings - Fork 367
Add Ahead of Time Repository support #2124
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
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.
I left some minor complaints.
In general this looks good to me.
This optimization moves query method processing from runtime to build-time, which can lead to a significant performance improvement as query methods do not need to be analyzed reflectively upon each application start. | ||
|
||
The resulting AOT repository fragment follows the naming scheme of `<Repository FQCN>Impl__Aot` and is placed in the same package as the repository interface. | ||
You can find all queries in their String form for generated repository query methods. |
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 guess there is a "in the implementation" missing or something to that effect.
...ava/org/springframework/data/relational/repository/query/SimpleRelationalEntityMetadata.java
Show resolved
Hide resolved
|
||
Assert.notNull(tableEntity, "Table entity must not be null"); | ||
|
||
this.type = (Class) tableEntity.getType(); |
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.
Shouldn't we call the other constructor, so we have a single place where initialization takes place?
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.
Calling this(…)
potentially incurs a NullPointerException
if tableEntity
is null
.
* @author Mark Paluch | ||
* @since 4.0 | ||
*/ | ||
public class ParameterBinding { |
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 would expect a ParameterBinding
to be the binding of a value to a parameter. But this seems to be just a BindParameter
: a parameter to which a value can be bound.
Correct?
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.
Actually, the ParameterBinding
is intended as command to bind the parameter and not do this optionally. We have a similar naming setup in other modules. We could dive deeper into naming nuances if we wanted to. Feel free to create a ticket at the commons level.
...data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/StatementFactory.java
Outdated
Show resolved
Hide resolved
...-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/StringValueUtil.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public T mapRow(ResultSet resultSet, int rowNumber) throws SQLException { |
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.
are we missing a @Nullable
annotation here, or is the object != null
check superfluous?
String parameterNames = StringUtils.collectionToDelimitedString(context.getAllParameterNames(), ", "); | ||
|
||
if (StringUtils.hasText(parameterNames)) { | ||
this.parameterNames = ", " + parameterNames; | ||
} else { | ||
this.parameterNames = ""; | ||
} |
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.
IMO it is confusing to have something called parameterNames
that contains a starting colon which influences the method called when being used in the CodeBlock
via $L
later on.
expressionString = "#{" + expressionString + "}"; | ||
} | ||
|
||
builder.addStatement("evaluate($L, $S$L).bind($S, $L)", context.getExpressionMarker().enclosingMethod(), |
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.
$S$L
magic method call switch via parameter names mentioned already above.
builder.addStatement("return ($T) convertMany($L, %s)".formatted(dynamicProjection ? "$L" : "$T.class"), | ||
context.getReturnTypeName(), result, queryResultTypeRef); |
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.
should we use string formatting to bypass more expressive use of the builder API? Puts additional cognitive load on the reader having to deal with 2 placeholder patterns and a dynamic result type ref.
|
||
if (Optional.class.isAssignableFrom(context.getReturnType().toClass())) { | ||
builder.addStatement( | ||
"return ($1T) $1T.ofNullable(convertOne($2L, %s))".formatted(dynamicProjection ? "$3L" : "$3T.class"), |
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.
do we need that ($1T)
cast to do satisfy the compiler? (Optional) Optional.ofNullable(...)
looks strange though.
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 have a bit of general dynamic in row mappers as we can have user-specified row mappers that do not necessarily comply with the RowMapper<ReturnType>
declaration and we can end up with slightly different typing. Therefore, we broadly cast types to make it work now and refine typing later on, especially once we have a fluent API that would remove the need for broad casting.
builder.addStatement("$T $L = getRowMapperFactory().create($T.class)", RowMapper.class, rowMapper, | ||
context.getRepositoryInformation().getDomainType()); | ||
|
||
builder.addStatement("$T $L = ($T) getJdbcOperations().query($L, $L, new $T<>($L))", List.class, result, |
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.
indexed placeholders maybe?
We now provide AOT support to generate repository implementations during build-time for JDBC repository query methods. Supported Features Derived query methods, @query and named query methods * @Modifying methods returning void, int, and long * Pagination, Slice, Stream, and Optional return types * DTO and Interface Projections * Value Expressions Excluded methods * CrudRepository, Querydsl, Query by Example, and other base interface methods as their implementation is provided by the base class respective fragments * Methods whose implementation would be overly complex * Methods accepting ScrollPosition (e.g. Keyset pagination)
41d11de
to
8a0447a
Compare
We now provide AOT support to generate repository implementations during build-time for JDBC repository query methods.
Supported Features
@Query
and named query methods@Modifying
methods returningvoid
,int
, andlong
Slice
,Stream
, andOptional
return typesLimitations
ScrollPosition
(e.g.Keyset
pagination) are not yet supportedExcluded methods
CrudRepository
, Querydsl, Query by Example, and other base interface methods as their implementation is provided by the base class respective fragments** Methods accepting
ScrollPosition
(e.g.Keyset
pagination)Example repository:
Generated fragment:
Metadata (truncated):