An incident caused by an incorrect TypeScrpit type definition of Mongoose

The TypeScript type definition of Mongoose are not fully correct!

I inspected (or debugged) a nodejs server project of Little Gengsha‘s, where he’s using koa as the server and mongodb as the data store. The project seems called “Writer’s Platform”, which seemed very primitive then. There were only two routes, a get, a post, the latter of which is used to submit an article to mongodb. During the test he discovered a problem — the body of the response should be changed in the callback of the save function, but the action response is “OK”, which is the default response of koa.

router.post('/path', (ctx, next) => {
    const article = new Article( /* something */ );
    article.save((err, article) => {
      if (err){
        ctx.body = err;
      } else {
        ctx.body = "success";
      }
    });
})

At the first sight I discovered thatnext() is not returned in /* body */ of router.post('/path', (ctx, next) => { /* body */ });This may cause the response to return early before the save action calls back. But simply return a next() did not help.

Then I thought it could be due to the chaos of the versions of the koa ecosystem. The versions of koa and koa-router are all mixed. They both defaulted to the old version while people would like to use the new, under-development version. Koa advanced afterwards, while koa-router still defaults to the older version. I once encountered a problem caused by such incompatibility and solved it for someone.

But this time it’s not the case. We switched from promise to async/await, but the problem persists.

router.post('/path', (ctx, next) => {
    const article = new Article( /* something */ );
    return article.save((err, saved) => {
      if (err){
        ctx.body = err;
      } else {
        console.log(saved);
        ctx.body = "success";
      }
      // or `return next();` here.
    }).then((thing) => {
        console.log(thing);
        return next();
    });
})

(However, we discovered that  thing is undefined, which turns out to be the key)

I was then upset with the signatures of the various functions (like what should be returned in the callback of router.get(), or what is the type of next), so I proposed using TypeScript for auxiliary checks.

We began to suspect that  article.save is not working as expected. The function, which is similar to the only sample code from koa regarding async operations, is expected to work as demonstrated by the doc：

router.get(
  '/users/:id',
  function (ctx, next) {
    return User.findOne(ctx.params.id).then(function(user) {
      ctx.user = user;
      return next();

    });
  },
  function (ctx) {
    console.log(ctx.user);
    // => { id: 17, name: "Alex" }
  }
);

So we specially inspected the TypeScript type definition of mongoose, in hope of finding any misuse. The  article is an instance of Article, while Article is actually a class (not only a instance, but also a class itself) dynamically constructed by mongoose. It is reflected in the type system:

interface Model<T extends Document> extends NodeJS.EventEmitter, ModelProperties {
    new(doc?: Object): T;
    /* other things */
}

and article is just the <T extends Document>， Document‘s savemethod is written as follows：

    /**
     * Saves this document.
     * @param options options optional options
     * @param options.safe overrides schema's safe option
     * @param options.validateBeforeSave set to false to save without validating.
     * @param fn optional callback
     */
    save(options?: SaveOptions, fn?: (err: any, product: this, numAffected: number) => void): Promise<this>;
    save(fn?: (err: any, product: this, numAffected: number) => void): Promise<this>;

(taken from the result of npm install @types/mongoose, which somehow differs from that on the github repo of DefinitelyTyped .)  Seeing the Promise<this>, I was convinced that our use of the function is valid. But the fact is the thing returned above is undefined. So are there any peculiarities?

Since TypeScript was not reliable then, we could only resort to the source code. Directly searching for  .prototype.save will yield us the relevant code. It’s actually here. But seeing it makes me more desperate:

Model.prototype.save = function(options, fn) {
  if (typeof options === 'function') {
    fn = options;
    options = undefined;
  }

  if (!options) {
    options = {};
  }

  if (fn) {
    fn = this.constructor.$wrapCallback(fn);
  }

  return this.$__save(options, fn);
};

It returns the result of this.$__save, but that function does not return actually! How could it return a promise from a function that returns nothing？

We thought of another ultimate measure: inspecting the call stack。We used console.trace for it. Although no direct clue shown, it still gave us some hint——save is not called directly, but wrapped with some hook.

Suddenly, I thought about directly console.log(article.save), which told us that article.save is actually the result of a wrappedPointCut function! Searching for wrappedPointCut in the code almost revealed the answer. The crucial part is here:

model.prototype[pointCut] = (function(_newName) {
      return function wrappedPointCut() {
        var Promise = PromiseProvider.get();

        var _this = this;
        /* something omitted for simplicity */
        var promise = new Promise.ES6(function(resolve, reject) {
          args.push(function(error) {
            if (error) {
              /* something omitted for simplicity */
              reject(error);
              return;
            }
            $results = Array.prototype.slice.call(arguments, 1);
            resolve.apply(promise, $results);
          });
          _this[_newName].apply(_this, args);
        });
        if (fn) {
          /* something omitted for simplicity */
          return promise.then(
            function() {
              process.nextTick(function() {
                fn.apply(null, [null].concat($results));
              });
            },
            function(error) {
              process.nextTick(function() {
                fn(error);
              });
            });
        }
        return promise;
      };
    })(newName);

Without fn， a promise defined on line 7 will be returned on line 33, while with afn in the parameter, the promise defined would be chained with a then on line 21, which returns nothing — In this case, the return value should be Promise<undefined> instead of Promise<this>—— We were fooled by such sloppy type definition! How I wish it could be annotated correctly from the beginning, just like this:

    save(options: SaveOptions, fn: (err: any, product: this, numAffected: number) => void): Promise<undefined>;
    save(fn: (err: any, product: this, numAffected: number) => void): Promise<undefined>;
    save(options?: SaveOptions): Promise<this>;
    save(): Promise<this>;

May, 4, 2017