-
Notifications
You must be signed in to change notification settings - Fork 63
Node Tagging: #88
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
Node Tagging: #88
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,11 @@ NodeModelFactory.$provide = 'Models.Node'; | |
| NodeModelFactory.$inject = [ | ||
| 'Model', | ||
| 'Services.Waterline', | ||
| '_' | ||
| '_', | ||
| 'Promise' | ||
| ]; | ||
|
|
||
| function NodeModelFactory (Model, waterline, _) { | ||
| function NodeModelFactory (Model, waterline, _, Promise) { | ||
| return Model.extend({ | ||
| connection: 'mongo', | ||
| identity: 'nodes', | ||
|
|
@@ -54,6 +55,10 @@ function NodeModelFactory (Model, waterline, _) { | |
| type: 'array', | ||
| defaultsTo: [] | ||
| }, | ||
| tags: { | ||
| type: 'array', | ||
| defaultsTo: [] | ||
| }, | ||
| // We only count a node as having been discovered if | ||
| // a node document exists AND it has any catalogs | ||
| // associated with it | ||
|
|
@@ -64,6 +69,33 @@ function NodeModelFactory (Model, waterline, _) { | |
| return !_.isEmpty(catalog); | ||
| }); | ||
| } | ||
| }, | ||
| addTags: function(id, tags) { | ||
| // TODO: Refactor to native mongo command once it is merged | ||
| return Promise.fromNode(this.native.bind(this)) | ||
| .then(function(collection) { | ||
| return collection.update( | ||
| { _id: waterline.nodes.mongo.objectId(id) }, | ||
| { | ||
| $addToSet: { tags: { $each: tags } }, | ||
| $set: { updatedAt: new Date() } | ||
| }); | ||
| }); | ||
| }, | ||
| remTags: function(id, tag) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remTags is named as 'removeTags' is more straightforward。
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your opinion is appreciated, but rem/add is more straightforward to me.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just the mind about code readability when first glance this line, at least for me, not for trivials, it's also fine for me if others also think it's fine
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't really have a problem with remTags, it's just like "delX" which you see in a lot of places. Also @zyoung51 if you get the chance will you add a // TODO above this to refactor it into this.updateMongo once my workflow PR goes in? |
||
| // TODO: Refactor to native mongo command once it is merged | ||
| return Promise.fromNode(this.native.bind(this)) | ||
| .then(function(collection) { | ||
| return collection.update( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure whether @benbp have concerns about introducing mongodb native commands to RackHD for compatibility consideration in the future? it's ok for me, because waterline CRUD cannot cover the cases such as remove a key:value in one object here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @anhou: Ben did move the native commands to a common place in his workflow changes, but they are not in yet and I do not want to make my PR dependent on his.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just noticed @benbp 's PR's native commands codes. appreciate that, it makes sense that not depend that PR
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| { _id: waterline.nodes.mongo.objectId(id) }, | ||
| { | ||
| $pull: { tags: tag }, | ||
| $set: { updatedAt: new Date() } | ||
| }); | ||
| }); | ||
| }, | ||
| findByTag: function(tag) { | ||
| return waterline.nodes.find({tags: tag}); | ||
| } | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // Copyright 2016, EMC, Inc. | ||
|
|
||
| 'use strict'; | ||
|
|
||
| module.exports = TagModelFactory; | ||
|
|
||
| TagModelFactory.$provide = 'Models.Tag'; | ||
| TagModelFactory.$inject = [ | ||
| 'Model', | ||
| '_', | ||
| 'Assert', | ||
| 'Validatable', | ||
| 'anchor' | ||
| ]; | ||
|
|
||
| function TagModelFactory (Model, _, assert, Validatable, Anchor) { | ||
| var allRules = _.keys(new Anchor().rules); | ||
|
|
||
| return Model.extend({ | ||
| types: { | ||
| tagRules: function(rules) { | ||
| assert.arrayOfObject(rules, 'rules'); | ||
| _.forEach(rules, function (rule) { | ||
| assert.string(rule.path, 'rule.path'); | ||
| _(rule).omit('path').keys().forEach(function (key) { | ||
| assert.isIn(key, allRules, 'rule.' + key); | ||
| }).value(); | ||
| }); | ||
| return true; | ||
| }, | ||
| }, | ||
| connection: 'mongo', | ||
| identity: 'tags', | ||
| attributes: { | ||
| name: { | ||
| type: 'string', | ||
| required: true | ||
| }, | ||
| rules: { | ||
| type: 'json', | ||
| tagRules: true, | ||
| required: true | ||
| } | ||
| } | ||
| }); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,5 +152,72 @@ describe('Models.Node', function () { | |
| }); | ||
| }); | ||
|
|
||
| describe('tags', function() { | ||
| before(function() { | ||
| this.subject = this.attributes.tags; | ||
| }); | ||
|
|
||
| it('should be an array', function() { | ||
| expect(this.subject.type).to.equal('array'); | ||
| }); | ||
|
|
||
| it('should default to empty', function() { | ||
| expect(this.subject.defaultsTo).to.deep.equal([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Node Tag Functions', function() { | ||
| var collection = { | ||
| update: sinon.stub().resolves() | ||
| }; | ||
|
|
||
| before(function() { | ||
| sinon.stub(this.model, "native", function(cb) { cb(null, collection); }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is stubbed out with this pattern so the underlying code can promisify the function. It is being stubbed out to remove the mongodb dependency from the unittest. Functional/integration testing would be used to validate MongoDB behavior/interactions, not unittests.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No qualms with the tactics, just trying to grok where |
||
| }); | ||
|
|
||
| after(function() { | ||
| this.model.native.restore(); | ||
| }); | ||
|
|
||
| it('should have all valid functions', function() { | ||
| var self = this; | ||
| var valid = ['addTags', 'remTags', 'findByTag']; | ||
| _.forEach(valid, function(name) { | ||
| expect(self.model[name]).to.exist; | ||
| expect(self.model).to.respondTo(name); | ||
| }); | ||
| }); | ||
|
|
||
| it('addTags should call update', function() { | ||
| var self = this; | ||
| return this.model.addTags.call(this.model, 'id', ['tag']) | ||
| .then(function() { | ||
| expect(collection.update).to.have.been.called; | ||
| expect(self.model.native).to.have.been.called; | ||
| }); | ||
| }); | ||
|
|
||
| it('remTags should call update', function() { | ||
| var self = this; | ||
| return this.model.remTags.call(this.model, 'id', 'tag') | ||
| .then(function() { | ||
| expect(collection.update).to.have.been.called; | ||
| expect(self.model.native).to.have.been.called; | ||
| }); | ||
| }); | ||
|
|
||
| it('findByTag should call find', function() { | ||
| var self = this; | ||
| sinon.stub(this.model, "find").resolves(); | ||
| return this.model.findByTag.call(this.model, 'tag') | ||
| .then(function() { | ||
| expect(self.model.find).to.have.been.calledWith({tags: 'tag'}); | ||
| }) | ||
| .finally(function() { | ||
| self.model.find.restore(); | ||
| }); | ||
| }); | ||
|
|
||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| // Copyright 2015, EMC, Inc. | ||
|
|
||
|
|
||
| 'use strict'; | ||
|
|
||
| var base = require('./base-spec'); | ||
| var sandbox = sinon.sandbox.create(); | ||
|
|
||
| describe('Models.Tag', function () { | ||
| helper.before(function (context) { | ||
| context.MessengerServices = function() { | ||
| this.start= sandbox.stub().resolves(); | ||
| this.stop = sandbox.stub().resolves(); | ||
| this.publish = sandbox.stub().resolves(); | ||
| }; | ||
| return [ | ||
| helper.di.simpleWrapper(context.MessengerServices, 'Messenger') | ||
| ]; | ||
| }); | ||
|
|
||
| base.before(function (context) { | ||
| context.model = helper.injector.get('Services.Waterline').tags; | ||
| context.attributes = context.model._attributes; | ||
| }); | ||
|
|
||
| helper.after(); | ||
|
|
||
| describe('Base', function () { | ||
| base.examples(); | ||
| }); | ||
|
|
||
| describe('Attributes', function () { | ||
| describe('name', function () { | ||
| before(function () { | ||
| this.subject = this.attributes.name; | ||
| }); | ||
|
|
||
| it('should be a string', function () { | ||
| expect(this.subject.type).to.equal('string'); | ||
| }); | ||
|
|
||
| it('should be required', function () { | ||
| expect(this.subject.required).to.equal(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('rules', function () { | ||
| before(function () { | ||
| this.subject = this.attributes.rules; | ||
| }); | ||
|
|
||
| it('should be required', function () { | ||
| expect(this.subject.required).to.equal(true); | ||
| }); | ||
|
|
||
| it('should be json', function () { | ||
| expect(this.subject.type).to.equal('json'); | ||
| }); | ||
| }); | ||
|
|
||
| }); | ||
|
|
||
| describe('Tag Rules', function () { | ||
| beforeEach(function () { | ||
| return helper.reset(); | ||
| }); | ||
|
|
||
| it('should validate tag with rules', function () { | ||
| return this.model.create({ | ||
| name: 'test1', | ||
| rules: [ | ||
| { | ||
| path: 'dmi.memory.total', | ||
| equals: '32946864kB' | ||
| } | ||
| ] | ||
| }).should.be.fulfilled; | ||
| }); | ||
|
|
||
| it('should not validate tag rules with invalid values', function () { | ||
| return this.model.create({ | ||
| name: 'test2', | ||
| rules: [1, 2, 3] | ||
| }).should.be.rejectedWith(Error); | ||
| }); | ||
|
|
||
| it('should not validate tag rules with a missing path', function () { | ||
| return this.model.create({ | ||
| name: 'test3', | ||
| rules: [ | ||
| { | ||
| path: null, | ||
| } | ||
| ] | ||
| }).should.be.rejectedWith(Error); | ||
| }); | ||
|
|
||
| it('should not validate tag rules with an invalid validation rule', function () { | ||
| return this.model.create({ | ||
| name: 'test4', | ||
| rules: [ | ||
| { | ||
| path: 'dmi.memory.free', | ||
| badMatcher: 'asdf' | ||
| } | ||
| ] | ||
| }).should.be.rejectedWith(Error); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finally spotted the
nativehere - guessing it'll be easier to catch you on chat to ask about this, but any pointers you can help me with are appreciated. is thePromise.fromNodewhat promisifyies thethis.nativeas you're binding it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise.fromNode promisifies this: http://sailsjs.org/documentation/reference/waterline-orm/models/native. Credit to @benbp for the work: https://github.com/RackHD/on-core/pull/75/files#diff-89e9e9d58a3c4aa21cf6a96f2bde6e15R110