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

Clean up automatic timestamps feature #1798

Merged
merged 7 commits into from
Mar 25, 2018
Merged

Conversation

ricardograca
Copy link
Member

@ricardograca ricardograca commented Mar 25, 2018

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 ricardograca changed the title Rg fix timestamps Clean up automatic timestamps feature Mar 25, 2018
@ricardograca
Copy link
Member Author

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

@kirrg001
Copy link
Contributor

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 👍

@ricardograca ricardograca merged commit 9a49e70 into master Mar 25, 2018
@ricardograca ricardograca deleted the rg-fix-timestamps branch March 25, 2018 22:40
@kirrg001
Copy link
Contributor

kirrg001 commented Mar 26, 2018

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
Copy link
Member Author

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
Copy link
Contributor

kirrg001 commented Mar 26, 2018

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
Copy link
Member Author

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

@kirrg001
Copy link
Contributor

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

@ricardograca
Copy link
Member Author

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

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Mar 26, 2018
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`.
kevinansfield pushed a commit to TryGhost/Ghost that referenced this pull request Mar 26, 2018
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
Copy link

davericher commented Apr 17, 2018

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
Copy link
Member Author

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

@davericher
Copy link

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
Copy link

Your unit test covers .save, not .create

@ricardograca
Copy link
Member Author

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

@mborst
Copy link
Contributor

mborst commented Apr 19, 2018

@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
Copy link
Member Author

@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
Copy link
Contributor

mborst commented Apr 20, 2018

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
Copy link
Member Author

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.

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

Successfully merging this pull request may close these issues.

4 participants