Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions lib/models/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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
Expand All @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

finally spotted the native here - 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 the Promise.fromNode what promisifyies the this.native as you're binding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.then(function(collection) {
return collection.update(
{ _id: waterline.nodes.mongo.objectId(id) },
{
$addToSet: { tags: { $each: tags } },
$set: { updatedAt: new Date() }
});
});
},
remTags: function(id, tag) {
Copy link
Member

Choose a reason for hiding this comment

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

remTags is named as 'removeTags' is more straightforward。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your opinion is appreciated, but rem/add is more straightforward to me.
Bike-shedding: https://en.wikipedia.org/wiki/Law_of_triviality.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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(
Copy link
Member

Choose a reason for hiding this comment

The 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.
But I think it's better to move mongodb native commands to some common place so that it could be leveraged by other codes, just like removing a 'key:value' here is a common case. and previous waterline CRUD message is published to amqp
https://github.com/RackHD/on-core/blob/master/lib/common/model.js#L62-L89 , the native operations in a common place could also so similar things that publish native messages to amqp

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@anhou yep, @tldavies is also going to do the same thing for the findAndModify atomic operation you suggested a while back for lookup entries on-dhcp-proxy I believe.

{ _id: waterline.nodes.mongo.objectId(id) },
{
$pull: { tags: tag },
$set: { updatedAt: new Date() }
});
});
},
findByTag: function(tag) {
return waterline.nodes.find({tags: tag});
}
});
}
46 changes: 46 additions & 0 deletions lib/models/tags.js
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
}
}
});
}
67 changes: 67 additions & 0 deletions spec/lib/models/node-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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); });
Copy link
Member

Choose a reason for hiding this comment

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

teachme question - why stub out model.native like this? Not cluing in how it's used by the tests.... (not clear on what model.native is - are we just using it to make sure the collections got called and invoked?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

No qualms with the tactics, just trying to grok where .native normally resides. I'm not clear on where it's getting promisified. Is native something that's already in the waterline model objects and is a convenient hook for capturing the flow that would otherwise go down into the MongoDB drivers?

});

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();
});
});

});
});
});
110 changes: 110 additions & 0 deletions spec/lib/models/tags-spec.js
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);
});
});
});