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

Usage of LocalDateTime for commit timestamps #743

Closed
cberg-zalando opened this Issue Nov 20, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@cberg-zalando
Copy link

cberg-zalando commented Nov 20, 2018

We added JaVers to one of our projects for managing the history of our database entities. As history support was not a feature desired from the beginning, we used Spring to manage auditing information using the @CreatedDate and LastModifiedDate annotations on properties of type Instant. As database we are using PostgreSQL and using TIMESTAMPTZ as our field type.

We were wondering why there was a difference between these timestamps and the ones JaVers creates for each commit. Digging into the code, I see that you have a DateProvider interface which is explicitly using LocalDateTime. Was there a reason why this was explicitly limited to that type?

@bartoszwalacik

This comment has been minimized.

Copy link
Member

bartoszwalacik commented Nov 20, 2018

DateProvider was ctrated to mock clock in test. Javers is not ready for different timestamp types in commit.createDate.
So what java type do you like to have in createDate?

@cberg-zalando

This comment has been minimized.

Copy link

cberg-zalando commented Nov 21, 2018

We would prefer using Instant as we are already using it for all other timestamps in our application. I guess, it depends how configurable one wants to make DateProvider. You could change DateProvider::now() to return jave.time.temporal.Temporal interface and then allowing the library user to provide their own implementation and having DefaultDateProvider still using LocalDateTime.now() and keeping backwards compatibility.

Its not like you are otherwise doing anything with the LocalDateTime instance currently returned then assigning it to the CommitMetadata::comimitDate property.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

bartoszwalacik commented Nov 23, 2018

Sounds good, two things are important here.

  • commitDate is also used internally by javers to sort commits when CommitIdGenerator.RANDOM is used, so putting LocalDate here would be rather a bad idea.
  • persistence layer. What if someone change the type of commitDate during the application lifetime, then you have, for example old commits with ZonedDateTime and new commits with LocalDateTime.

Maybe commitDate should be left as is to be used internally by javers, and the new field can be added to CommitMetadata, let's say Temporal effectiveCommitDate? And this new field would be managed by users by supplying EffectiveCommitDateProvider.

Think about contributing

@cberg-zalando

This comment has been minimized.

Copy link

cberg-zalando commented Nov 28, 2018

If commits are stored in a database, this would mean that we would store the value of effectiveCommitDate in the commit_date column, I assume?!

Regarding your second point, I guess, having some kind of validation to see what Java date types are compatible with the currently chosen database column type might be tricky.

At least in our example, I used the output SQL in the application log and then manually changed the type of the commit_date column to be TIMESTAMPTZ as we are using Flyway for our database migrations.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

bartoszwalacik commented Nov 29, 2018

I'm not suggesting to change an existing field or db column, I suggest to add a new field and a new db column for it

public class CommitMetadata implements Serializable {
    private final String author;
    private final Map<String, String> properties;
    
    // used by JaVers to sort commits, you can't change how it's created
    private final LocalDateTime commitDate;

    // provided by users by registering an EeffectiveCommitDateProvider, could have arbitrary  precision or time zone 
    private final Temporal effectiveCommitDate;

    private final CommitId id;
@bartoszwalacik

This comment has been minimized.

Copy link
Member

bartoszwalacik commented Dec 27, 2018

After some research, I think, that adding another date (effectiveCommitDate) isn't a good idea.

Simply, we should change the type of CommitMetadata.commitDate from LocalDateTime to ZonedDateTime or Instant.

Btw, we can't just change the type of commit date, it would break compatibility. People have their snapshots persisted in databases with commit date as LocalDateTime.

bartoszwalacik added a commit that referenced this issue Dec 30, 2018

#743
sorting by COMMIT_DATE_INSTANT in MongoRepository
@bartoszwalacik

This comment has been minimized.

Copy link
Member

bartoszwalacik commented Dec 30, 2018

@cberg-zalando @NicoBondarenco PR is ready
#763
It will be released in 5.1. It should solve your issues

Now, DateProvider supplies ZonedDateTime which is saved in CommitMetadata as:

LocalDateTime commitDate - backward compatibility
Instant commitDateInstant - new field, provides reliable chronological ordering

bartoszwalacik added a commit that referenced this issue Dec 30, 2018

#743
javadoc
@bartoszwalacik

This comment has been minimized.

Copy link
Member

bartoszwalacik commented Dec 30, 2018

Released in 5.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment