findByIdAndRemove callback passes null instead of matching doc

[qstraza]

Docs are stating that findByIdAndRemove:

Finds a matching document, removes it, passing the found document (if any) to the callback.

In my case callback always passes null.

Nodejs: v7.5.0
Mongodb: v3.4.1
mongoose: 4.8.4

To reproduce use:

var mongoose = require('mongoose');
mongoose.connect('mongodb://localhost/ex');

var Schema = mongoose.Schema;

var Example = mongoose.model('Example',
  new mongoose.Schema({
    description: String
  })
);

Example.find({}, function(err, doc) {
  console.log('Finding all: ', doc)
  var example = new Example();
  example.description = 'this is a description';
  example.save(function (err){
    if (err) return;
    console.log('Saved: ', example);
    Example.find({}, function(err, doc) {
      console.log('Finding all: ', doc)
      Example.findByIdAndRemove(example._id, function(doc) {
        console.log('findByIdAndRemove doc: ', doc);
        Example.find({}, function(err, doc) {
          console.log('Finding all: ', doc)
        })
      })
    })
  })
})

Package.json and above JS code attached here.
bug.zip

[sobafuchs]

Remember that in node, callbacks usually take 2 parameters. The first parameter is the error, the second parameter is the result.

You are logging the error as doc:

Example.findByIdAndRemove(example._id, function(doc) { // doc here is actually err
        console.log('findByIdAndRemove doc: ', doc);
        Example.find({}, function(err, doc) {
          console.log('Finding all: ', doc)
        })
      })

What you want to do is:

Example.findByIdAndRemove(example._id, function(err1, doc1) { // doc here is actually err
        // handle err1
        console.log('findByIdAndRemove doc: ', doc1);
        Example.find({}, function(err2, docs) {
          console.log('Finding all: ', docs)
        })
      })

[dkpbot]

this sometimes returns null even if you use the correct callback params, but will correctly delete regardless. if you need to know whether the doc existed in the first place or not this function is not reliable.

[thammarith]

todo.findByIdAndRemove(id, (err, todo) => {
  if(err) {
    return next(new Error('Todo was not found!'));
  }
  res.json('Successfully removed');
});

When the record doesn't exist, the err === null and it never enters the if(err). What should I do to check if the record exists?

[lineus]

@dkpbot findByIdAndRemove() will only return null for the result when there's an error or when 0 documents matched the _id passed in, i.e. when you're trying to delete a doc that has already been deleted or never existed.

one thing to look out for is awaiting the query while simultaneously passing in a callback. e.g.

  await MyModel.findByIdAndRemove(someId, (err, res) => { .. });

This will result in the query being executed twice, and whichever one gets there second, sees no doc to delete.

@thammarith if both result and error are null, then that collection does not contain a document with the _id that you specified to delete. Here's a gist that shows the expectations and has another way to delete a doc, that you can chain .orFail() with and get an error back in either the .catch() or the callback.

deleteOne docs here
orFail docs here

[thammarith]

@lineus That works! Thanks so much!

[dkpbot]

@lineus i was handling err in a callback. i did not know that made it execute twice. thank you.

[ravibalg-aj]

It deletes the document successfully, but callback document <message> is null!?
Why?

Any adjustment in code

[lineus]

@ravibalg-aj you are awaiting and passing a callback simultaneously as described above. You need to do one or the other, but not both.

[ravibalg-aj]

one thing to look out for is awaiting the query while simultaneously passing in a callback. e.g.

  await MyModel.findByIdAndRemove(someId, (err, res) => { .. });

This will result in the query being executed twice, and whichever one gets there second, sees no doc to delete.

In the above reply, you said await the query simultaneously pass the callback!
that's what I tried!
Am I doing wrong!?

[lineus]

one thing to look out for

In this context, this turn of phrase means one thing to avoid. In other words, don't ever await the promise and pass a callback simultaneously into any mongoose query.

I do my best to avoid being ambiguous but I clearly failed here. You should either handle the promise with .then await etc, or pass a callback in.

[ravibalg-aj]

Okay cool!

But actually I found this issue, during searching "findbyidandremove() sometimes returns null document on callback but deletes the document successfully!"

And found your answer - adding await will solve the problem!

But even after using that first two calls worked perfectly(means deleted and returned the document successfully) but after that the problem occurs!

What I'm trying to say is that even before adding "await", I had the same issue!

[lineus]

@ravibalg-aj If you can come up with a script that demonstrates the behavior you are describing, please open a new issue following the template. Here is an example script that uses assertions to explicitly describe the anticipated behavior:

5022.js

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

const assert = require('assert');
const mongoose = require('mongoose');
const { Schema, connection } = mongoose;
const uri = 'mongodb://localhost:27017/5022'
const opts = {
    useUnifiedTopology: true,
    useNewUrlParser: true
};

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

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

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

run();

async function run() {
    await mongoose.connect(uri, opts)
    await connection.dropDatabase();
    let inserted = await Test.create(test)
    assert.ok(inserted && inserted.name === 'test')
    let deletedResponse = await Test.findByIdAndDelete(inserted._id);
    assert.strictEqual(deletedResponse.id, inserted.id);
    let found = await Test.findOne({ _id: inserted.id });
    assert.strictEqual(found, null);
    return connection.close();
}

Output:

$:~/dev/mongoose/issues$ ./5022.js 
All Assertions Pass.
$:~/dev/mongoose/issues$ 

[ravibalg-aj]

const message = await Messages.findByIdAndRemove(req.params.id);

    if (!message) {
        return res.status(400).json(
            {
                success: false,
                error: 'Message not found!'
            }
        )
    }

    return res.status(200).json({
        success: true,
        data: message,
    })

Tried this and working properly!
Don't know what I did wrong previously!
Anyway thank you for your time!
🤗

[vkarpov15]

Thanks @lineus