Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix init hook invokation when finding docs #1221

Closed
wants to merge 3 commits into from

5 participants

@feugy

When querying docs (with a query or with Model finders), I figured out that 'init' middleware are properly invoked on each rehydrated models.

But weirdly, the corresponding Model 'init' method is not invoked on the last rehydrated model.

I made a unit test in 'model.querying.test.js' to show this, and propose a fix.
Just comment my fix to reproduce the bug.

Query.prototype.execFind = function (callback) {
    // ...

    for (var i = 0, l = docs.length; i < l; i++) {
      arr[i] = new model(undefined, fields, true);
      arr[i].init(docs[i], self, function (err) {
        if (err) return promise.error(err);
        --count || promise.complete(arr); /* setTimeout(function() {
          promise.complete(arr);
        }, 0); */
      });
    }

Thanks a lot !

@temsa

+1

@vvision

+1

@aheckmann aheckmann commented on the diff
test/model.querying.test.js
((8 lines not shown))
+ var db = start()
+ , BlogPostB = db.model('BlogPostB', collection)
+ , title = 'Wooooot ' + random();
+
+ var post = new BlogPostB();
+ post.set('title', title);
+
+ post.save(function (err) {
+ assert.ifError(err);
+
+ // register init extension
+ var initialized = false;
+ var originalInit = mongoose.Model.prototype.init;
+ mongoose.Model.prototype.init = function () {
+ originalInit.apply(this, arguments);
+ initialized = true;
@aheckmann Owner

switch the order of your operations around. you're passing control on to the next step (which is your callback) before marking it as initialized.

initialized = true;
originalInit.apply(this, arguments);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aheckmann aheckmann commented on the diff
test/model.querying.test.js
@@ -1079,6 +1113,41 @@ describe('model: querying:', function(){
});
})
+ it('execute init hooks on results', function (done){
+ var db = start()
+ , Mod = db.model('Mod');
+
+ Mod.create({num: 1}, {num: 2}, {num: 3}, function (err, one, two, three) {
+ assert.ifError(err);
+
+ // register init extension
+ var initialized = [];
+ var originalInit = mongoose.Model.prototype.init;
+ mongoose.Model.prototype.init = function () {
+ originalInit.apply(this, arguments);
+ initialized.push(this);
@aheckmann Owner

same here. the order of ops is invalid. I cannot reproduce the error with valid test cases.

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

Indeed I know, but my real test case is complex and I wanted to simulate it.
I'll try to explain it.

The problem is that I need to make operation on models while they are rehydrated, before any other part can use them but after beeing initialized (after the schema was compiled).

If I execute my operations inside the 'init' hook, this aims at the raw content of attributes, and it does not fit.
Same reason inside the 'init' method' : my code need to be executed after the original 'init' method has proceeded.

The real example:

var originalInit = mongoose.Document.prototype.init
mongoose.Document.prototype.init = function() {
  var ret = originalInit.apply(this, arguments);

  if (this.type.properties != null) {
    var self = this;
    // define setters and getters for properties once the instance have been initialized
    for (var name in this.type.properties) {
      ((name) function() {
        if (Object.getOwnPropertyDescriptor(self, name) == null) {
          Object.defineProperty(self, name,{
            enumerable: true,
            configurable: true,
            get: function() { return self.get(name);},
            set: function(v) { self.set(name, v);}
        }
      })(name)
  }

  return ret;
};

'type' is a model property 'statically' defined inside the schema. But I contains 'dynamic' properties that are set at runtime, and that are different for models of the same class. that's why they are not inside the schema.
If I executed it before the 'originalInit', an error while be raised : 'cannot invoke get/set on [Object object]'.

Sorry for this too long explanation.
If

@aheckmann
Owner

have you tried a post: init listener? it is emitted after the doc is initialized but before the callback is executed.

schema.post('init', function () {
  if (this.type.properties != null) {
    var self = this;
    // define setters and getters for properties once the instance have been initialized
    ...
  }
})
@feugy

Thanks Aaron, it works with the post 'init' middleware.

As they were not documented since version 3, I thought they were deprecated, and used instead pre middleware with code after next().

So I'll close this pull request.

Just one question: this initialization code must also be run just after the instanciation with new operator.

Item newItem = new Item({type: {properties: {strength: Number, content:String}}});
newItem.strength = 18;
newItem.save();

As we cannot overload constructors, I overloaded the _registerHook method (last one called by the Document constructor).

Is there a better way (a hidden hook ?) to run code at Document construction ?

@feugy feugy closed this
@aheckmann
Owner

thanks for the heads up on the post init docs. just added them to master.

there isn't a constructor hook at present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 21, 2012
  1. Add test on model init hook when usgin find

    Damien Feugas (a127380) authored
  2. @feugy
Commits on Nov 22, 2012
  1. @feugy
This page is out of date. Refresh to see the latest.
Showing with 76 additions and 3 deletions.
  1. +6 −2 lib/query.js
  2. +70 −1 test/model.querying.test.js
View
8 lib/query.js
@@ -1484,7 +1484,9 @@ Query.prototype.execFind = function (callback) {
arr[i] = new model(undefined, fields, true);
arr[i].init(docs[i], self, function (err) {
if (err) return promise.error(err);
- --count || promise.complete(arr);
+ --count || setTimeout(function() {
+ promise.complete(arr);
+ }, 0);
});
}
}
@@ -1549,7 +1551,9 @@ Query.prototype.findOne = function (callback) {
var casted = new model(undefined, fields, true);
casted.init(doc, self, function (err) {
if (err) return promise.error(err);
- promise.complete(casted);
+ setTimeout(function(){
+ promise.complete(casted);
+ }, 0);
});
}));
View
71 test/model.querying.test.js
@@ -627,7 +627,41 @@ describe('model: querying:', function(){
done();
});
});
- })
+ });
+
+ it('execute init hooks on result', function (done){
+ var db = start()
+ , BlogPostB = db.model('BlogPostB', collection)
+ , title = 'Wooooot ' + random();
+
+ var post = new BlogPostB();
+ post.set('title', title);
+
+ post.save(function (err) {
+ assert.ifError(err);
+
+ // register init extension
+ var initialized = false;
+ var originalInit = mongoose.Model.prototype.init;
+ mongoose.Model.prototype.init = function () {
+ originalInit.apply(this, arguments);
+ initialized = true;
@aheckmann Owner

switch the order of your operations around. you're passing control on to the next step (which is your callback) before marking it as initialized.

initialized = true;
originalInit.apply(this, arguments);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ };
+
+ BlogPostB.findOne({ title: title }, function (err, doc) {
+ assert.ifError(err);
+ assert.equal(title, doc.get('title'));
+ assert.equal(false, doc.isNew);
+ assert.equal(true, initialized);
+
+ // restore original init
+ mongoose.Model.prototype.init = originalInit;
+
+ db.close();
+ done();
+ });
+ });
+ });
});
describe('findById', function () {
@@ -1079,6 +1113,41 @@ describe('model: querying:', function(){
});
})
+ it('execute init hooks on results', function (done){
+ var db = start()
+ , Mod = db.model('Mod');
+
+ Mod.create({num: 1}, {num: 2}, {num: 3}, function (err, one, two, three) {
+ assert.ifError(err);
+
+ // register init extension
+ var initialized = [];
+ var originalInit = mongoose.Model.prototype.init;
+ mongoose.Model.prototype.init = function () {
+ originalInit.apply(this, arguments);
+ initialized.push(this);
@aheckmann Owner

same here. the order of ops is invalid. I cannot reproduce the error with valid test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ };
+ // find some models
+ Mod.find().sort({field: 'asc', num: 1}).exec(function (err, found) {
+ assert.ifError(err);
+
+ assert.equal(3, found.length);
+ assert.equal(found[0]._id.toString(), one._id);
+ assert.equal(found[1]._id.toString(), two._id);
+ assert.equal(found[2]._id.toString(), three._id);
+ assert.equal(3, initialized.length);
+ assert.equal(found[0]._id.toString(), one._id);
+ assert.equal(found[1]._id.toString(), two._id);
+ assert.equal(found[2]._id.toString(), three._id);
+
+ // restore original init
+ mongoose.Model.prototype.init = originalInit;
+ done();
+ });
+ });
+
+ });
+
it('where $ne', function(done){
var db = start()
, Mod = db.model('Mod', 'mods_' + random());
Something went wrong with that request. Please try again.