Skip to content

Add model def objectid support #778

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

Closed
superkhau wants to merge 2 commits into from
Closed

Add model def objectid support #778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d586af7
Cleanup
Nov 29, 2015
5d987f6
Add model definition ObjectId support
Nov 29, 2015
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
64 changes: 38 additions & 26 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,55 @@
TESTER = ./node_modules/.bin/mocha
OPTS = --growl
TESTS = test/*.test.js
E2E_TEST_DIR = test/e2e/*.js

default: help
# Default target

.PHONY: clean
clean:
rm -rf $(CURDIR)/node_modules

.PHONY: help
help:
.PHONY: help h
help h:
@echo 'Usage: make [target]'
@echo 'Targets:'
@echo ' clean Delete `node_modules`'
@echo ' help Print help (this message)'
@echo ' refresh Delete `node_modules` and run `npm install`'
@echo ' test Run tests in silent mode'
@echo ' test-verbose Run tests in verbose mode'
@echo ' testing Run tests continuously'

.PHONY: refresh
refresh: clean
npm install

.PHONY: test
test:
@echo ' e e2e Run end-to-end tests'
@echo ' h help Print help (this message)'
@echo ' t test[s] Run unit tests and e2e tests'
@echo ' u unit Run unit tests in silent mode'
@echo ' uv unit-verbose Run unit tests in verbose mode'
@echo ' uw unit-watch Run unit tests in watch (--watch) mode'

# Targets

.PHONY: e2e e
e2e e:
$(TESTER) --reporter spec $(E2E_TEST_DIR)

.PHONY: test tests t
test tests t: unit e2e

.PHONY: unit u
unit u:
NO_DEPRECATION=loopback-datasource-juggler $(TESTER) $(OPTS) $(TESTS)

.PHONY: test-verbose
test-verbose:
.PHONY: unit-verbose uv
unit-verbose uv:
$(TESTER) $(OPTS) --reporter spec $(TESTS)

.PHONY: testing
testing:
.PHONY: unit-watch uw
unit-watch uw:
$(TESTER) $(OPTS) --watch $(TESTS)

# Deprecated targets
# Deprecated targest (left for backwards compat)

.PHONY: about-testing
about-testing:
@echo 'DEPRECATED: Use `make help` instead'
make help
$(MAKE) help

.PHONY: test-verbose
test-verbose:
@echo 'DEPRECATED: Use `make unit-verbose` instead'
$(MAKE) unit-verbose

.PHONY: testing
testing:
@echo 'DEPRECATED: Use `make unit-watch` instead'
$(MAKE) unit-watch
12 changes: 12 additions & 0 deletions lib/objectid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
var ObjectId;

try {
ObjectId = require('mongodb').ObjectId;
} catch(e) {
if (e.code === 'MODULE_NOT_FOUND')
throw e;

@bajtos bajtos Dec 15, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@superkhau AFAICT, this silently swallows any mongodb parse errors, which does not look like a good thing to me. Could you please explain what is the rationale for this, why we need to give require('mongodb').ObjectId errors a special treatment?

@superkhau superkhau Dec 15, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was suggested by @ritch in a previous comment in this issues history. I think it was for uses cases where mongodb is not available (ie. the browser).

@bajtos bajtos Dec 22, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough. Please add a debug log to let the user know ObjectId will be undefined because mongodb was not found.

debug('Cannot load mongodb, ObjectId will be undefined. %s', e.stack);

}

@superkhau superkhau Dec 2, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ritch LGTY?

@bajtos bajtos Dec 2, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be honest, I don't like adding mongodb dependency to juggler at all. Remember, there is no guarantee that mongodb module referenced by juggler will be the same module as referenced by loopback-connector-mongodb. In fact, this will be the most frequent case.

app
  node_modules
    loopback-datasource-juggler
      node_modules
        mongodb
    loopback-connector-mongodb
      node_modules
        mongodb

As a result, you are registering a type (constructor function) that is actually never used by the connector, because the connector has it's own local type (constructor function).

@superkhau superkhau Dec 2, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TBH, I don't like it either, but it was the simplest way to get the full power of an ObjectId (all it's validations, unique string generation, other functions etc) in one shot. The other option is to just not use mongodb at all and come up with our own unique implementation for each function. However, this is not a 5 point job. I figure in the long term proposal task I have for this sprint, we can decide whether we want to come up with our own implementations for each function or use source code from other projects, etc.

@superkhau superkhau Dec 2, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would have an API similar to https://docs.mongodb.org/manual/reference/object-id/ basically.

@superkhau superkhau Dec 2, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Another thing to consider is the option to allow user-configurable ObjectId types. Like in model-config.json, we allow {"objectid": "../path/to/your/custom/objectid"}, or something along those lines.

@bajtos bajtos Dec 3, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes me wonder if there is a way how to allow loopback-connector-mongodb to call modelTypes.registerType(ObjectId) during connector/datasource initialisation. That way we can keep the implementation in mongodb and still have ObjectID available in the app.

Of course, that would not work for isomorphic app in the browser, so it would be still a short-term fix.

Anyways, I guess you spend more time thinking about this issue than I did, and if this is the best short-term solution we have, then I can live with it.

@superkhau superkhau Dec 3, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes me wonder if there is a way how to allow loopback-connector-mongodb to call modelTypes.registerType(ObjectId) during connector/datasource initialisation. That way we can keep the implementation in mongodb and still have ObjectID available in the app.

I have thought about this too, but figured it would be harder to implement because we don't expose that part of the lib in DAO. The long term solution I had in mind and discussed with @ritch is an app-level registry for datatypes. During boot time, we would register all basic built in datatypes, then register any custom types. Then at run time, we can dynamically register/deregister new datatypes. Something along the lines of:

app.datatypes.add(ObjectId);
app.datatypes.list();
app.datatypes.remove(ObjectId);

Or use an a K/V array for datatypes, etc. We can discuss further in the long-term proposal with regards to implementations.

module.exports = ObjectId;
module.exports.ObjectID = ObjectId;
module.exports.ObjectId = ObjectId;
4 changes: 3 additions & 1 deletion lib/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Types.Any.prototype.toObject = Types.Any.prototype.toJSON = function () {
module.exports = function (modelTypes) {

var GeoPoint = require('./geo').GeoPoint;
var ObjectId = require('./objectid');

for(var t in Types) {
modelTypes[t] = Types[t];
Expand All @@ -62,6 +63,7 @@ module.exports = function (modelTypes) {
modelTypes.registerType(Array);
modelTypes.registerType(GeoPoint);
modelTypes.registerType(Object);
modelTypes.registerType(ObjectId);

@bajtos bajtos Dec 11, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any way how to register ObjectID as an alias for ObjectId? Or is it automatically handled?

@superkhau superkhau Dec 14, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is already registered as an alias in the ObjectId class definition, so it should work properly. I added an e2e test to prove it.

};

module.exports.Types = Types;
module.exports.Types = Types;
9 changes: 6 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,16 @@
"depd": "./lib/browser.depd.js"
},
"scripts": {
"clean": "make clean",
"help": "make help",
"refresh": "make refresh",
"e2e": "make e2e",
"unit": "make unit",
"test": "make test"
},
"engines": [
"node >= 0.6"
],
"devDependencies": {
"bluebird": "^2.9.9",
"loopback": "^2.25.0",
"mocha": "^2.1.0",
"should": "^5.0.0"
},
Expand All @@ -43,5 +43,8 @@
"qs": "^3.1.0",
"traverse": "^0.6.6"
},
"optionalDependencies": {
"mongodb": "^2.0.49"
},
"license": "MIT"
}
25 changes: 24 additions & 1 deletion test/datatype.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,30 @@ var should = require('./init.js');

var db, Model;

describe('datatypes', function () {
describe('datatypes', function() {

@superkhau superkhau Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bajtos Added data type tests here.

context('ObjectId', function(done) {
var OidModel;

before(function setup() {
var db = getSchema();
OidModel = db.define('OidModel', {
id: {
type: 'ObjectId',
id: true
}
});
db.automigrate('OidModel', done);
});

it('should work', function(done) {
OidModel.create({id: 'hello1hello1'}, function(err, inst) {
if (err) return done(err);
inst.id.should.be.type('object');
inst.id.toString().should.be.type('string').and.have.length(24);
done();
});
});
});

before(function (done) {
db = getSchema();
Expand Down
59 changes: 59 additions & 0 deletions test/e2e/model-definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
var DataSource = require('../..').DataSource;
var ObjectId = require('../../lib/objectid');
var should = require('should');

describe('model definition', function() {
context('ObjectId definition', function() {
var Todo;

before(function setup() {
var ds = new DataSource();
Todo = ds.define('Todo', {
id: {type: 'ObjectId', id: true}
});
});

it('should allow 12 byte strings', function() {
var todo = new Todo({id: 'hello1hello1'});

todo.id.should.be.an.instanceof(ObjectId);
// 12 byte strings are coerced into 24 character hex strings
todo.id.toString().should.have.length(24);
});

it('should allow 24 character hex strings', function() {
var hexStr = '507f191e810c19729de860ea';

var todo = new Todo({id: hexStr});

todo.id.should.be.an.instanceof(ObjectId);
todo.id.toString().should.equal(hexStr);
});

it('should allow ObjectIds', function() {
var objId = new ObjectId();

var todo = new Todo({id: objId});

todo.id.should.be.an.instanceof(ObjectId);
todo.id.toString().should.equal(objId.toString());
});
});

context('ObjectID (alias) definition', function() {
var Todo;

before(function setup() {
var ds = new DataSource();
Todo = ds.define('Todo', {
id: {type: 'ObjectID', id: true}

@superkhau superkhau Dec 14, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Test added here.

});
});

it('should work', function() {
var todo = new Todo({id: 'hello1hello1'});

todo.id.should.be.an.instanceof(ObjectId);
});
});
});
1 change: 0 additions & 1 deletion test/memory.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,6 @@ describe('Memory connector', function() {
}
}, function (err, users) {
should.not.exist(err);
console.log(users);
users.length.should.be.equal(5);
done();
});
Expand Down
62 changes: 62 additions & 0 deletions test/objectid.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
var MongoObjId = require('mongodb').ObjectId;
var ObjectId = require('../lib/objectid').ObjectId;

describe('ObjectId', function() {
context('Constructor', function() {
it('should allow 12 byte strings', function() {
var str = 'hello1hello1';

var objId = new ObjectId(str);

objId.should.be.an.instanceof(ObjectId);
objId.toString().should.be.type('string');
// 12 byte strings are coerced into 24 character hex strings
objId.toString().should.have.length(24);
});

it('should allow 24 character hex strings', function() {
var hexStr = '507f191e810c19729de860ea';

var objId = new ObjectId(hexStr);

objId.should.be.an.instanceof(ObjectId);
objId.toHexString().should.equal(hexStr);
});

it('should allow ObjectId', function() {
var existingObjId = new ObjectId();

var objId = new ObjectId(existingObjId);

objId.should.be.an.instanceof(ObjectId);
objId.valueOf().should.equal(existingObjId.valueOf());
});

it('should allow MongoDB ObjectIds', function() {
var objId = new ObjectId(new MongoObjId());

objId.should.be.an.instanceof(ObjectId);
});

it('should not allow invalid data types', function() {
var fn = function() {};
[
true,
{},
fn
].forEach(function(invalidDataType) {
(function() {
new ObjectId(invalidDataType);
}).should.throw();
});
});
});

context('toString', function() {
it('should be serializable', function() {
var objId = new ObjectId();

objId.toString().should.be.type('string');
});
});
});