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

add setDateTime parameter for DriverJdbcType[DateTime] #77

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

xuwei-k
Copy link
Contributor

@xuwei-k xuwei-k commented Dec 4, 2020

I think #30 is NOT right way in most cases.
But I have not change the default behavior directly for keep compatibility.

I think tototoshi#30 is **NOT** right way in most cases.
But I have not change the default behavior directly for keep compatibility.
@tototoshi
Copy link
Owner

I still don't understand why the current implementation is not correct.

Can you please explain the correct behavior you expect? Or I would like some test code. Also, can you tell me which database you are using?

@y-yu
Copy link
Contributor

y-yu commented Sep 13, 2022

I agree with @xuwei-k, there is something wrong in the current implementation. I wrote this test to show up: 👀

[info] MySQLJodaSupportSpec:
[info] JodaSupport
[info] - should enable us to use joda-time with slick
[info] - should enable us to use joda-time with string interpolation API
[info] - can be used with comparative operators
[info] - should be able to filter with the specified date
[info] - should be the same in/out joda-time zoned Asia/Tokyo
[info] - should be the same in/out joda-time zoned UTC *** FAILED ***
[info]   1354806000000 was not equal to 1354838400000 (JodaSupportSpec.scala:252)
[info] - should be the same in/out joda-time zoned Europe/London *** FAILED ***
[info]   1354806000000 was not equal to 1354838400000 (JodaSupportSpec.scala:252)
[info] - should be the same in/out joda-time zoned America/New_York *** FAILED ***
[info]   1354806000000 was not equal to 1354856400000 (JodaSupportSpec.scala:252)

This test is comparison of UTC zoned joda DateTime value then save into the table, and that loaded from DB. In SQL TIMESTAMP doesn't hold timezone but unix epoch times of those must be the same. So I think the current way is not correct. So I think it would be better to revert #30 or merge this PR. @xuwei-k san's fix version pass the test.

[info] MySQLJodaSupportWithoutCalenderSpec:
[info] JodaSupport
[info] - should enable us to use joda-time with slick
[info] - should enable us to use joda-time with string interpolation API
[info] - can be used with comparative operators
[info] - should be able to filter with the specified date
[info] - should be the same in/out joda-time zoned Asia/Tokyo
[info] - should be the same in/out joda-time zoned UTC
[info] - should be the same in/out joda-time zoned Europe/London
[info] - should be the same in/out joda-time zoned America/New_York

getValue also should be fixed in addition to @xuwei-k san's fix.
xuwei-k/slick-joda-mapper@setDateTime...y-yu:slick-joda-mapper:setDateTime-and-getDateTime

I don't understand why ResultSet#getTimestamp takes Calendar but it works.

@y-yu
Copy link
Contributor

y-yu commented Oct 17, 2022

Are there any updates? 👀

@y-yu
Copy link
Contributor

y-yu commented Jun 23, 2023

@tototoshi 👀 🙏

@tototoshi tototoshi merged commit 9e36d11 into tototoshi:master Jun 23, 2023
@tototoshi
Copy link
Owner

I had the opportunity to speak with @y-yu directly about this issue some time ago. I really want to solve this problem fundamentally. But if it causes incompatible changes, users may have a new accident.

This library is already not actively developed. And there will be no need for it. A major upgrade to fundamentally solve the problem is overkill. This PR is a workaround, but at least it is safe for current users.

@y-yu
Copy link
Contributor

y-yu commented Jun 23, 2023

Thank you!

@xuwei-k xuwei-k deleted the setDateTime branch June 30, 2023 07:17
@y-yu y-yu mentioned this pull request Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants