-
Notifications
You must be signed in to change notification settings - Fork 356
Introduced new JDBC dialect counterparts #2036
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
@@ -109,23 +111,23 @@ public Optional<Dialect> getDialect(JdbcOperations operations) { | |||
} | |||
|
|||
@Nullable | |||
private static Dialect getDialect(Connection connection) throws SQLException { | |||
private static JdbcDialect getDialect(Connection connection) 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.
So, here I am not sure what is the best solution. Let me clarify. The JdbcDialectProvider
interface is defined like this:
public interface JdbcDialectProvider {
Optional<Dialect> getDialect(JdbcOperations operations);
}
The common sense dictates, that since it is a DialectProvider
for JDBC, then the returned dialect is supposed to be an instance of JdbcDialect
. However, such type migration change would not be backward compatible.
The solution I can think of is to deprecate this method in the interface, and maybe even make JdbcDialectProvider
a top-level interface, instead of being an inner one. And in this case, we can direct users to a new interface to be used, where getDialect
method returns the Optional<JdbcDialect>
If we just deprecate the JdbcDialectProvider#getDialect
method, we will not be able to say at the moment what is the replacement.
Maybe I'm just overcomplicating things... I need your option, Mark @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.
We would split the change into two parts. Updating JdbcDialectProvider
to return JdbcDialect
in the signature would be a change shipped with the next major release.
For the 3.5 release, it should be fine to have an adapter that translates a generic Dialect
into JdbcDialect
and from DialectResolver.getDialect(…)
we can directly return JdbcDialect
.
Signed-off-by: mipo256 <[email protected]>
Deprecate original DialectResolver and JdbcArrayColumns as they've been in the wrong package and introduce replacements in the dialect package. Let deprecated types extend from their replacements to retain compatibility. Make instance holders final, fix Javadoc typos, update reference docs.
@schauder can you have a look after a round of polishing? Going forward, we would drop the deprecated elements in 4.0 and update the configuration so that we exclusively require |
Sure, I would take a look into your changes @mp911de, I'll reply shortly |
} | ||
|
||
/** | ||
* Exception thrown when {@link DialectResolver} cannot resolve a {@link Dialect}. | ||
*/ | ||
public static class NoDialectException extends NonTransientDataAccessException { | ||
@Deprecated(since = "3.5", forRemoval = true) |
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 add a javadoc @deprecated
tag here? Just in case to specify the replacement like in other parts of the old DialectResolver
...
@mp911de I think it is great overall. I only have the minor comment above. As a side note, I can provide a PR for 4.x milestone to got rid of those classes once this is merged. |
Closes #2031
The idea is to introduce for each relational
Dialect
its jdbc counterpart. All the Jdbc dialects that were already in place (there were a couple of those) and added ones now extend theJdbcDialect
and their corresponding relational counterpart.CC: @mp911de @schauder