Clean up automatic timestamps feature #1798

ricardograca · 2018-03-25T15:37:16Z

Previous PRs: Fix timestamps after messy 1607 PR #1710, New implementation for the setting of timestamps columns values #1607, Allow passing of time via options #1592, Use created_at from query if passed #1583

Introduction

This cleans up the previous implementations of the automatic timestamps logic.

It also changes the existing functionality so that saving a model that hasn't changed will not update its updated_at attribute as discussed in #1710.

Passing date in the options to save() is also deprecated.

All the documentation regarding this feature is updated and new examples are added to further explain it.

Closes #1710.

Motivation

The previous PRs introduced valuable functionality but were overly complicated in their implementations or introduced bugs in the way this feature was expected to work. The current solution behaves like the Rails ActiveRecord's counterpart.

In #1710 the general idea was good, but it introduced a bug that would lead to created_at always getting updated with a new value whenever a model was updated, and the bugs around editCreatedAt that were mentioned there are not valid anymore since those options are removed.

With PR #1607 it was possible to prevent a model's timestamp attributes from being updated by user values, but this can easily be achieved using other methods, like unsetting the attributes before saving. It also introduced (along with #1583) the ability to update the timestamps with user supplied values, which was good, but the implementation was a bit spread out throughout the code base and too complicated, and in the case of #1583 it introduced a bug that prevented the updated_at attribute from being automatically updated in some cases.

The ability to pass a date in the options to save() that was introduced in #1592 is redundant since the same thing can easily be achieved with existing methods, so it was deprecated.

Proposed solution

This will prioritize user defined values for the created_at and updated_at timestamps over any other considerations, so it's always possible to update these values directly.

If no user defined values are provided for these attributes they will be automatically set. If the model is new both of them will be set to the current date. This also works if the model being saved is new but already includes the id attribute, because in this case the {method: 'insert'} option is passed to the save() call.

If the model is not new only the updated_at attribute will be automatically updated.

The options.date functionality of .save() to set both timestamp attributes with the same value is deprecated and will be removed in the next version. It's already possible to achieve the same effect by setting the timestamp attributes with the wanted values and saving.

Current PR Issues

Breaks backwards compatibility if users are abusing this feature to only update the updated_at attribute of a model without doing any further changes, but the same effect can be easily achieved with:

// Before
myModel.save() // updated_at got updated to the current date

// Now
myModel.save({updated_at: new Date()})

- These are actually unit tests, not integration.

- This fixes the automatic timestamp handling to behave like the Rails' ActiveRecord counterpart. - The updated_at attribute now isn't automatically updated if the model hasn't changed. - User supplied values of the created_at and updated_at attributes always override the automatic values.

- This feature has been deprecated in favor of setting the value of timestamps directly on the model's attributes.

ricardograca · 2018-03-25T19:53:49Z

@kirrg001 Care to take a look? There are a couple of changes here.

kirrg001 · 2018-03-25T20:29:29Z

The changes sound logical to me 👍

If timestamps are enabled:

on insert, both created_at and updated_at are automatically set
- but Bookshelf respects custom values for both properties
on update, only updated_at is automatically set
- if you don't provide a custom value for updated_at and nothing else has changed, updated_at won't change
- if you provide a custom value, updated_at will change

If timestamps are disabled:

respects custom values for updated_at and created_at

The options.date functionality of .save() to set both timestamp attributes with the same value is deprecated and will be removed in the next version. It's already possible to achieve the same effect by setting the timestamp attributes with the wanted values and saving.

Agree

In theory, changing the creation date of a resource is a question mark, but up to the developer.

Thanks for sharing 👍

kirrg001 · 2018-03-26T07:44:01Z

and in the case of #1583 it introduced a bug that prevented the updated_at attribute from being automatically updated in some cases.

We just discovered this bug in Ghost. Our updated_at field doesn't get updated anymore.

When does the patch release goes out? 😎This is critical bug.

ricardograca · 2018-03-26T10:28:56Z

The only problem is that this PR introduces a potentially breaking change, so it can't be a patch release. However, it should be possible to branch off from the 0.13.2 release commit and create a new 0.13.3 build from that without the breaking change.

Basically the only difference is that this line should be changed to:

if (isNewModel && !setUpdatedAt || !setUpdatedAt) {
  // ...
}

I can do the release if you take care of the rest. The associated test should also be removed BTW.

kirrg001 · 2018-03-26T10:31:27Z

I am fine with releasing the breaking change. We did it last time (see 0.13 breaking changes), so we can continue with it until we have a 1.0.

ricardograca · 2018-03-26T10:37:49Z

Ok then, but I'll direct any angry developers to you 😜

kirrg001 · 2018-03-26T10:39:46Z

@ricardograca lalalalala not looking at any further bookshelf emails anymore 🙈

ricardograca · 2018-03-26T10:54:44Z

Then you won't know that the new release is out 😄

closes TryGhost#9520 - it contains a dependency bump of the latest Bookshelf release - Bookshelf introduced a bug in the last release - see bookshelf/bookshelf#1583 - see bookshelf/bookshelf#1798 - this has caused trouble in Ghost - the `updated_at` attribute was not automatically set anymore --- The bookshelf added one breaking change: it's allow to pass custom `updated_at` and `created_at`. We already have a protection for not being able to override the `created_at` date on update. We had to add another protection to now allow to only change the `updated_at` property. You can only change `updated_at` if you actually change something else e.g. the title of a post. To be able to implement this check i discovered that Bookshelfs `model.changed` object has a tricky behaviour. It remembers **all** attributes, which where changed, doesn't matter if they are valid or invalid model properties. We had to add a line of code to avoid remembering none valid model attributes in this object. e.g. you change `tag.parent` (no valid model attribute). The valid property is `tag.parent_id`. If you pass `tag.parent` but the value has **not** changed (`tag.parent` === `tag.parent_id`), it will output you `tag.changed.parent`. But this is wrong. Bookshelf detects `changed` attributes too early. Or if you think the other way around, Ghost detects valid attributes too late. But the current earliest possible stage is the `onSaving` event, there is no earlier way to pick valid attributes (except of `.forge`, but we don't use this fn ATM). Later: the API should transform `tag.parent` into `tag.parent_id`, but we are not using it ATM, so no need to pre-optimise. The API already transforms `post.author` into `post.author_id`.

closes #9520 - it contains a dependency bump of the latest Bookshelf release - Bookshelf introduced a bug in the last release - see bookshelf/bookshelf#1583 - see bookshelf/bookshelf#1798 - this has caused trouble in Ghost - the `updated_at` attribute was not automatically set anymore --- The bookshelf added one breaking change: it's allow to pass custom `updated_at` and `created_at`. We already have a protection for not being able to override the `created_at` date on update. We had to add another protection to now allow to only change the `updated_at` property. You can only change `updated_at` if you actually change something else e.g. the title of a post. To be able to implement this check i discovered that Bookshelfs `model.changed` object has a tricky behaviour. It remembers **all** attributes, which where changed, doesn't matter if they are valid or invalid model properties. We had to add a line of code to avoid remembering none valid model attributes in this object. e.g. you change `tag.parent` (no valid model attribute). The valid property is `tag.parent_id`. If you pass `tag.parent` but the value has **not** changed (`tag.parent` === `tag.parent_id`), it will output you `tag.changed.parent`. But this is wrong. Bookshelf detects `changed` attributes too early. Or if you think the other way around, Ghost detects valid attributes too late. But the current earliest possible stage is the `onSaving` event, there is no earlier way to pick valid attributes (except of `.forge`, but we don't use this fn ATM). Later: the API should transform `tag.parent` into `tag.parent_id`, but we are not using it ATM, so no need to pre-optimise. The API already transforms `post.author` into `post.author_id`.

davericher · 2018-04-17T21:02:27Z

setting this to null in the model definition (updated at since it does not exist on the model) just breaks
everything. How did this make it through

        Models.JoinLogging.create({
            nick,
            channel,
            user: message.user,
            host: message.host,
        })
            .catch(logger.error);

sql: 'insert into `joinLogging` (`channel`, `host`, `nick`, `null`, `timestamp`, `user`) values (?, ?, ?, ?, ?, ?)'

const JoinLogging = Models.Base.extend({
    tableName: 'joinLogging',
    hasTimestamps: ['timestamp', null],
    soft: false,
});

ricardograca · 2018-04-17T21:11:40Z

Care to elaborate on what "breaks everything" means? We have a test in place for that setting and it is passing so...

davericher · 2018-04-17T21:18:53Z

from 0.13.2 to 0.13.3, having a model defined with no updated_at , so hasTimestamps: ['created_at', null] for example or even just hasTimestamps: ['created_at'] will still try and insert an updated at timestamp when using the Model.create function. Thus throwing a 'no col named undefined/null' depending on how u did it.

davericher · 2018-04-17T21:19:59Z

Your unit test covers .save, not .create

ricardograca · 2018-04-17T21:25:09Z

Ok, thanks for the explanation. Will take a look at it.

mborst · 2018-04-19T14:37:08Z

@ricardograca I disagree with the notion that options.date should be deprecated. Why do you think so?
I assume you think that manually setting createdAt and updatedAt when saving takes care of this, but don't i have to be always aware if the record is currently created or updated then? That was my motivation when introducing this feature: I can stupidly pass in date: myIdeaOfWhatTimeItIs in every save call in my application code and the library then takes care that this timestamp is used where applicable!

ricardograca · 2018-04-19T18:53:30Z

@mborst It was deprecated for the reasons you stated. That feature leaves some ambiguity as to which columns will be updated, so what's the purpose of "stupidly" setting an unknown timestamp column? How are you using this in practice?

mborst · 2018-04-20T10:02:24Z

I am using this in practice as stated: I pass in myIdeaOfWhatTimeItIs everywhere as date, and assume that the library takes care of handling the rest.
In my opinion, I shouldn't have to care about what exactly the library will do in my application code. My brain is already full with the application logic. I know the library will set the right fields, because the developer (in this case I) spent time thinking about it once and took care that in any case, the right timestamps are filled with the right values. That's what I mean by "stupid": I don't need to care, because it has been solved once.

ricardograca · 2018-04-20T11:25:31Z

Ok, it's still not exactly clear to me why you would want to constantly override the date that gets set as now for the automatic timestamp feature, because it's not that automatic anymore in that case, but given that this feature seems to be useful to at least one person I'll remove the deprecation warning on the next release.

ricardograca added 7 commits March 24, 2018 18:32

Move Model#timestamp tests to the correct file

5686f2a

- These are actually unit tests, not integration.

Refactor timestamp tests

4a06cd3

Add more tests to verify user supplied timestamps

ae80dcf

Update timestamps related documentation

e8f5806

Remove Model.save date option test

cf11ffd

- This feature has been deprecated in favor of setting the value of timestamps directly on the model's attributes.

Update timestamp test description

d37eb47

ricardograca added change timestamps labels Mar 25, 2018

ricardograca added this to To Do in Version 0.14.0 via automation Mar 25, 2018

ricardograca moved this from To Do to In progress in Version 0.14.0 Mar 25, 2018

ricardograca changed the title ~~Rg fix timestamps~~ Clean up automatic timestamps feature Mar 25, 2018

ricardograca merged commit 9a49e70 into master Mar 25, 2018

Version 0.14.0 automation moved this from In progress to Done Mar 25, 2018

ricardograca deleted the rg-fix-timestamps branch March 25, 2018 22:40

kirrg001 mentioned this pull request Mar 26, 2018

🐛Fixed updated_at not being updated TryGhost/Ghost#9532

Merged

ricardograca mentioned this pull request Apr 23, 2018

Fix timestamp() setting a key named "null" in some cases #1820

Merged

mborst mentioned this pull request Apr 24, 2018

Undeprecate options.date argument to timestamp #1826

Merged

snyk-bot mentioned this pull request May 7, 2020

[Snyk] Upgrade bookshelf from 0.12.1 to 0.15.2 Financial-Times/fruit#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up automatic timestamps feature #1798

Clean up automatic timestamps feature #1798

ricardograca commented Mar 25, 2018 •

edited

ricardograca commented Mar 25, 2018

kirrg001 commented Mar 25, 2018

kirrg001 commented Mar 26, 2018 •

edited

ricardograca commented Mar 26, 2018

kirrg001 commented Mar 26, 2018 •

edited

ricardograca commented Mar 26, 2018

kirrg001 commented Mar 26, 2018

ricardograca commented Mar 26, 2018

davericher commented Apr 17, 2018 •

edited

ricardograca commented Apr 17, 2018

davericher commented Apr 17, 2018

davericher commented Apr 17, 2018

ricardograca commented Apr 17, 2018

mborst commented Apr 19, 2018

ricardograca commented Apr 19, 2018

mborst commented Apr 20, 2018

ricardograca commented Apr 20, 2018

Clean up automatic timestamps feature #1798

Clean up automatic timestamps feature #1798

Conversation

ricardograca commented Mar 25, 2018 • edited

Introduction

Motivation

Proposed solution

Current PR Issues

ricardograca commented Mar 25, 2018

kirrg001 commented Mar 25, 2018

kirrg001 commented Mar 26, 2018 • edited

ricardograca commented Mar 26, 2018

kirrg001 commented Mar 26, 2018 • edited

ricardograca commented Mar 26, 2018

kirrg001 commented Mar 26, 2018

ricardograca commented Mar 26, 2018

davericher commented Apr 17, 2018 • edited

ricardograca commented Apr 17, 2018

davericher commented Apr 17, 2018

davericher commented Apr 17, 2018

ricardograca commented Apr 17, 2018

mborst commented Apr 19, 2018

ricardograca commented Apr 19, 2018

mborst commented Apr 20, 2018

ricardograca commented Apr 20, 2018

ricardograca commented Mar 25, 2018 •

edited

kirrg001 commented Mar 26, 2018 •

edited

kirrg001 commented Mar 26, 2018 •

edited

davericher commented Apr 17, 2018 •

edited