Fix inconsistent timestamp values #1784
Merged
Conversation
- This removes the code duplication in Model and ModelBase objects.
- This coerces a model's timestamps to a consistent Date format for all fetch operations across all database implementations. Previously dates would be returned as Unix timestamps after a fetch on SQLite, but they would be a regular Date object right after model creation. - There's currently a bug in knex that prevents this from working correctly for MySQL, since timestamp columns will discard milisecond information and round to the nearest second.
This was referenced Mar 21, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Introduction
Ensure date values of automatic timestamps are always returned in a consistent format.
Motivation
When using SQLite dates are returned as Unix timestamps when fetching data from the database, but when a model is created and saved they are stored as Date objects on the model. This creates inconsistency when dealing with dates between saving and fetching, which leads to users having to deal with this inconsistency themselves. However it should be the ORM's job to deal with these issues and provide a consistent interface for users. See the above related issue for more info.
Fixes #1068.
Proposed solution
This adds a new method to format the dates whenever a model has the
hasTimestampsattribute set totrueor an array of key names. Unlike the previous PR this doesn't wrap existing methods (parse) and just calls the new method when needed. The advantage of this approach is that theparsemethod is kept untouched which is a positive thing since it's already complicated and tangled as it is.Current PR Issues
Doesn't format other date columns the user might have set on the database, but since Bookshelf doesn't know about the database structure it wouldn't be feasible anyway.
There's also a problem in the way that MySQL handles dates that causes millisecond information to be discarded when saving to the database. This can be fixed but requires a change in Knex and there's nothing we can do on our side except for recommending that users use a custom date format in migrations when dealing with MySQL.