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

Attributes modified in hooks are not saved #3534

Open
Verdier opened this Issue Apr 14, 2015 · 22 comments

Comments

@Verdier
Contributor

Verdier commented Apr 14, 2015

If you add a hook to (for exemple) a User model, and save an instance of the model with some attributes modified, such as:

user.email = 'toto@toto.com';
user.save();

And if you modify some user attributes into the (for exemple...) afterValidate hook:

function afterUserValidate(user) {
  if(user.changed('email')) {
    return u.md5(user.email).then((md5) => user.emailMd5 = md5);
  }
}

The emailMd5 attribute is not saved. This is because dirty attributes are computed before the pre-save hooks.

So my question: could we imagine to compute dirty attributes after the pre-save hooks?

@janmeier

This comment has been minimized.

Member

janmeier commented May 18, 2015

@Verdier Should be fixed on latest master, can you test it?

@Hoverbear

This comment has been minimized.

Hoverbear commented Jul 3, 2015

Confirming this, I had the same issue, am running latest release.

I make some changes to an item, call .save() on it, have the afterValidate or beforeValidate hooks called, have them set something on the item and correctly return. The result of the save() call is the modified item, but the changes made in the hook are never saved to the database.

sequelize@3.3.2 node_modules/sequelize
├── shimmer@1.0.0
├── depd@1.0.1
├── toposort-class@0.3.1
├── dottie@0.3.1
├── generic-pool@2.2.0
├── inflection@1.7.1
├── node-uuid@1.4.3
├── validator@3.40.1
├── moment-timezone@0.4.0
└── lodash@3.10.0

Hook:

            afterValidate: function (member, options, fn) {
                if (member.name &&
                    member.type &&
                    member.gender &&
                    member.birthDate &&
                    member.contactName &&
                    member.contactRelation &&
                    member.contactPhone &&
                    member.medicalNumber)
                {
                    console.log("Marking complete");
                    member.complete = true;
                } else {
                    console.log("Marking incomplete");
                    member.complete = false;
                }
                fn(null, member);
            },

@mickhansen mickhansen added the bug label Jul 6, 2015

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jul 6, 2015

@Hoverbear Does it work with member.set('complete', true|false)

@Hoverbear

This comment has been minimized.

Hoverbear commented Jul 7, 2015

I tried doing that as well...

            afterValidate: function (member, options, fn) {
                if (member.name &&
                    member.type &&
                    member.gender &&
                    member.birthDate &&
                    member.contactName &&
                    member.contactRelation &&
                    member.contactPhone &&
                    member.medicalNumber)
                {
                    console.log("Complete");
                    member.set('complete', true);
                } else {
                    console.log("Incomplete");
                    member.set('complete', false);
                }
                fn(null, member);
            },

Still not saving this change. I've also tried using a promise...

            afterValidate: function (member, options) {
                if (member.name &&
                    member.type &&
                    member.gender &&
                    member.birthDate &&
                    member.contactName &&
                    member.contactRelation &&
                    member.contactPhone &&
                    member.medicalNumber)
                {
                    console.log("Complete");
                    member.set('complete', true);
                } else {
                    console.log("Incomplete");
                    member.set('complete', false);
                }
                return new Promise(function (resolve, reject) {
                    return resolve(member);
                });
            },

Both versions output "Complete" (twice!) and proceed to not save the data.

@Hoverbear

This comment has been minimized.

Hoverbear commented Jul 13, 2015

This appears to be isolated to the beforeValidate and afterValidate hooks. I tried beforeCreate and beforeUpdate and they seemed to act reasonable.

@Verdier

This comment has been minimized.

Contributor

Verdier commented Oct 15, 2015

@Hoverbear: Yep, I'm confirm that's only in beforeValidate and afterValidate. Here is a workaround:

afterValidate: function (instance, options) {
 // ... change `instance` fields ...
 options.fields = instance.changed()
}
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 17, 2015

Looks like only beforeCreate is set up to figure out if values changed during a hook (in cases of default .changed() fields): https://github.com/sequelize/sequelize/blob/master/lib/instance.js#L583

Same code would need to be applied to after validation.

@fredrikekelund

This comment has been minimized.

fredrikekelund commented Dec 10, 2015

+1 for this

janmeier added a commit that referenced this issue Apr 2, 2016

Merge pull request #5665 from sushantdhiman/fix-3534
Tests for #3534, values modified in validate hooks are saved

jchbh-duplicate pushed a commit to jchbh-duplicate/sequelize that referenced this issue May 7, 2016

Changhai Jiang
Merge pull request sequelize#5 in SKYLINE/skyline-sequelize from upgr…
…ade-sequelize to master

* commit 'ce88acae0e14b20a5341e42f41adf09041f90100': (395 commits)
  remove unused INTARRAY
  bump fix version
  revert undocumented transaction inherits event emitter
  revert unecessary boolean coercion
  revert query 3 argument support
  space issue
  Disables setting AUTOCOMMIT for Postgres >= 9.4.0
  Revert "disable autocommit for postgresql 9.5"
  3.21.0
  changelog for v3.21.0
  Support calling setAssociation twice on hasOne. Closes sequelize#5315
  [ci skip] babel-preset-es2015@6.6.0. Closes sequelize#5505
  Rewriting of the benchmarking feature
  Update validation to return null
  Add unit-tests for custom validation functions
  (tests) sequelize#3534, values modified in validate hooks are saved
  fix limit=0 issue
  chore: typo in test description
  add: beforeConnect hook
  add: support single object as Sequelize constructor parameter
  ...
@goodwill

This comment has been minimized.

goodwill commented Dec 11, 2016

Tried on 3.27 still have same problem with beforeValidate

All I did was use a model lookup for an email field, and then save the UserId if email matches, wasted me 2 hours and turn out not working. Using beforeUpdate works.

@sushantdhiman

This comment has been minimized.

Member

sushantdhiman commented Dec 11, 2016

Its not ported to v3, its in master branch i.e. v4

@goodwill

This comment has been minimized.

goodwill commented Dec 11, 2016

In this case when is 4 suppose to release?

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Dec 11, 2016

soon, hopefully before mid-January. See the milestone for release blockers.

@svvac

This comment has been minimized.

svvac commented Apr 10, 2017

Any chance to see this backported to 3.x series ?

@mrstif

This comment has been minimized.

mrstif commented Jul 5, 2017

Hi guys,

Just upgraded from v3.30.4 to v4.2.1 to try and solve this issue but apparently it's still there...

The tests included by @sushantdhiman aren't really testing the whole scenario since what's persisted in the database isn't actually what's returned in the save/update callback. I hit the same pitfall in which the object in the callback is updated whereas getting the record directly from the database is not...

I'll try to change the tests to show what I mean.

@xitij

This comment has been minimized.

xitij commented Sep 14, 2017

In a related issue I'm seeing that when you update a field in beforeCreate it works fine (the value is saved). However, removing fields isn't saved properly. Ex.

beforeCreate: function (object, options) {
  if (object.data === 'some value') {
    delete object.data;
  }
}

When the value is returned after the create object.data = 'some value'

@arungangula

This comment has been minimized.

arungangula commented Nov 26, 2017

I fixed the issue by calling the .save() method in the hooks

@akenger

This comment has been minimized.

akenger commented Nov 29, 2017

This bug still exists in 4.22.12. If you set an attribute in a beforeUpdate hook that was not part of the original list of update fields, the updated field appears in the response but not in the database.
You can't call .save in a beforeUpdate hook b/c it creates an endless loop.

@sushantdhiman sushantdhiman reopened this Nov 30, 2017

@mariotacke

This comment has been minimized.

mariotacke commented Feb 15, 2018

Running into the same thing right now (pg: 7.4.1, sequelize: 4.33.3). I'm trying to reset my updatedAt column before it updates:

hooks: {
  beforeUpdate: (entity) => {
    entity.updatedAt = null;
  },
},

As a work-around I can manually set updatedAt in my update call, but I'd much rather set it in the model definition.

@davidmicksch

This comment has been minimized.

davidmicksch commented Feb 17, 2018

I had a similar problem. It turns out that I was using bcrypt in an async mode in a beforeSave hook to encrypt the user's password. This worked fine for the create process. However, for the update process, this ended up writing the unencrypted (original value) of the password. I switched over to using the synchronous mode of bcrypt and it worked fine for both the create and update.

With the async version, it did actually encrypt the password and assign it back to the attribute, but that was too late to take effect.

@JSandmark

This comment has been minimized.

Contributor

JSandmark commented Feb 20, 2018

Surprised to find this issue! Same issue for me on Sequelize CLI [Node: 8.9.0, CLI: 3.2.0, ORM: 4.32.6], changing date-data in the beforeValidate hook when calling model.save() is not persisted. I am returning a promise from the hook and in my tests only dates are affected (not saved). Could someone else please confirm?

myDate: {
      type: DataType.DATE,
    },
@ThaisFaria

This comment has been minimized.

ThaisFaria commented Mar 26, 2018

I called .save on the beforeDestroy hook and it worked but as pointed above, it will not work for update as it will cause an infinite loop.

@SolveSoul

This comment has been minimized.

SolveSoul commented May 8, 2018

I'm at version 4.37 and I'm also having this issue. As pointed out above when using create everything works fine but when upserting the altered values in the hook are ignored. Everything inside the hook is running sync so that should not be an issue. Even when logging the values at the last line of the hook I can see the altered values, but they are not used in the Sequelize query :/

Any updates in this issue?

EDIT: When logging the value with a create it seems you get the complete Sequelize model, when running an upsert you only seem to get the raw values, which might explain why it isn't working.

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