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 support to pass-thru TemporalAccessor auditing values #2719

Closed
xenoterracide opened this issue Oct 17, 2022 · 7 comments
Closed

Add support to pass-thru TemporalAccessor auditing values #2719

xenoterracide opened this issue Oct 17, 2022 · 7 comments
Labels
type: enhancement A general enhancement

Comments

@xenoterracide
Copy link

This is incorrect, it would seem that java.time.Instant is also supported, sorry if this belongs in the commons repo. Note, I was trying to use OffsetDateTime for this.

businessDivisionId=a
createdOn=
id=0183ae78-9359-7422-a7b7-471f29af8980
lastModifiedOn=
version=0
]; Supported types are [org.joda.time.DateTime, org.joda.time.LocalDateTime, java.util.Date, java.lang.Long, long]

see this ticket, but especially this comment.

#880 (comment)

Most databases support "with timezone", javascript Date only seems to support offset, and so it is useful in cases to be able to communicate that and store it in the database.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 17, 2022
@schauder
Copy link
Contributor

schauder commented Nov 10, 2022

To clarify:

  • You prefer to use OffsetDateTime for auditing timestamps, which are currently not supported by Spring Data.
  • Instant does work although not official supported.

Can you please confirm this interpretation of the issue is correct?

Side note: the suppport for OffsetDateTime of some database JDBC drivers is quite limited in that they just drop the offset.

@schauder schauder added the status: waiting-for-feedback We need additional information before we can continue label Nov 10, 2022
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Nov 17, 2022
@xenoterracide
Copy link
Author

hmm... yes that is the correct interpretation. Interesting to learn that they drop, is that true if the underlying type is some kind of "timestamp with timezone"? Longterm the goal is both consistency, and dealing with silly people. In non auditables

json (Date only supports offset) -> server -> timestamp with offset stored

because for some reason someone wants localtime stored ,and we're also using the above

audit -> timestamp with offset stored (usually means a timestamp with timezone type)

another argument would be consistency, as doesn't hibernate just work with offsetdatetime? (sorry, it's now been a minute again)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Nov 17, 2022
@peterdieleman
Copy link

TIMESTAMP_WITH_TIMEZONE is an official column type defined by SQL standards, but support is indeed hit and miss even among popular and modern relational databases. PostgreSQL supports it out of the box and recommends its use, but other database systems don't (e.g. MariaDB).

@schauder May I ask what you mean by OffsetDateTime is not/sketchily supported by Spring Data? (I'm assuming you are referring to OffsetDateTime support in general, not just the auditing functionality). To what extent is this still true since version 2.2? I'm going by baeldung on jpa 2.2, but I think you might be referring to problems occurring 'under the hood'.

@schauder
Copy link
Contributor

I was referring to the JDBC drivers. I clarified my comment above. If I'm not mistaken some databases simply ignored the offset.

@xenoterracide
Copy link
Author

personally, sounds like a good note to put in docs, but a bug in those drivers? personal opinion

@mp911de mp911de changed the title support OffsetDatetime Add support to pass-thru TemporalAccessor auditing values Jul 12, 2023
@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jul 12, 2023
@mp911de
Copy link
Member

mp911de commented Jul 12, 2023

Generally speaking, our auditing infrastructure components could be more consistent. Error messages mention only a subset of supported temporal types, DefaultAuditableBeanWrapperFactory rejects temporal types, while MappingAuditableBeanWrapperFactory allows wrapper creation.

The use of TemporalAccessor values for which we do not provide converters already works (i.e. if a creation date is of ZonedDateTime or OffsetDateTime and the DateTimeProvider creates an instance of the declared temporal property).

I think that we should mention all supported temporal types for which we provide converters, we should refine our checks and have a set of tests to assert interoperability without providing additional converters as we cannot make assumptions over offsets or zone identifiers.

mp911de added a commit that referenced this issue Jul 12, 2023
We now allow passing-thru TemporalAccessor auditing values, bypassing conversion if the target value type matches the value provided from e.g. DateTimeProvider.

Refined the error messages and listing all commonly supported types for which we provide converters.

Closes #2719
schauder pushed a commit that referenced this issue Jul 13, 2023
Remove outdated Javadoc.

Original pull request #2874
See #2719
schauder added a commit that referenced this issue Jul 13, 2023
Minor formatting changes.

Original pull request #2874
See #2719
@schauder schauder added this to the 3.2 M1 (2023.1.0) milestone Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants