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

option object shouldn't be updated internally. #4023

Merged
merged 5 commits into from Aug 4, 2015

Conversation

2 participants
@smihica
Contributor

smihica commented Jun 28, 2015

I got "not-null constraint" error because the previous save(opt) call inserted "fields" into opt.
This may cause confusing errors.

@smihica

This comment has been minimized.

Contributor

smihica commented Jun 29, 2015

This pr is for the following bug.
#4011

@@ -477,7 +477,7 @@ Instance.prototype.save = function(options) {
if (arguments.length > 1) {
throw new Error('The second argument was removed in favor of the options object.');
}
options = _.defaults(options || {}, {
options = _.defaults({}, options || {}, {

This comment has been minimized.

@janmeier

janmeier Jun 29, 2015

Member

I don't think we need options || {}, options should be fine

This comment has been minimized.

@smihica

smihica Jun 29, 2015

Contributor

Oh, exactly.

@@ -818,7 +818,7 @@ Instance.prototype.destroy = function(options) {
for(var i = 0; i < primaryKeys.length; i++) {
where[this.Model.rawAttributes[primaryKeys[i]].field] = this.get(primaryKeys[i], { raw: true });
}
return this.sequelize.getQueryInterface().delete(this, this.Model.getTableName(options), where, _.defaults(options, { type: QueryTypes.DELETE,limit: null}));
return this.sequelize.getQueryInterface().delete(this, this.Model.getTableName(options), where, _.defaults({}, options, { type: QueryTypes.DELETE,limit: null}));

This comment has been minimized.

@janmeier

janmeier Jun 29, 2015

Member

Can't we just use options here?

This comment has been minimized.

@smihica

smihica Jun 29, 2015

Contributor

yeah I think so. thank you.

@janmeier

This comment has been minimized.

Member

janmeier commented Jun 29, 2015

Looks good @smihica - do you mind adding a few tests. Preferrably unit tests, since they are faster. Something along the lines of

stub(sequelize, 'query').returns(Promise.resolve([]));

var options = { ... }

model.reload(options);

expect(options).to.deep.equal({... }

If you have too many problems trying to stub sequelize.query, integration tests that hit the db will be fine as well

@smihica

This comment has been minimized.

Contributor

smihica commented Jun 29, 2015

Yes, I'll try to add some tests later. thanks reviewing.

@smihica smihica force-pushed the smihica:#4011 branch from f0afe11 to 8cd25ce Aug 4, 2015

@smihica

This comment has been minimized.

Contributor

smihica commented Aug 4, 2015

@janmeier
OK. I added tests and rebased. And all unit and integration tests were passed in my environment.

janmeier added a commit that referenced this pull request Aug 4, 2015

Merge pull request #4023 from smihica/#4011
option object shouldn't be updated internally.

@janmeier janmeier merged commit 03008f7 into sequelize:master Aug 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@janmeier

This comment has been minimized.

Member

janmeier commented Aug 4, 2015

Solid work on the tests @smihica ! Thanks for this, and the other PR also :)

janmeier added a commit that referenced this pull request Aug 4, 2015

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