GH-2020 Added SqlTypeResolver abstraction

[mipo256]

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 with Lazy here. The reason is described in the ad-hoc comment and in this issue.

CC: @mp911de @schauder

[mp911de]

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.

[mipo256]

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.

[mp911de]

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.

[mipo256]

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.

[mp911de]

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.

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.

[mipo256]

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.

[mipo256]

Everything is fixed, except for the remaining two conversation threads @mp911de

[schauder]

This PR has failing DependencyTests.

Could you please fix those.

[mipo256]

@schauder sure, let me check

[mipo256]

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.

[mipo256]

Maybe we can move this in the separate commit? We have a chance to remove it in spring-data-jdbc 4.0 major release

[schauder]

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

[mipo256]

I'll take a look at it shortly and also rebase thsi branch onto main @schauder, thanks

[mipo256]

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

[schauder]

They do run on a simple mvn clean verify. Anyway, thanks for fixing the tests.

[mipo256]

Rebased onto the main branch