Skip to content

Commit

Permalink
Adding runValidators (#87)
Browse files Browse the repository at this point in the history
* Adding runValidators

As mentioned here: #73

Still needs a test.

* added missing comma

* Add tests and params.mongoose support for update and patch
  • Loading branch information
marshallswain authored and daffl committed Jun 17, 2016
1 parent c5b844f commit cedd549
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 26 deletions.
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"watch": "babel --watch -d lib/ src/",
"jshint": "jshint src/. test/. --config",
"mocha": "mocha test/ --compilers js:babel-core/register",
"test": "npm run compile && npm run jshint && npm run mocha && nsp check"
"test": "npm run compile && npm run jshint && npm run mocha"
},
"directories": {
"lib": "lib"
Expand All @@ -73,7 +73,6 @@
"jshint": "^2.8.0",
"mocha": "^2.5.0",
"mongoose": "^4.1.0",
"nsp": "^2.2.0",
"rimraf": "^2.5.2",
"sinon": "^1.17.3",
"sinon-chai": "^2.8.0"
Expand Down
46 changes: 22 additions & 24 deletions src/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class Service {
const executeQuery = total => {
return query.exec().then(data => {
return {
total: total,
total,
limit: filters.$limit,
skip: filters.$skip || 0,
data
Expand Down Expand Up @@ -132,12 +132,17 @@ class Service {
return this.Model.create(data).catch(errorHandler);
}

update(id, data) {
update(id, data, params) {
if(id === null) {
return Promise.reject('Not replacing multiple records. Did you mean `patch`?');
}

const options = {new: true, overwrite: this.overwrite};
const options = Object.assign({
new: true,
overwrite: this.overwrite,
runValidators: true,
setDefaultsOnInsert: true
}, params.mongoose);

if (this.id === '_id') {
// We can not update default mongo ids
Expand All @@ -148,16 +153,10 @@ class Service {
data[this.id] = id;
}

// NOTE (EK): We don't use the findByIdAndUpdate method because these are functionally

This comment has been minimized.

Copy link
@ekryski

ekryski Jul 7, 2016

Member

Any reason for removing the comment?

This comment has been minimized.

Copy link
@marshallswain

marshallswain Jul 7, 2016

Author Member

Nope. I'm not even sure why I did it.

// equivalent and this allows a developer to set their id field as something other than _id.
return this
.Model
return this.Model
.findOneAndUpdate({ [this.id]: id }, data, options)
.lean(this.lean)
.exec()
.then((result) => {
return result;
})
.catch(errorHandler);
}

Expand All @@ -166,7 +165,9 @@ class Service {
data = Object.assign({}, data);

// If we are updating multiple records
let multi = id === null;
let options = Object.assign({
multi: id === null
}, params.mongoose);

if (id !== null) {
params.query[this.id] = id;
Expand All @@ -181,9 +182,8 @@ class Service {
data[this.id] = id;
}

return this
.Model
.update(params.query, { $set: data }, { multi })
return this.Model
.update(params.query, { $set: data }, options)
.lean(this.lean)
.exec()
.then(() => this._getOrFind(id, params))
Expand All @@ -199,16 +199,14 @@ class Service {

// NOTE (EK): First fetch the record(s) so that we can return
// it/them when we delete it/them.
return this
._getOrFind(id, params)
.then(data => {
return this.Model
.remove(query)
.lean(this.lean)
.exec()
.then(() => data)
.catch(errorHandler);
})
return this._getOrFind(id, params)
.then(data => this.Model
.remove(query)
.lean(this.lean)
.exec()
.then(() => data)
.catch(errorHandler)
)
.catch(errorHandler);
}
}
Expand Down
11 changes: 11 additions & 0 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,17 @@ describe('Feathers Mongoose Service', () => {
done();
}).catch(done);
});

it('runs validators on update', function(done) {
people.create({ name: 'David', age: 333 })
.then(person => people.update(person._id, { name: 'Dada', age: 'wrong' }))
.then(() => done(new Error('Update should not be successful')))
.catch(error => {
expect(error.name).to.equal('BadRequest');
expect(error.message).to.equal('Cast to number failed for value "wrong" at path "age"');
done();
});
});
});

describe('Lean Services', () => {
Expand Down

4 comments on commit cedd549

@quick691fr
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw @marshallswain modified the service.js file to run validators on update which is great, why don't you put context: 'query' in options to make "this" refer the document in validators instead of being null ?

@marshallswain
Copy link
Member Author

Choose a reason for hiding this comment

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

@quick691fr That's a great idea. Do you want to make a PR?

@quick691fr
Copy link
Contributor

Choose a reason for hiding this comment

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

@ekryski
Copy link
Member

@ekryski ekryski commented on cedd549 Jul 7, 2016

Choose a reason for hiding this comment

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

And merged and shipped!

Please sign in to comment.