-
Notifications
You must be signed in to change notification settings - Fork 683
Auditing should support Java 8 Date & Time types [DATACMNS-411] #880
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
Comments
Nick Williams commented It's been almost five months since I recorded this issue, and I haven't even received a comment about it. I'd like to get some clarification on:
|
Oliver Drotbohm commented The currently supported types can be found in
The third point is the most blocking reason right now. I've briefly thought about making the I've filed SPR-11259 as Spring Framework already builds against JDK 8 (using Gradle) and if it added the necessary converters there's only little changes we needed to apply in Spring Data Commons. Plus, we generally support Java 8 on Spring 4 anyway, so it'd be a nice match and we didn't have to rule out incompatible scenarios (e.g. Spring 3.2.x on Java 8) ourselves |
Oliver Drotbohm commented The tip regarding compilation against JDK 8 types on Twitter was really helpful. Thanks a lot for that. However our tests do not run on Java 8, mostly because of the ASM version inlined in Spring 3.2.x is not capable of handling Java 8 classes and thus a lot of container internals start to fail. The workaround would be to run our tests against Spring 4 which effectively won't work without a hard dependency upgrade to Spring 4 (again, due to Maven restrictions). I'll see Juergen Hoeller early January so we might be able to roll the fix for the Spring Framework ticket into Spring 4.0.1, so that we can transparently benefit from using Spring Data alongside Spring 4. If that happens, we even include the necessary changes from our side into a bugfix release of Spring Data Commons |
Nick Williams commented
<project>
|
Oliver Drotbohm commented My general sentiment is that I don't want to start to spread "if Java 8 do that, else do that" code inside our codebase. Spring Framework already has this kind of stuff in it, so if it can take over the burden, even better. Especially as we support Java 8 on Spring 4 only. So we'd derail from this path by adding Java 8 specific code to a Spring 3.2.x codebase and then guarding it with "if (on Java 8 && on Spring 4) { …". Regarding 3. To be honest: types like Regarding 4. I wasn't arguing from a stability perspective of Java 8. I was arguing from a distribution perspective amongst Java developers and even more important: real-world Java projects. We've taken care of Spring Data working without issues on Java 8 in the latest release and will continue to improve the support going forward. I just don't want to rush a half-baked solution into the library quickly, if there's no urgent need for it, especially if the alternatives are clearly obvious and just need a bit of time to be discussed. Going forward, there's definitely an option to arrange suiting release dates alongside external deadlines. This just not going to work 3 weeks in advance. Regarding 5. I think it probably would. But I don't think the gained performance benefit is worth the complexity added to the codebase. The different supported types are effectively different representations of the same thing, exactly the kind of thing the |
Nick Williams commented You misunderstand. I'm not suggesting we have even one public interface AuditingTimestampGenerator<T> {
T generateTimestamp();
}
public class DateAuditingTimestampGenerator implements AuditingTimestampGenerator<Date> {
Date generateTimestamp() {
return new Date();
}
}
public class CalendarAuditingTimestampGenerator implements AuditingTimestampGenerator<Calendar> {
Calendar generateTimestamp() {
return Calendar.getInstance();
}
}
public class TimestampAuditingTimestampGenerator implements AuditingTimestampGenerator<Timestamp> {
Timestamp generateTimestamp() {
return new Timestame(new Date().getTime());
}
}
public class DateTimeAuditingTimestampGenerator implements AuditingTimestampGenerator<DateTime> {
DateTime generateTimestamp() {
return new DateTime();
}
}
public class InstantAuditingTimestampGenerator implements AuditingTimestampGenerator<Instant> {
Instant generateTimestamp() {
return Instant.now();
}
}
public class LocalDateTimeAuditingTimestampGenerator implements AuditingTimestampGenerator<LocalDateTime> {
LocalDateTime generateTimestamp() {
return LocalDateTime.now();
}
}
public class OffsetDateTimeAuditingTimestampGenerator implements AuditingTimestampGenerator<OffsetDateTime> {
OffsetDateTime generateTimestamp() {
return OffsetDateTime.now();
}
}
public class ZonedDateTimeAuditingTimestampGenerator implements AuditingTimestampGenerator<ZonedDateTime> {
ZonedDateTime generateTimestamp() {
return ZonedDateTime.now();
}
} Then, ...
if (Date.class == returnType) {
return new DateAuditingTimestampGenerator();
}
else if (Calendar.class == returnType) {
return new CalendarAuditingTimestampGenerator();
}
else if (Timestamp.class == returnType) {
return new TimestampAuditingTimestampGenerator();
}
else if ("org.joda.time.DateTime".equals(returnType.getName())) {
return new DateTimeAuditingTimestampGenerator();
}
else if ("java.time.Instant".equals(returnType.getName())) {
return new InstantAuditingTimestampGenerator();
}
else if ("java.time.LocalDateTime".equals(returnType.getName())) {
return new LocalDateTimeAuditingTimestampGenerator();
}
else if ("java.time.OffsetDateTime".equals(returnType.getName())) {
return new OffsetDateTimeAuditingTimestampGenerator();
}
else if ("java.time.ZonedDateTime".equals(returnType.getName())) {
return new ZonedDateTimeAuditingTimestampGenerator();
}
... This is nearly identical to how Spring's ...
else if (Locale.class.equals(paramType)) {
return RequestContextUtils.getLocale(request);
}
else if (TimeZone.class.equals(paramType)) {
TimeZone timeZone = RequestContextUtils.getTimeZone(request);
return (timeZone != null ? timeZone : TimeZone.getDefault());
}
else if ("java.time.ZoneId".equals(paramType.getName())) {
return ZoneIdResolver.resolveZoneId(request);
}
... My primary concern now is the hard-coded dependency on Joda Time, even if the entity is using |
Oliver Drotbohm commented Feels like we've got different understanding of clean code. A huge if-cascade clearly is not what I was imagining. We can avoid all this stuff and simply add a single if (conversionService.canConvert(Date.class, targetType) {
conversionService.convert(value.toDate(), targetType);
} and be done with it. No need for a plethora of one-method-of-one-line types per supported type, let alone the additional SPI interface etc. I don't quite get the hassle with JodaTime. It just works, and makes things easier. Yes we could make its usage optional and only require it if someone actually uses |
Nick Williams commented Well that's a fair point (the ...
// in AnnotationAuditingMetadata
generators.add(Long.class.getName(), new LongAuditingTimestampGenerator());
generators.add(Date.class.getName(), new DateAuditingTimestampGenerator());
generators.add(Calendar.class.getName(), new CalendarAuditingTimestampGenerator());
generators.add(Timestamp.class.getName(), new TimestampAuditingTimestampGenerator());
if(ReflectionUtils.classExists("org.joda.time.DateTime")) {
generators.add("org.joda.time.DateTime", new DateTimeAuditingTimestampGenerator());
}
if(ReflectionUtils.classExists("java.time.Instant")) {
generators.add("java.time.Instant", new InstantAuditingTimestampGenerator());
generators.add("java.time.LocalDateTime", new LocalDateTimeAuditingTimestampGenerator());
generators.add("java.time.OffsetDateTime", new OffsetDateTimeAuditingTimestampGenerator());
generators.add("java.time.ZonedDateTime", new ZonedDateTimeAuditingTimestampGenerator());
}
...
AuditingTimestampGenerator<?> generator = generators.get(targetType.getName());
...
// in wherever AnnotationAuditingMetadata is used
setDateCreatedMethod.invoke(entity, setDateCreatedGenerator.generateTimestamp());
... That's really clean. The "hassle" is a lot of developers don't want classes on their classpath that they aren't using. Every additional library in a Java EE web application increases the startup time because that's another library whose classes must be scanned. Prior to Java 8, I would've been using Joda Time, because like you said, it's easier and it "just works." But now that Joda Time has been ported to the Java SE platform as |
Oliver Drotbohm commented Just wanted to give you some feedback as we made reasonable progress, which I'll polish up in the upcoming days but haven't finished yet. We'll be able to support JDK 8 types on Spring 4 transparently with the changes introduced in SPR-11259. We'll move to a The problematic part of the support is now really moved to the underlying store as currently there's no JPA persistence provider supporting these types natively. The Jadira Usertypes project for Hibernate also only works for the backport. I haven't tested the MongoDB driver yet, but we could potentially register appropriate converters ourselves. For the JPA module we might even be able to ship some JPA 2.1 converter but the devil is in the details here again |
Nick Williams commented I wouldn't worry about persisting the types to the underlying datastore. IF the user choses to create an entity whose auditing dates are the Java 8 Date & Time types, then they already have a plan for how to convert them. If they didn't have a plan, they'd choose some other auditing date type. Plus, JPA 2.2 will have native support for the Java 8 Date & Time types. I just don't think we have to put any effort into providing converters or other capabilities for converting these types to their persistence store. Now, with all that said, I'm very glad to hear this will make its way into Commons 1.7. I see RC1 was released a few days ago. Hurray! I took a look at the code and I have a couple of questions/comments:
Thanks again! |
Oliver Drotbohm commented @ Java 8 Date / Time types and JPA - Well, there's other aspects to add here that might not be so obvious from a very "my special way of using it" perspective. Some of these aspects were reasons I wasn't to eager to get the support in so quickly but let me outline this: Us providing the support for Java 8 Date / Time types will make people assume they just work as other date / time types, which is what the Java 8 ones clearly do not for now. It might not be a surprise to you, but it will be to others. Not sure, where you found the information about the JPA 2.2 feature set (I am on the expert group and don't have any so far) but even if we assume that JPA 2.2 will require persistence providers to support these types, when exactly will be actually seeing implementations for that? By 2016? Seems like we're at least going to see support for Hibernate in that regard pretty soon. @ @ |
Nick Williams commented Oliver Drotbohm, thanks for clearing this up for me. Makes a lot of sense. I didn't "find" information about the JPA 2.2 feature set. Java EE 8 will naturally require Java SE 8, which requires support for the java.time types in JDBC, so naturally I imagine JPA will require support for the java.time types. Also (some time ago) I created JPA_SPEC-63 to address this requirement. I did not realize you were on the expert group—that's good to know. I'm in the process of joining the JCP and am interested in joining the JPA expert group. I understand the motivation for including converters in Spring Data and/or Spring Framework. However, if you do, please be sure that |
Holger Stolzenberg commented Hey guys, I am currently setting up a new project with
I have defined my database columns for
Having I also tried following example project, but the problem occurs there two. Am I missing something or is this the desired behaviour? I ended up using Jadira, but this makes explicit I am not sure if this is an In the end, is it possible to map |
Nick Williams commented If you're using Hibernate directly, you need to create a custom If you're using JPA, you must use the latest version (JPA 2.1, part of Java EE 7), create a custom |
Holger Stolzenberg commented Thanks for the quick reply. So in the end I will stick with Jadira for now, as I see no advantage in writing my own |
I think this should be re-opened.
|
@adnklauser – Thanks for chiming in. Can you clarify what you like to see changed or added? Spring Data's auditing has supported Java 8 times for ages now. Why should the ticket be re-opened? What exactly is missing? |
Hey @odrotbohm, It might actually be just a case of a misleading error message 😅. I am using the Azure cosmos DB spring data integration and I want to have java.time Upon closer examination of how Creating a custom I think we can leave this issue closed, but it might be worth improving the "rejectUnsupportedType" error message. |
You'r observation is correct. We strongly believe in time zones being a presentation layer problem and thus currently don't support any kinds of zoned date time types. Primarily because persisting them directly makes the values incomparable unless someone downstream unifies them to a reference time zone and persisting the zone separately. AFAIK, only Oracle currently supports persisting zoned times this way. That's why try to nudge folks into using Does that make sense? |
Yes, fully agree. UTC And indeed, using In any case, my problem is solved (I have switched to tl;dr for every one else stumbling across this ticket: use |
I can't believe that this is still not working in 2021.... For everyone tired of all this, just create this configuration class: (Kotlin) @Configuration
@EnableJpaAuditing(dateTimeProviderRef = "dateTimeProvider")
class PersistenceConfig {
@Bean
fun dateTimeProvider() = DateTimeProvider { Optional.of(OffsetDateTime.now()) }
} |
Me too. @odrotbohm argument:
is very questionable if generalized (but the author pointed to It was true that many DB were unable to store TZ with timestamp in the past. As I understand SQL 2003 defines JDBC 4.2 spec provides official mapping for I assume Spring Data Auditing is just broken for the present.
Prior discussions:
I'll try to open a proper report justifying why |
@spyro2000 And let me add, me too. Thanks for the snippet. @gavenkoa Thanks, 100% your quote in bold part points out exactly why this is unacceptable for an auditing framework. This is as far as my internet queries got me. Did you try and open a report since? |
Using That said, discussing this on an issue that was closed 8 years ago isn't helpful either. For the record: The issue was closed because we do support Java 8 In order to get this ticket here it's well earned rest:
|
@peterdieleman I forgot about this issue due to shift of priorities. Feel free to open a new one. Key points:
|
@schauder I want to use |
currently I'm questioning the usability of this feature for this, better to write my own listener and only use it for user recording. Joda and Date/Calendar shouldn't be used anymore in my opinion |
(sorry can't edit comments) this issue should be reopened instead of a new one, IMO. As it hasn't actually been resolved and google will lead here. |
@xenoterracide please open a new issue non the less. We use these issues to track changes and link them to releases. So reopening an issue that has released changes attached to it will break stuff. If you reference this issue here there will be links between the issues, so people will be able to find any related issue. |
done |
- ZonedDateTime으로 audit할 수 있도록 수정 - 참고링크 - spring-projects/spring-data-commons#880
- ZonedDateTime으로 audit할 수 있도록 수정 - 참고링크 - spring-projects/spring-data-commons#880
Nick Williams opened DATACMNS-411 and commented
This may or may not be in the right project. I think it is, but it may need to be moved to Spring Data Commons.
Currently, fields annotated
@CreatedDate
and@LastModifiedDate
can be Joda Time'sDateTime
or (I think)java.util.Date
,java.util.Calendar
, orjava.sql.Timestamp
. If it doesn't support these last three types, it should.Auditing should also support the Java 8 Date & Time API. Fields modified with one of these two annotations should be able to be
java.time.Instant
,java.time.LocalDateTime
,java.time.OffsetDateTime
, andjava.time.ZonedDateTime
Affects: 1.7 M1 (Codd), 1.6.3 (Babbage SR2)
Issue Links:
SPR-11259 Add Converter implementations that convert legacy Date instances into JDK 8 date/time types
("depends on")
DATAJPA-592 Auditing annotations not working when applied to methods
DATACMNS-307 Remove JodaTime dependency
Referenced from: commits 51b128f
The text was updated successfully, but these errors were encountered: