Mongoose: orFail() returns undefined when a match IS found

[Akrion]

Mongodb: 3.6.5, Mongoose: 5.3.1, Node Driver: 3.1.6

As per the docs onFail() helper function should throw when nothing is found (or done in the case of update/delete) but if it matches on a document should return it right? It seems that just adding it to a working query with a valid _id it would make the query output undefined:

Schema:

const mongoose = require('mongoose');
const Schema = mongoose.Schema;

var AuthorSchema = new Schema({
  email: {
    type: String,
    lowercase: true,
    required: true
  })
module.exports = mongoose.model('Author', AuthorSchema)

Running this then:

var mongoose = require('mongoose');
var A = require('./models/author');

var r = A.findOne({ _id: mongoose.Types.ObjectId("5bb5806083512515c9dd88af")})
   .then(x => console.log('result', x))

Would return the expected model as x. However:

var r = A.findOne({ _id: mongoose.Types.ObjectId("5bb5806083512515c9dd88af")})
   .orFail()
   .then(x => console.log('result', x))

Would output undefined. Same happens if we use .exec().

[gsandorx]

Just want to point out that if the query filter passed to the findOne() function is not matched, orFail() does work as expected by throwing the specified exception (if any)

[lineus]

@Akrion Nothing in the docs suggests that orFail() will return the found document, just that it throws when a document matching the query doesn't exist ( I assumed it would too though. ).

@vkarpov15 can we make orFail() return the doc on a successful match?

[gsandorx]

@lineus http://thecodebarbarian.com/whats-new-in-mongoose-53-orfail-and-global-toobject.html

Reading that blogpost by @vkarpov15 , one would assume that if a document is found, findById() would return the matched document. Only when the query filter doesn't match any document, orFail() would kick in...

[lineus]

@gsandorx thanks for pointing that out! That answers my question to @vkarpov15. This is reproducible with the following script:

7099.js

#!/usr/bin/env node
'use strict';

const assert = require('assert');
const mongoose = require('mongoose');
const { Schema, connection} = mongoose;
const DB = '7099';
const URI = `mongodb://localhost:27017/${DB}`;
const OPTS = { family: 4, useNewUrlParser: true };

const schema = new Schema({
  name: String
});

const Test = mongoose.model('test', schema);

const test = new Test({ name: 'test1' });

async function run() {
  await mongoose.connect(URI, OPTS);
  await connection.dropDatabase();
  const doc = await Test.create(test);
  assert.ok(doc);
  const found = await Test.findOne({}).orFail();
  assert.ok(found, `got ${found} instead of a document.`);
  console.log('All Assertions Pass.');
  await connection.close();
}

run().catch(error);

function error(e) {
  console.error(e);
  return connection.close();
}

Output:

issues: ./7099.js
{ AssertionError [ERR_ASSERTION]: got undefined instead of a document.
    at run (/Users/lineus/dev/node/mongoose/issues/7099.js:25:10)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  generatedMessage: false,
  name: 'AssertionError [ERR_ASSERTION]',
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: true,
  operator: '==' }
issues:

[gsandorx]

File: https://github.com/Automattic/mongoose/blob/master/lib/query.js
Line: 3716

I added a return res statement (just testing this out, given that the res object contains the found document) and the problem is gone. The problem is that if the (res == null) statement is false (ie. a document was found), there's no code branch returning the res object, thus we are getting undefined.

We have to do the same thing on the other case 's in that code section.

I can submit a pull request...

[lineus]

I was just working on it too 👍

I think all of the cases in that swtich (including the default?) should return res if they don't throw an error. Also, all of the tests for orFail need to be updated.

If you want to submit a PR, I'm happy to stand aside on this one :smile: just let me know if that's the case.

[gsandorx]

@lineus I just did it :)

I didn't add the return res to the default case, just in case undefined was more appropriate for an unsupported query type. Maybe I'm wrong :)