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.

Sign up for GitHub

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

Jump to bottom

Inconsistent behavior for paranoid records since #5897 (see issue #7204) #7290

Closed
wants to merge 5 commits into from
Closed

Inconsistent behavior for paranoid records since #5897 (see issue #7204) #7290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1f56e06
Fix for 7204 part 1 (see also 5880 & 5897)
Wsiegenthaler Feb 25, 2017
fe5c3ad
Fix for 7204 part 2 (see also 5880 & 5897)
Wsiegenthaler Feb 25, 2017
efec443
Tests for 7204 (see also 5880 & 5897)
Wsiegenthaler Feb 25, 2017
6e3d473
Changelog entries for 7204
Wsiegenthaler Feb 25, 2017
1353275
Merge branch 'master' into deleted-at-fix-pr
Wsiegenthaler Mar 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.md
View file
Open in desktop
Expand Up @@ -54,6 +54,8 @@
- [FIXED] Add quotes around column names for unique constraints in sqlite [#4407](https://github.com/sequelize/sequelize/pull/4407)
- [FIXED] Connection error when fetching OIDs for unspported types in Postgres 8.2 or below [POSTGRES] [#5254](https://github.com/sequelize/sequelize/issues/5254)
- [FIXED] Expose OptimisticLockError on Sequelize object [#7291](https://github.com/sequelize/sequelize/pull/7291)
- [FIXED] Always use database time for populating and evaluating deletedAt field on paranoid records. [#7204](https://github.com/sequelize/sequelize/issues/7204)
- [FIXED] Deleted paranoid records can be queried in the same second. [#7204](https://github.com/sequelize/sequelize/issues/7204)

## BC breaks:
- `DATEONLY` now returns string in `YYYY-MM-DD` format rather than `Date` type
Expand Down
6 changes: 3 additions & 3 deletions lib/model.js
View file
Open in desktop
Expand Up @@ -77,7 +77,7 @@ class Model {
const deletedAtObject = {};
let deletedAtDefaultValue = deletedAtAttribute.hasOwnProperty('defaultValue') ? deletedAtAttribute.defaultValue : null;

deletedAtDefaultValue = deletedAtDefaultValue || { $or: { $gte: model.sequelize.literal('CURRENT_TIMESTAMP'), $eq: null } };
deletedAtDefaultValue = deletedAtDefaultValue || { $or: { $gt: model.sequelize.literal('CURRENT_TIMESTAMP'), $eq: null } };

deletedAtObject[deletedAtAttribute.field || deletedAtCol] = deletedAtDefaultValue;

Expand Down Expand Up @@ -2465,7 +2465,7 @@ class Model {

where[field] = deletedAtAttribute.hasOwnProperty('defaultValue') ? deletedAtAttribute.defaultValue : null;

attrValueHash[field] = Utils.now(this.sequelize.options.dialect);
attrValueHash[field] = this.sequelize.literal('CURRENT_TIMESTAMP');
return this.QueryInterface.bulkUpdate(this.getTableName(options), attrValueHash, _.defaults(where, options.where), options, this.rawAttributes);
} else {
return this.QueryInterface.bulkDelete(this.getTableName(options), options.where, options, this);
Expand Down Expand Up @@ -3697,7 +3697,7 @@ class Model {
const field = attribute.field || this.constructor._timestampAttributes.deletedAt;
const values = {};

values[field] = new Date();
values[field] = this.sequelize.literal('CURRENT_TIMESTAMP');
where[field] = attribute.hasOwnProperty('defaultValue') ? attribute.defaultValue : null;

this.setDataValue(field, values[field]);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/instance.test.js
View file
Open in desktop
Expand Up @@ -1810,7 +1810,7 @@ describe(Support.getTestDialectTeaser('Instance'), function() {
return this.ParanoidUser.create({ username: 'fnord' }).then(function() {
return self.ParanoidUser.findAll().then(function(users) {
return users[0].destroy().then(function() {
expect(users[0].deletedAt.getMonth).to.exist;
expect(users[0].deletedAt.val).to.be.equal('CURRENT_TIMESTAMP');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be populated with actual date object, which is returned by CURRENT_TIMESTAMP ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since updates only return the number of affected rows, we would need to issue a subsequent SELECT to retrieve the actual timestamp assigned by the database.


return users[0].reload({ paranoid: false }).then(function(user) {
expect(user.deletedAt.getMonth).to.exist;
Expand Down
7 changes: 1 addition & 6 deletions test/integration/model.test.js
View file
Open in desktop
Expand Up @@ -1333,22 +1333,17 @@ describe(Support.getTestDialectTeaser('Model'), function() {
return ParanoidUser.sync({ force: true }).then(function() {
return ParanoidUser.bulkCreate(data);
}).bind({}).then(function() {
// since we save in UTC, let's format to UTC time
this.date = moment().utc().format('YYYY-MM-DD h:mm');
return ParanoidUser.destroy({where: {secretValue: '42'}});
}).then(function() {
return ParanoidUser.findAll({order: ['id']});
}).then(function(users) {
expect(users.length).to.equal(1);
expect(users[0].username).to.equal('Bob');

return self.sequelize.query('SELECT * FROM ' + qi('ParanoidUsers') + ' WHERE ' + qi('deletedAt') + ' IS NOT NULL ORDER BY ' + qi('id'));
return self.sequelize.query('SELECT * FROM ' + qi('ParanoidUsers') + ' WHERE ' + qi('deletedAt') + ' IS NOT NULL AND ' + qi('deletedAt') + ' <= CURRENT_TIMESTAMP ORDER BY ' + qi('id'));
}).spread(function(users) {
expect(users[0].username).to.equal('Peter');
expect(users[1].username).to.equal('Paul');

expect(moment(new Date(users[0].deletedAt)).utc().format('YYYY-MM-DD h:mm')).to.equal(this.date);
expect(moment(new Date(users[1].deletedAt)).utc().format('YYYY-MM-DD h:mm')).to.equal(this.date);
});
});

Expand Down