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

fix(destroy): Properties updated in a beforeDestroy hook are now pers… #9319

Merged
merged 2 commits into from Jul 22, 2018

Conversation

@ddolcimascolo
Copy link
Contributor

@ddolcimascolo ddolcimascolo commented Apr 17, 2018

…isted on soft delete. Closes #9318

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

When getting the list of things to update in the database to perform a soft delete, the code is now considering changed values (through Model.changed()) instead of only updating deletedAt.

Closes issue #9318

@ddolcimascolo
Copy link
Contributor Author

@ddolcimascolo ddolcimascolo commented Apr 17, 2018

I think this should be applied on 4.x branch also, but I'm unsure if I should submit a new PR targetting another branch, or if you automagically cherry-pick commits to backport...

Just tell me, I'm happy to open another PR if needs be.

@codecov
Copy link

@codecov codecov bot commented Apr 17, 2018

Codecov Report

Merging #9319 into master will not change coverage.
The diff coverage is 100%.

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

A few changes

@@ -1433,6 +1433,26 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});
});

it('sets other changed values when soft deleting and a beforeDestroy hooks kicks in', function() {

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 19, 2018
Contributor

Move to https://github.com/sequelize/sequelize/blob/master/test/integration/hooks/destroy.test.js

Please create a new group for paranoid, under that place this test

This comment has been minimized.

@ddolcimascolo

ddolcimascolo Apr 19, 2018
Author Contributor

Done

@@ -3896,7 +3896,7 @@ class Model {
if (this.constructor._timestampAttributes.deletedAt && options.force === false) {
const attribute = this.constructor.rawAttributes[this.constructor._timestampAttributes.deletedAt];
const field = attribute.field || this.constructor._timestampAttributes.deletedAt;
const values = {};
const values = Utils.mapValueFieldNames(this, this.changed() || [], this.constructor);

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 19, 2018
Contributor

it should be mapValueFieldNames(this.dataValues,....

This comment has been minimized.

@ddolcimascolo

ddolcimascolo Apr 19, 2018
Author Contributor

Done

@@ -1433,6 +1433,26 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});
});

it('sets other changed values when soft deleting and a beforeDestroy hooks kicks in', function() {
const ParanoidUser = this.sequelize.define('ParanoidUser', {

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 19, 2018
Contributor

Also add a test where you try to update a virtual field in hook and see it doesn't get updated (i.e. it should not throw error for unknown column)

This comment has been minimized.

@ddolcimascolo

ddolcimascolo Apr 19, 2018
Author Contributor

Done

@ddolcimascolo ddolcimascolo force-pushed the ddolcimascolo:issue-9318 branch from 591e2ca to d1be757 Apr 19, 2018
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

LGTM, some cosmetic changes

@@ -76,6 +76,51 @@ describe(Support.getTestDialectTeaser('Hooks'), () => {
});
});
});

describe('with paranoid mode enabled', () => {

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 19, 2018
Contributor

remove extra line

expect(user.virtualField).to.equal(0);
});
});

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 19, 2018
Contributor

remove extra line

});

});

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 19, 2018
Contributor

remove extra line

…isted on soft delete.

Closes #9318
@ddolcimascolo ddolcimascolo force-pushed the ddolcimascolo:issue-9318 branch from d1be757 to bb42b17 Apr 19, 2018
@ddolcimascolo
Copy link
Contributor Author

@ddolcimascolo ddolcimascolo commented Apr 19, 2018

@sushantdhiman Done, sorry for the extra blank lines

@sushantdhiman sushantdhiman requested a review from sequelize/orm Apr 19, 2018
@ddolcimascolo
Copy link
Contributor Author

@ddolcimascolo ddolcimascolo commented Apr 19, 2018

Thanks for the appoval, what about the backport to v4 ?

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 19, 2018

Thanks for the appoval, what about the backport to v4 ?

I'll cherry pick commit from master and port to v4 :)

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 20, 2018

I was checking other use of this hook, we need to apply this change to

sequelize/lib/model.js

Lines 2546 to 2568 in 4caa17c

// Get daos and run beforeDestroy hook on each record individually
if (options.individualHooks) {
return this.findAll({where: options.where, transaction: options.transaction, logging: options.logging, benchmark: options.benchmark})
.map(instance => this.runHooks('beforeDestroy', instance, options).then(() => instance))
.then(_instances => {
instances = _instances;
});
}
}).then(() => {
// Run delete query (or update if paranoid)
if (this._timestampAttributes.deletedAt && !options.force) {
// Set query type appropriately when running soft delete
options.type = QueryTypes.BULKUPDATE;
const attrValueHash = {};
const deletedAtAttribute = this.rawAttributes[this._timestampAttributes.deletedAt];
const field = this.rawAttributes[this._timestampAttributes.deletedAt].field;
const where = {};
where[field] = deletedAtAttribute.hasOwnProperty('defaultValue') ? deletedAtAttribute.defaultValue : null;
attrValueHash[field] = Utils.now(this.sequelize.options.dialect);
return this.QueryInterface.bulkUpdate(this.getTableName(options), attrValueHash, Object.assign(where, options.where), options, this.rawAttributes);

So individualHooks: true cases can use this as well. Will need extra tests.

Also Model.destroy is using is Utils.now but Model.prototype.destroy is setting new Date, probably both need to use Utils.now

This change may be unexpected for some users and definitely change hook behaviour (#9318 (comment)) so its better to keep this in v5

@ddolcimascolo
Copy link
Contributor Author

@ddolcimascolo ddolcimascolo commented Apr 23, 2018

Fine to keep this v5 only.
Regarding your latest comments I can handle the changes and add tests for the bulk destroy use cases. Will try to do this later this day and update the PR.

Cheers

@ddolcimascolo
Copy link
Contributor Author

@ddolcimascolo ddolcimascolo commented Apr 23, 2018

@sushantdhiman Sorry, I'm puzzled 😀

Model.destroy will indeed call the beforeDestroy hook on every instance impacted by the bulk destroy, but then how is the code supposed to "know" that some values changed on the instances, how is the code supposed to translate that in the bulk UPDATE statement that is issued to the database ?
We could even imagine that the hook has changed different attributes on different instances...

I'm thinking that the code should individually call destroy on each instance instead of doing a bulk operation, if and only if individualHooks: true is passed.

That would sum up to something like (pseudo-code):

runHook('beforeBulkDestroy')
if (individualHooks) {
  for each instance returned by findAll()
    instance.destroy()
} else {
  doTheBulkOperation
}
runHook('afterBulkDestroy')

Calling instance.destroy() will indeed run the before/afterDestroy hooks on it, which is what is expected by individualHooks: true anyway.

WDYT?

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 30, 2018

@ddolcimascolo Good question and your pseudo-code is exactly what we should do (like the update case), I reckon we need this change for restore process as well.

@ddolcimascolo
Copy link
Contributor Author

@ddolcimascolo ddolcimascolo commented Jul 5, 2018

Hey guys (and @sushantdhiman),

Can you share the status of this contribution? Do you need more code from me or can we consider it good to merge, eventually opening another issue to track the refactoring of the bulk 'destroy' operation?

For what it's worth, we're using this patch in production for several weeks now without any issue. It fits well with bonaval/sequelize-temporal#7 which I contributed too.

Thanks,
Regards,

David

@sushantdhiman sushantdhiman requested review from janmeier and eseliger Jul 5, 2018
@sushantdhiman sushantdhiman merged commit 0fe77d6 into sequelize:master Jul 22, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 95.98%)
Details
codecov/project 95.98% (+0%) compared to d65cc7b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants