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

Adding dirty tracking #798

Merged
merged 3 commits into from Aug 2, 2013

Conversation

8 participants
@mex
Contributor

mex commented Jul 31, 2013

This will add a dirty flag to objects.

New objects, that are built but not saved, will be flagged as dirty until they are saved.
Objects fetched from the database are not flagged as dirty, but if you change an attribute on that object without saving, the object will be flagged as dirty. If the attribute was set, but not actually changed, it will not be flagged as dirty.

As this tracking is hooked into the setters of the objects, it originally got flagged as dirty every time an object was created or built/saved, because id, createdAt, touchedAt, and updatedAt would be set, which I wrote check for. This is not the most beautiful thing to do, but I haven't found an alternative way of doing this.

Tested with MySQL and SQLite.

@noazark

This comment has been minimized.

noazark commented Jul 31, 2013

👍 way, way 👍

@durango

This comment has been minimized.

Member

durango commented Jul 31, 2013

Dude, we were just talking about this on IRC last night, was about to do it xD although some of your postgres tests fails, send me a ping when it's fixed :) Very cool dude, and thanks for your awesome work.

@mstorgaard Could you do me a huge favor? And change all of the assertions to check for true/false instead of falsy/truthy since you're using straight booleans? :) Should just be expect(var).to.be.false or expect(var).to.be.true

I did a quick glance at your code and it seems fairly solid, again thanks :)

lib/dao.js Outdated
DAO.prototype.setDataValue = function(name, value) {
if (this.dataValues[name] != value) {

This comment has been minimized.

@durango

durango Jul 31, 2013

Member

Could you do !== ?

lib/dao.js Outdated
Utils._.each(updates, function(value, attr) {
var updateAllowed = (
(readOnlyAttributes.indexOf(attr) == -1) &&
(readOnlyAttributes.indexOf(Utils._.underscored(attr)) == -1) &&
(self.attributes.indexOf(attr) > -1)
)
updateAllowed && (self[attr] = value)
if (updateAllowed) {
if (self[attr] != value) {

This comment has been minimized.

@durango
lib/dao.js Outdated
@@ -330,7 +345,15 @@ module.exports = (function() {
// (the same is true for __defineSetter and 'prototype' getters)
if (has !== true) {
this.__defineGetter__(attribute, has.get || function() { return this.dataValues[attribute]; });
this.__defineSetter__(attribute, has.set || function(v) { this.dataValues[attribute] = v; });
this.__defineSetter__(attribute, has.set || function(v) {
if (this.dataValues[attribute] != v) {

This comment has been minimized.

@durango
lib/dao.js Outdated
this.isDirty = true
}
}
this.dataValues[attribute] = v;

This comment has been minimized.

@durango

durango Jul 31, 2013

Member

Drop semi-colon before @sdepold sees it! haha

Added helper method for checking change (incl. Date and empty values)…
…, fixed some small stuff and added more tests
@durango

This comment has been minimized.

Member

durango commented Jul 31, 2013

Solid work, me and @sdepold was just discussing this a little bit on IRC, it definitely has my approval, but I'll bring this to his attention to see if he had something else in mind (I don't think so personally but I'd like to double check on this). Again, thanks for the PR! :) Huge win for Sequelize

@@ -84,10 +84,15 @@ module.exports = (function() {
DAO.prototype.getDataValue = function(name) {
return this.dataValues && this.dataValues.hasOwnProperty(name) ? this.dataValues[name] : this[name]
}
DAO.prototype.get = DAO.prototype.getDataValue

This comment has been minimized.

@sdepold

sdepold Aug 1, 2013

Member

❤️

lib/dao.js Outdated
DAO.prototype.setDataValue = function(name, value) {
if (Utils.areChanged(this.dataValues[name], value)) {

This comment has been minimized.

@sdepold

sdepold Aug 1, 2013

Member

can we rename that method to differs or didChange ?

this.dataValues[name] = value
}
DAO.prototype.set = DAO.prototype.setDataValue

This comment has been minimized.

@sdepold

sdepold Aug 1, 2013

Member

❤️

This comment has been minimized.

@durango

durango Aug 1, 2013

Member

For real, my jaw dropped in awe at how simple yet powerful that single statement was :)

This comment has been minimized.

@iamjochem

iamjochem Aug 1, 2013

Contributor

what is so wonderful about this? what am I missing!

@sdepold

This comment has been minimized.

Member

sdepold commented Aug 1, 2013

cool. like. i would prefer the renaming of areChanged to differs or didChange. Everything else is cool. Thanks a lot for that PR @mstorgaard

@durango

This comment has been minimized.

Member

durango commented Aug 1, 2013

@sdepold what about hasChanged ?

lib/dao.js Outdated
this.__defineSetter__(attribute, has.set || function(v) {
if (Utils.areChanged(this.dataValues[attribute], v)) {
//Only dirty the object if the change is not due to id, touchedAt, createdAt or updatedAt being initiated
if (this.dataValues[attribute] || (attribute != 'id' && attribute != 'touchedAt' && attribute != 'createdAt' && attribute != 'updatedAt')) {

This comment has been minimized.

@durango

durango Aug 1, 2013

Member

We'll also need to check if underscore is true and if so the fields are deleted_at, created_at, etc.

if (Utils._.isDate(attrValue)) {
return attrValue.valueOf() !== value.valueOf()
}
//If both are them are empty, don't set as changed

This comment has been minimized.

@ricardograca

ricardograca Aug 1, 2013

Contributor

Typo, should be: "...both of them...".

@mex

This comment has been minimized.

Contributor

mex commented Aug 1, 2013

I renamed areChanged to didChange and added support for underscore in the setter.

And thank you for all the kind words!

@sdepold sdepold merged commit a9524ac into sequelize:master Aug 2, 2013

1 check failed

default The Travis CI build failed
Details
@sdepold

This comment has been minimized.

Member

sdepold commented Aug 2, 2013

Thank you!

@durango I renamed it to hasChanged

@danhstevens

This comment has been minimized.

danhstevens commented Feb 25, 2014

Is this (or will this be) merged into the 2.0 branch?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 25, 2014

@danhstevens if it's in master it's in 2.0.
Although isDirty now implements .changed() instead (.changed() returns an array of keys that have changed or true or false if provided a key ala .changed(key))

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