Avoid exception when using sqlDate#7565
Avoid exception when using sqlDate#7565gmethvin merged 2 commits intoplayframework:masterfrom marcospereira:issue-7522-forms-sqlFormat
Conversation
| @@ -225,15 +223,15 @@ object Formats { | |||
| */ | |||
| def sqlDateFormat(pattern: String, timeZone: TimeZone = TimeZone.getDefault): Formatter[java.sql.Date] = new Formatter[java.sql.Date] { | |||
There was a problem hiding this comment.
I see that you're ignoring the timeZone parameter now. I think that's correct behaviour, since the JDBC driver should be using the JRE's default timezone and we should just use the default too. But I just want to check that it's intentional to ignore this parameter.
There was a problem hiding this comment.
On second thoughts, we may want to allow the user to set this parameter if their JDBC driver has different behaviour and expects, e.g. a UTC timezone.
There was a problem hiding this comment.
It was intentional since we are converting to a LocalDate, which does not have a timezone. From the docs:
A date without a time-zone in the ISO-8601 calendar system, such as 2007-12-03.
And from java.sql.Date.valueOf:
The conversion creates a LocalDate that represents the same date value as this Date in local time zone.
I think the problem here is that we are missing a sqlTimestampFormat to support java.sql.Timestamp (which can be converted to/from LocalDateTime).
There was a problem hiding this comment.
If the timeZone parameter is not used it seems like it should be removed, deprecated or have a comment about it.
There was a problem hiding this comment.
Agree. Maybe the best option here is to have two methods instead of just one:
- One without the timezone
- Another with the timezone, but deprecated.
We won't have binary compatibility, but we can have code compatibility and then warn users about the deprecation.
WDTY?
There was a problem hiding this comment.
If we deprecate the one with the timezone doesn't that still give us binary compatibility?
It seems reasonable to deprecate if the code never actually used the timezone, since it's a confusing/useless API with an obvious replacement.
There was a problem hiding this comment.
Since this PR is closed I created an issue to track this: #7590
* Avoid exception when using sqlDate Fix #7522. * Add form support for java.sql.Timestamp
3 participants
Fixes
Fixes #7522.
Purpose
This is using the
LocalDateformat to bind/unbindjava.sql.Datesince the conversion between these two types is straightforward and supported.