Attributes changed during event hook are not persisted

[naz]

• Related Issues: attrs in 'creating' event handler only contains 'created_at' and 'updated_at' #1433
• Related PRs: Fix missing attributes in some events #1934

Introduction

Changes made to object attributes during saving event hook are not persisted.

Issue Description

When hooking into saving event of the object and making changes through set('attrName', value) during the hook, those changes are not later persisted in the database.

The issue was spotted during version bump from 0.14.2 -> 0.15.1 in Ghost.
An example attribute change that is not being picked is one from this line.

When digging through the source code the main difference is that before this PR which introduced attributesToSave, attributes were taken from this.attributes after triggerThen was done with it's logic.

Steps to reproduce issue

1. Hook into 'saving' event in the model
2. Change one of the attributes of the saved object through object.set('attrName', value)
3. Check the record persisted in the database and the attribute hasn't changed

Expected behaviour

When making changes to the saved object during even hooks they should be persisted.

Actual behaviour

The changes done during the saving event hook are not persisted.

[ricardograca]

Can you provide an actual reproducible test case? A gist is fine.

[naz]

Hi @ricardogama was trying to reproduce this bug in a clean environment and created this script which is very close to how the 'edit' works in Ghost model layer but with no luck.

Looks like it's something internal that messes with this in different contexts, because attributesToSave which is assigned from this.attributesis not being edited by reference properly somewhere along the lines and when logging this.attributes and attributesToSave in this step two values are different - this.attributes contains the changes done in saving hooks but attributesToSave doesn't.

Closing this issue as it seems like there's a problem on our side. Thanks for your time!

[naz]

Hey @ricardogama 👋 I have been working on an update to new bookshelf in Ghost again and finally was able to reproduce the scenario that causes the above issue. Here is a gist with bare minimum reproducible scenario - https://gist.github.com/gargol/c21f2069e176daaafe897718ce62c8fa.

This line - https://gist.github.com/gargol/c21f2069e176daaafe897718ce62c8fa#file-bookself_bug_investigation-js-L28, is causing the attributesToSave value in save method to not be picked up correctly. It's something similar that happens in Ghost core to white-list attributes that model can manipulate here - https://github.com/TryGhost/Ghost/blob/master/core/server/models/base/index.js#L406-L409.

My question regarding this behaviour is following: from bookshelf's perspective is it allowed to operate directly on this.attributes property like above or that should be rather avoided and only this.set(...) should be used to do any attribute modification?

P.S. Also spotted inconsistency related to this behaviour for insert scenario which is described here: https://gist.github.com/gargol/c21f2069e176daaafe897718ce62c8fa#file-bookself_bug_investigation-js-L77-L80

[ricardogama]

👋

[naz]

Sorry, was meant to cc @ricardograca

[ricardograca]

Good findings. I'd say it's not expected that users modify this.attributes directly, but if we can fix this use case easily I don't see why not. Seems possible from a quick look at that past PR.

[naz]

Hey @ricardograca 👋 Thanks for quick reply! If the above fix is considered, would it be possible to ship it not just with the next planned version but also as a patch for v0.15.x? This would make the upgrade in Ghost a lot less painful 🤞 Thank you!

[ricardograca]

Sure. I'll create a 0.15 branch and you can then apply your changes there.

[ricardograca]

Done: https://github.com/bookshelf/bookshelf/tree/0.15