Skip to content
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

Address #1089 issue. Change ZonedDateTime conversion mechanism #1119

Closed
wants to merge 5 commits into from
Closed

Address #1089 issue. Change ZonedDateTime conversion mechanism #1119

wants to merge 5 commits into from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Jan 2, 2022

Addressing #1089 issue. Change ZonedDateTime mapping from String to ZonedDateTime, also make ZonedDateTime to be treated as Types.TIMESTAMP_WITH_TIMEZONE. Also supplied with H2 reading converter for ZonedDateTime. @schauder, can you please review the changes?

@mipo256 mipo256 changed the title change ZonedDateTime conversion mechanism Address #1089 issue. Change ZonedDateTime conversion mechanism Jan 2, 2022
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 4, 2022
@mipo256
Copy link
Contributor Author

mipo256 commented Jan 4, 2022

@schauder I will also add dialect-specific OffsetDateTime and ZonedDateTime conversions. The problem is in JdbcUtil class. If we dive deep into the actual class, StatementCreatorUtils, that will set the values into PreparedStatement - it sets the Types.TIMESTAMP_WITH_TIMEZONE by the means of

ps.setObject(paramIndex, inValue, sqlType);

Problem - couple of drivers (MySQL jdbc driver for example) actually does not support values to be set in the way above via sqlType Types.TIMESTAMP_WITH_TIMEZONE. Just to prove, take a look at mysql-connector - this is used in order to derive appropriate MySQLType in ClientPreparedStatement. So currently OffsetDateTime conversion in case of MySQL at least will fail.

Copy link
Contributor Author

@mipo256 mipo256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left a couple of explanational comments, just to make it a little bit more easy to understand the reason behind some changes.

@@ -240,21 +251,6 @@ public Object readValue(@Nullable Object value, TypeInformation<?> type) {
return super.readValue(value, type);
}

/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can even remove this API, because parent method also perform the same null check. Also if we assume that any external client also use this API, then they will not notice anything, because in this case they will just call parent writeValue directly.

@@ -545,13 +548,26 @@ private IdentifierProcessing getIdentifierProcessing() {
private void addConvertedPropertyValue(SqlIdentifierParameterSource parameterSource,
RelationalPersistentProperty property, @Nullable Object value, SqlIdentifier name) {

addConvertedValue(parameterSource, value, name, converter.getColumnType(property), converter.getSqlType(property));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JdbcConverter#getSqlType will invoke getColumnType internaly. This is inefficient, since we already called converter#getColumnType() for this RelationalPersistentProperty

@mipo256 mipo256 marked this pull request as draft January 6, 2022 15:44
@schauder
Copy link
Contributor

Thanks for putting so much effort into this.
Unfortunately I think it doesn't cut deep enough into what is wrong with the BasicJdbcConverter.
And we should fix that first. See #1136.
Really we need to break this change down into multiple tickets.

I therefore won't accept the PR. Sorry.

Apart from the main reason above I noticed a couple of things I'd like to point out, so they can be avoided in the future:

  • don't look up Dialects. The correct Dialect should get injected, not rediscovered over and over again.
  • Very basic classes like JdbcUtil or JdbcColumn shouldn't rely on Dialect instead when Dialect gets a new feature, everyone should use a method on Dialect which then might or might not access the original feature on a simpler class.
  • There shouldn't be the need to use @NotNull since it is applied by default on the package level. If it isn't the respective package file should be fixed/added.
  • Do not change formatting, especially not for code that didn't get touched otherwise.

@schauder schauder closed this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants