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

Monkey patching of ObjectId, but still undefined? #14154

Closed
2 tasks done
nicolas-urbantz opened this issue Dec 5, 2023 · 10 comments
Closed
2 tasks done

Monkey patching of ObjectId, but still undefined? #14154

nicolas-urbantz opened this issue Dec 5, 2023 · 10 comments
Labels
can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity.

Comments

@nicolas-urbantz
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

7.6.4

Node.js version

20

MongoDB server version

5.9.1

Typescript version (if applicable)

5.1.6

Description

Hello!

We are upgrading an old codebase from mongoose 5 to 7. I see that mongoose is monkey patching the "native" ObjectId by adding a _id attribute... But I'm wondering if the type is correct because in the code, I can see:

declare module 'bson' {
  interface ObjectId {
    /** Mongoose automatically adds a conveniency "_id" getter on the base ObjectId class */
    _id: this;
  }
}

while, in reality, it's undefined (see steps to reproduce).

I saw that issue, but it doesn't answer my question :/

Steps to Reproduce

This is my unit test:

import { ObjectId as MongoDBObjectId } from 'mongodb';
test('_id is undefined', () => {
	const objectId = new MongoDBObjectId('5b348a71fe5e561dd1c70628');
	expect(objectId._id).toBeUndefined();
});

Expected Behavior

It passes, but shouldn't it fail? Or shouldn't the monkey patch say that _id could be undefined?

@vkarpov15
Copy link
Collaborator

_id won't be added as a property on MongoDBObjectId unless you import Mongoose. So your unit test failing looks expected, unless there's an import 'mongoose' somewhere.

@vkarpov15 vkarpov15 added the needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity label Dec 5, 2023
@nicolas-urbantz
Copy link
Author

Hey, thanks for your quick answer... I tried to run this then, and it passes:

  • One test with the "native" ObjectId => Stil no _id
  • One test with the ObjectId from mongoose => _id is present
import { ObjectId as MongoDBObjectId } from 'mongodb';
import mongoose from 'mongoose';

describe('Native ObjectId', () => {
	test('_id is undefined', () => {
		const objectId = new MongoDBObjectId('5b348a71fe5e561dd1c70628');
		expect(objectId._id).toBeUndefined();
	});
});

describe('mongoose ObjectId', () => {
	test('_id is undefined', () => {
		const objectId = new mongoose.Types.ObjectId(
			'5b348a71fe5e561dd1c70628'
		);
		expect(objectId._id).toBe(objectId);
	});
});

Copy link

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Dec 21, 2023
@nicolas-urbantz
Copy link
Author

Up :) I add some clarification, but I don't know if/how I can remove the label @vkarpov15

@github-actions github-actions bot removed the Stale label Dec 22, 2023
@vkarpov15 vkarpov15 added needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue and removed needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity labels Jan 1, 2024
@vkarpov15 vkarpov15 added this to the 7.6.8 milestone Jan 1, 2024
@IslandRhythms IslandRhythms added can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Jan 3, 2024
@IslandRhythms
Copy link
Collaborator

Please modify the script to demonstrate your issue.

const mongoose = require('mongoose');
const { ObjectId } = require('mongodb');
let MongoDBObjectID = ObjectId;

const mongodbId = new MongoDBObjectID('5b348a71fe5e561dd1c70628');
console.log('what is mongodbId', mongodbId);

const mongooseObjectId = new mongoose.Types.ObjectId('5b348a71fe5e561dd1c70628');
console.log('what is mongoose object id', mongooseObjectId);
console.log(mongooseObjectId._id);

@vkarpov15
Copy link
Collaborator

I modified @IslandRhythms ' script slightly, but still not able to repro this issue:

const mongoose = require('mongoose');
const { ObjectId } = require('mongodb');
let MongoDBObjectID = ObjectId;

const mongodbId = new MongoDBObjectID('5b348a71fe5e561dd1c70628');
console.log('what is mongodbId', mongodbId._id);

const mongooseObjectId = new mongoose.Types.ObjectId('5b348a71fe5e561dd1c70628');
console.log('what is mongoose ObjectId', mongooseObjectId._id);

Output:

$ node gh-14154.js 
what is mongodbId new ObjectId('5b348a71fe5e561dd1c70628')
what is mongoose ObjectId new ObjectId('5b348a71fe5e561dd1c70628')

@nicolas-urbantz can you please run npm run list | grep "mongo" and provide the output? I suspect you have a different version of the MongoDB Node driver than Mongoose.

@vkarpov15 vkarpov15 removed this from the 7.6.8 milestone Jan 3, 2024
@nicolas-urbantz
Copy link
Author

Thanks, I'll have a look tomorrow 🫶

@nicolas-urbantz
Copy link
Author

@vkarpov15 , you are right I think, we have another version.

I copy/paste your script and here is my output:

what is mongodbId undefined
what is mongoose ObjectId new ObjectId("5b348a71fe5e561dd1c70628")

Output of npm list:

├── mongodb@5.9.1
├── mongoose@7.6.4

I see that mongoose is using 5.9.0 in the lock. Do you recommend to not install mongodb "manually", but then use the "mongodb of mongoose", if I want to call mongodb directly (without mongoose)?

@vkarpov15
Copy link
Collaborator

@nicolas-urbantz yes I'd recommend you use require('mongoose').mongo, or match your version of the mongodb package to Mongoose's.

@nicolas-urbantz
Copy link
Author

Make sense!

Thank you for your time 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity.
Projects
None yet
Development

No branches or pull requests

3 participants