-
Couldn't load subscription status.
- Fork 377
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
Conversation
d1b8b08 to
8cc09d2
Compare
8cc09d2 to
d92a729
Compare
571fd96 to
1f2e694
Compare
...a-relational/src/main/java/org/springframework/data/relational/repository/query/SqlType.java
Outdated
Show resolved
Hide resolved
...a-relational/src/main/java/org/springframework/data/relational/repository/query/SqlType.java
Show resolved
Hide resolved
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.SQLTyperesolution 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 theDefaultSqlTypeResolverand 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
be52a4b to
329e256
Compare
Everything is fixed, except for the remaining two conversation threads @mp911de
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.
This PR has failing DependencyTests.
Could you please fix those.
@schauder sure, let me check
329e256 to
9bc242d
Compare
So, the dependency test is solved, @schauder. We only have one discussion thread left.
Can I kindly ask for the re-review @mp911de, @schauder? I have to make some Dialect --> JdbcDialect migrations as well in the PR since we now have the JdbcDialect counterparts in the main tree, so I, as agreed with Mark here, moved the SqlTypeResolver and the entire infrastructure into the jdbc module.
6a62dd2 to
e3a70e7
Compare
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.
Maybe we can move this in the separate commit? We have a chance to remove it in spring-data-jdbc 4.0 major release
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 still get DependencyTest failures:
DependencyTests.acrossModules:72 Architecture Violation [Priority: MEDIUM] - Rule 'slices assigned from Submodule should be free of cycles' was violated (1 times):
Cycle detected: Slice core ->
Slice repository ->
Slice core
Note that this is in R2DBC.
core should not depend on repository
I'll take a look at it shortly and also rebase thsi branch onto main @schauder, thanks
e3a70e7 to
ecc3a43
Compare
Yep, that was my mistake, sorry. I fixed the package now, thank you, @schauder
P.S: By the way, maybe we can make running this tests as a part of the PR build process? So that we can make sure that all tests are fine
They do run on a simple mvn clean verify. Anyway, thanks for fixing the tests.
ecc3a43 to
c97664d
Compare
Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>
c97664d to
28d78c4
Compare
Rebased onto the main branch
70f28df to
a28a2cd
Compare
Fixes #2020
I've introduced the
SqlTypeResolverinterface, and a default implementation of it -DefaultSqlTypeResolver.Backward compatibility: I've removed the
@Deprecatedconstructors. Apart from that, it seems that we're backward compatible, if I have not overlooked anything.Note: I had to wrap the
SqlTypeResolverwithLazyhere. The reason is described in the ad-hoc comment and in this issue.CC: @mp911de @schauder