Skip to content
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.

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

breaking change in findOne(ObjectId(id)) since 5.7.5 - not documented #8268

Closed
tomgrossman opened this issue Oct 22, 2019 · 3 comments
Closed
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@tomgrossman
Copy link

Do you want to request a feature or report a bug?
bug

What is the current behavior?
using the following snippet:

const mongoose = require('mongoose');

const opts = {
    useNewUrlParser: true,
    useUnifiedTopology: true
};

const connection = mongoose.createConnection('mongodb://localhost:27017/test', opts);

const userSchema = new mongoose.Schema({
    name: String
}, {
    collection: 'users',
    versionKey: false
});

const testModel = connection.model('User', userSchema);

let testId = null;

async function insertTest () {
    testId = (await (new testModel({name: 'tom'})).save())._id;
}

async function findTest () {
    const result = await testModel.findOne(testId).lean().exec();
    console.log(result);
    //in 5.7.4 it prints the inserted test user
    //in 5.7.5 it prints null because of the BSON fix
}

async function run() {
    await insertTest();
    await findTest();
}

run();

When using mongoose 5.7.4 it works fine and returns the inserted user.
When using mongoose 5.7.5 it returns null.
This is due the a BSON vulnerability fix done in
#8222
f3eca5b

What is the expected behavior?
If it's a vulnerability, it must be fixed. But if it's a breaking change, it needs to be documented in the release notes and probably change the version to major.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
Node.js - 10
Mongoose: 5.7.4, 5.7.5, 5.7.6
MongoDB - 4.0

@sibelius
Copy link
Contributor

const result = await testModel.findOne(testId).lean().exec(); should not work

but

const result = await testModel.findOne({ _id: testId }).lean().exec(); should

I think it was a bad behavior before

@tomgrossman
Copy link
Author

@sibelius I agree. But it still a breaking change if you have to change the code.

@vkarpov15 vkarpov15 added this to the 5.7.7 milestone Oct 23, 2019
@vkarpov15 vkarpov15 added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Oct 23, 2019
vkarpov15 added a commit that referenced this issue Oct 23, 2019
@vkarpov15
Copy link
Collaborator

@tomgrossman sorry for the inconvenience - this backwards breaking change was definitely unintentional. We just weren't aware the MongoDB driver supported this: https://github.com/mongodb/node-mongodb-native/blob/0f399b5c355bdfe8d5c237854953e405b536cb8b/lib/collection.js#L325-L327 .

@sibelius no real reason why testModel.findOne(testId) shouldn't work. It's a neat bit of syntactic sugar, that's why we introduced findById().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Projects
None yet
Development

No branches or pull requests

3 participants