Fix inconsistent previous attributes

[ricardograca]

• Related Issues: puzzled by previousAttributes #1520, Previous Attributes after save() call #326

Introduction

This makes previousAttributes() and previous() behave like users expect them to, and always return the actual previous attributes.

Also updates the documentation to make it clearer.

Motivation

The current behavior causes some confusion, which isn't helped by the documentation not being very clear about it, and it seems to be a bit different from what other ORMs do. It's also not very useful to have the previous attributes be reset back to match the current attributes on each save() or destroy() call. This makes these methods far less useful since they can only be used to check for the previous attributes before the model is saved or destroyed.

Proposed solution

This changes the previous() and previousAttributes() methods so that their values are not reset back to the current attributes when saving or destroying a model. This means that the previous attributes are accessible after any of these operations, making them way more useful.

This is a breaking change, but currently it's a bit useless having these methods return the same values that are already set on the model after a save(), so I doubt it will cause problems for anyone.

Fixes #1520.

Alternatives considered

Implementing additional methods to get the previous attributes before a save(), but that would require adding more properties to models, more methods, and more chance for confusion. It would mean this change wouldn't break backwards compatibility though.

[ricardograca]

@ErisDS @kirrg001 Care to check this out? This makes your custom _updatedAttributes implementation redundant, but it also changes the behavior of previousAttributes() so you may want to test these changes beforehand.

[kirrg001]

@ricardograca Sorry for delay - i was on vacation 😎Will look at it asap.

[kirrg001]

Is this line still needed? 🤔 Because we no longer reset the previous attributes in the reset fn. Just curious.

[ricardograca]

Good question. Probably not. Definitive answer in a few minutes.

[ricardograca]

@kirrg001 Doesn't seem to be needed. On top of that options.previousAttributes is not documented anywhere, so I doubt many people are using it. For anyone using it it's just a matter of changing options.previousAttributes to model.previousAttributes() in the event listeners.

I'll wait for further reviews before changing this, so I can do it all in one go.

[kirrg001]

If i fetch relations with withRelated, the relational models have _previousAttributes: {}.

[ricardograca]

That doesn't seem right. I also just found one other somewhat related test that would pass in such a situation when it should fail.

There's also no test for that particular scenario you described. I'll have to fix that.

[ricardograca]

@kirrg001 All reported issues fixed, and I added some tests to check for them.

[kirrg001]

@ricardograca Great to hear - i'll look at them asap :)

[kirrg001]

If i fetch relations with withRelated, the relational models have _previousAttributes: {}.

This is resolved 👍

---

But when i install this PR as tarball, i am having a different problem. Maybe unrelated to this PR.

onFetchingCollection ('fetching:collection')

This event no longer passes the options - it's always undefined.

First argument: is correct, the collection
Second argument: a function (?)
Third argument: undefined (which should be options)

[ricardograca]

That hasn't changed AFAIK. Looking at the relevant code, the options are being provided to triggerThen as the third argument, so it must be something before that. I'll investigate, but it's probably not related to these changes.

[kirrg001]

@ricardograca If you have a guess, let me know. I've also debugged a little, suddenly the options and columns values change after the fetching event.

[ricardograca]

I used git bisect and the issue was introduced in 2ebd1af which was the commit that converted the source code to vanilla JavaScript compatible with Node4, so it has nothing to do with these changes. I'll investigate and fix that in another PR.

My guess is that it's something in the .once() method.

[ricardograca]

@kirrg001 Done.

[kirrg001]

Let's say i fetch a user and it returns

{attributes: {status: active, ...more attributes...}, previousAttributes: {}}

Now i call user.set('status', 'active') and user.save() (status did not change !)

I am listening on the user updated event and i receive:

{attributes: {status: active}, previousAttributes: {}, changed: {}}

Is that expected? I am wondering why the previous attributes object is empty in the updated event 🤔 I think as soon as you save a model without fields being modified, i am receiving _previousAttributes: {}, which feels inconsistent? Let me know :)

I think the behaviour was different before?

[ricardograca]

It was different before but only because previously previousAttributes would be populated to match the current attributes with each database operation, therefore what you were getting after .save() were the current attributes (after save), but if you had actually changed any attributes you wouldn't be able to see the actual previous attributes. That's why the use case you describe appeared to work.

However I think you're correct and it does seem like if the status attribute appears in attributes then the previousAttributes should also contain the previous value, even though it's the same. changed will continue to be an empty object and that's how you would determine if a certain attribute changed anyway.

[ricardograca]

@kirrg001 Are you sure you're testing with the latest changes and the correct branch? This test ensures that calling previousAttributes() will return the current attributes if no changes were done to the model after fetch.

I haven't checked the case with the updated event yet though.

[ricardograca]

I just checked the use case you mentioned and it's working as expected. How are you getting that behavior of empty previousAttributes() after save() if the attributes haven't changed?

[ricardograca]

I've added a few more tests to ensure the behavior you mention is observed: 3cdfa6e

[kirrg001]

I am on the correct commit. I just double checked.

Steps:

• fetch
• user status is active
• user.set('status', 'active')
• user.save()
• updated event -> prev attrs are empty

[ricardograca]

As you can see the test case is passing, and it seems to do exactly what you're saying. Maybe you have some plugin interfering with this, or some other event listener, or something else.

I'm writing a simple stand-alone test case that you can use to try to reproduce the behavior, but at this point I think Bookshelf itself is working fine. Oh yeah, I remember you had a custom property in Ghost models for storing the proper previous attributes, before this change. Could it be related to that?

[ricardograca]

Here is the gist: https://gist.github.com/ricardograca/da68dc091fcbe15c395558c166fd7200

[kirrg001]

Oh yeah, I remember you had a custom property in Ghost models for storing the proper previous attributes, before this change. Could it be related to that?

I have commented out all the logic for that.

Here is the gist: https://gist.github.com/ricardograca/da68dc091fcbe15c395558c166fd7200

Thanks will test asap!!

[kirrg001]

The gist works for me too. It must be something else. I am looking into it now.

[kirrg001]

I figured it out. That was quick 🤣

https://gist.github.com/kirrg001/b8650a544db01541828d50fbcd1a227d

Bookshelf returns an empty _previousAttributes object if you fetch on a collection.
And if you then operate on a model of this collection, you end up that all further operations e.g. event hooks contain an empty previous attributes object.

[ricardograca]

Nice work! I'll fix it this weekend hopefully. Apart from that are there any other issues?

[ricardograca]

@kirrg001 Fixed. Nice catch there 🥇

[kirrg001]

💥🔥✨

Thanks so much!