-
Notifications
You must be signed in to change notification settings - Fork 356
GH-2020 Added SqlTypeResolver abstraction #2024
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
Signed-off-by: mipo256 <[email protected]>
571fd96
to
1f2e694
Compare
...a-relational/src/main/java/org/springframework/data/relational/repository/query/SqlType.java
Outdated
Show resolved
Hide resolved
import org.springframework.data.relational.core.dialect.DefaultSqlTypeResolver; | ||
|
||
/** | ||
* Serves as a hint to the {@link DefaultSqlTypeResolver}, that signals the {@link java.sql.SQLType} to be used. |
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 the strong reference to SQLType
, we should move the entire annotation and infrastructure into the JDBC module.
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.
That makes sense, agree
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 problem here, I have explained it in a separate ticket: #2031
Please, take a look.
* {@link SQLType} of the given parameter | ||
*/ | ||
@Nullable | ||
SQLType resolveSqlType(RelationalParameter relationalParameter); |
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'm not sure we should return @Nullable
. If a type is defined, then it must be used.
Also, the parameter should be a value object consisting of @Nullable
name
and vendorTypeNumber
. RelationalParameter
originates from the repository.query
package and we don't want to introduce any cycles.
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'm not sure we should return @nullable. If a type is defined, then it must be used.
Okay... what is the expected behavior in this case? The DefaultSqlTypeResolver
may fallback to JdbcUtil.targetSqlTypeFor
resolution, as it happens now, but this interface may be implemented by the end users, and I'm not sure what SQLType
they need to return in case they do not know/care about the target SQLType
in this exact case.
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.
in case they do not know/care about the target SQLType
Then there's no sense in implementing the interface in the first place. When an annotated element is annotated with @SqlType
, then this interface must be used. Otherwise, the resolveSqlType
method would not be called.
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.
Then there's no sense in implementing the interface in the first place. When an annotated element is annotated with @SqlType, then this interface must be used. Otherwise, the resolveSqlType method would not be called.
I think this idea is a good one, but we can, possibly, do even better. In this case, the resolution logic is split up between the DefaultSqlTypeResolver
and "somewhere else" (the JdbcParameter
in our case with JdbcUtil
call), if I got you right.
What are your thoughts on the solution, where we would move the java.sql.SQLType
resolution logic in its entirety to the SqlTypeResolver
, so that the SqlTypeResolver
, as you've pointed out, never returns null
, but if the user would want to implement his own SqlTypeResolver
, then he/she could just decorate the DefaultSqlTypeResolver
and provide some additional logic on top of 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.
What are your thoughts on the solution, where we would move the
java.sql.SQLType
resolution logic in its entirety to theSqlTypeResolver
, so that theSqlTypeResolver
, as you've pointed out, never returnsnull
, but if the user would want to implement his ownSqlTypeResolver
, then he/she could just decorate theDefaultSqlTypeResolver
and provide some additional logic on top of it.
Where would resolution happen? Somewhere inside JdbcDialect
? JdbcUtil
is widely used in parts that aren't associated with a Dialect
. I like generally the idea to move type resolution closer to the Dialect
level. JdbcArrayColumns
already has some code for SQLType
resolution.
Following that line of thought, we could also add SQLType getSqlType(Class<?> componentType)
to JdbcDialect
while we keep the array part independently as array type resolution uses already code specific to array component types and array type codes.
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.
Where would resolution happen? Somewhere inside JdbcDialect?
Well, I do not know to be honest. Please, read below.
JdbcUtil is widely used in parts that aren't associated with a Dialect. I like generally the idea to move type resolution closer to the Dialect level. JdbcArrayColumns already has some code for SQLType resolution.
I know about it. Let me go thought it step by step, I'm not as smart as you @mp911de 😄
The fact is that the JdbcUtil.targetSqlTypeFor(Class<?> type)
accepts a Class
. And, in general, it just takes advantage of a static mapping between Class
--> SQLType
. What we have in our case is an annotation @SqlType
that is placed on a org.springframework.core.MethodParameter
and that overrides that static mapping in very particular cases.
So, SqlTypeResolver
cannot just accept Class
. It has to accept something around MethodParameter
, or JdbcParamter
, because just Class
does not carry any annotation info itself, at least for now.
Moving on, @SqlType
can only be placed on method parameters, and not POJO fields. Therefore, sometimes, when we have JdbcDggregateTemplte.find()
or insert()
the resolution of java.sql.SQLType
would still happen based on Class
only.
So, the reason I'm talking all that through is to explain to myself and outline it here for others that SqlTypeResolver
can only be used in JdbcParameters
as a SQLType
resolution for them and them only, at least it seems. The MappingJdbcConverter
is used when we read and write our POJOs, but again, this is not our case.
So, to conclude, I think we can encapsulate all the logic for SQLType
resolution of the method parameters in the SqlTypeResolver
, but for now that is it.
Following that line of thought, we could also add
SQLType getSqlType(Class<?> componentType)
to JdbcDialect while we keep the array part independently as array type resolution uses already code specific to array component types and array type codes.
Yes, that probably makes sense. I need to play around with it. It is not yet 100% clear to me that moving getSqlType
from JdbcArrayColumns
into JdbcDialect
is going to be the right move... Maybe we can consider this in a separate PR? I can play around with it, and we will see what we have here.
...lational/src/main/java/org/springframework/data/relational/core/dialect/SqlTypeResolver.java
Outdated
Show resolved
Hide resolved
...-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/StringBasedJdbcQuery.java
Outdated
Show resolved
Hide resolved
Everything is fixed, except for the remaining two conversation threads @mp911de |
Fixes #2020
I've introduced the
SqlTypeResolver
interface, and a default implementation of it -DefaultSqlTypeResolver
.Backward compatibility: I've removed the
@Deprecated
constructors. Apart from that, it seems that we're backward compatible, if I have not overlooked anything.Note: I had to wrap the
SqlTypeResolver
withLazy
here. The reason is described in the ad-hoc comment and in this issue.CC: @mp911de @schauder