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

BSON ObjectId is not compatible with Types.ObjectId #12537

Closed
2 tasks done
kYem opened this issue Oct 6, 2022 · 15 comments · Fixed by #13515
Closed
2 tasks done

BSON ObjectId is not compatible with Types.ObjectId #12537

kYem opened this issue Oct 6, 2022 · 15 comments · Fixed by #13515
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@kYem
Copy link

kYem commented Oct 6, 2022

Prerequisites

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

Mongoose version

6.6.5

Node.js version

14.19.3

MongoDB server version

4.4

Description

I'm trying to upgrade to v6 of mongoose but keep receiving multiple errors related to ObjectId

          Types of property 'evidence' are incompatible.
            Type 'LeanDocument<ObjectId>[] | undefined' is not assignable to type 'ObjectId[] | undefined'.
              Type 'LeanDocument<ObjectId>[]' is not assignable to type 'ObjectId[]'.
                Type 'LeanDocument<ObjectId>' is missing the following properties from type 'ObjectId': toJSON, equals

That was new a error showing up after upgrade in a lot of places, LeanDocument 🤔 ?
I was expecting it to be original ObjectId, but I'm not sure if there is any way to change that other than updating interface?

Then that lead me to reading up recommend logic for ObjectId at this page: ObjectIds and Other Mongoose Types

Suggested ObjectId types

To define a property of type ObjectId, you should use Types.ObjectId in the TypeScript document interface. You should use ObjectId or Schema.Types.ObjectId in your schema definition.

Current codebase setup

Current approach is to use bson ObjectId as interface type and Schema.Types.ObjectId as schema type, therefore this will require to update all interfaces to use Types.ObjectId instead of bson ObjectId.

Now this is where the real problem is, seems like Types.ObjectId is not actually compatible with bson ObjectId due to required _id property. That would require huge amount of changes to replace new ObjectId() with new Types.ObjectId(). Is this an overlook? What is the reason Types.ObjectId is not compatible the actual underlying ObjectId class?

// Property '_id' is missing in type ('bson").ObjectID' but required in type 'import("mongoose").Types.ObjectId'. 
`node_modules/bson/bson").ObjectID' is not assignable to parameter of type 'import("mongoose").Types.ObjectId'`

Next steps

Determine if this an issue with queries now returning LeanDocument<ObjectId> instead of ObjectId? or incompatibility between Types.ObjectId and bson ObjectId?

I'm surprised this have not yet come up more 🤔

Let me know what is the best way to processed with upgrade!

Steps to Reproduce

import { Schema, Types } from 'mongoose';
import { ObjectId } from 'bson';

// 1. Create an interface representing a document in MongoDB.
interface IUser {
  name: string;
  email: string;
  organizationId: ObjectId;
}

// 2. Create a Schema corresponding to the document interface.
const userSchema = new Schema<IUser>({
  name: { type: String, required: true },
  email: { type: String, required: true },
  // And `Schema.Types.ObjectId` in the schema definition.
  organizationId: { type: Schema.Types.ObjectId, ref: 'Organization' }
});

const User = model<IUser>('User', userSchema);

const user = new User()
// Property '_id' is missing in type ('bson").ObjectID' but required in type 'import("mongoose").Types.ObjectId'. 
user.organizationId = new ObjectId()

Expected Behavior

bson ObjectId should be accepted as Types.ObjectId interface or query should not return LeanDocument<ObjectId>

@hasezoey hasezoey added the typescript Types or Types-test related issue / Pull Request label Oct 7, 2022
@hasezoey
Copy link
Collaborator

hasezoey commented Oct 7, 2022

Determine if this an issue with queries now returning LeanDocument instead of ObjectId? or incompatibility between Types.ObjectId and bson ObjectId?

this is only a problem with the bson-ObjectId and the mongoose-ObjectId

Is this an overlook? What is the reason Types.ObjectId is not compatible the actual underlying ObjectId class?

the problem (and typescript error) say that they are not compatible, because the mongoose side expects a mongoose object id (which is a extension from the mongodb objectid, which is a alias of bson's objectid), but you are only giving it a bson objectid, which does not have all the properties from a mongoose objectid

That was new a error showing up after upgrade in a lot of places, LeanDocument thinking ?

as a aside, what version were you upgrading from?


this type may become supported in the future, but (at least i) would recommend to switching over to only use the mongoose types (which are the proper type)
also the simplest solution would be to just change it now, it is not actually (from what i know) a breaking change (unless when you are exporting the type / schema and use it somewhere else in not the same project)

@kYem
Copy link
Author

kYem commented Oct 7, 2022

I was upgrading from 5.13.13.

What I don't understand is why the types are not compatible given that it's actually the same underlying object in JavaScript.

Digging more about it seems like mongoose is monkey patching bson ObjectId by adding _id property and making it only required in mongoose-ObjectId, but surely this should patch the ObjectId type as well because that is what mongoose is actually doing behind the scenes, right?


/**
 * Getter for convenience with populate, see gh-6115
 * @api private
 */

Object.defineProperty(ObjectId.prototype, '_id', {
  enumerable: false,
  configurable: true,
  get: function() {
    return this;
  }
});

so feels like it should try and patch bson ObjectId namespace and add that _id property there as well, therefore it will align with JavaScript implementation?

@hasezoey
Copy link
Collaborator

hasezoey commented Oct 7, 2022

Digging more about it seems like mongoose is monkey patching bson ObjectId by adding _id property and making it only required in mongoose-ObjectId, but surely this should patch the ObjectId type as well because that is what mongoose is actually doing behind the scenes, right?

true, mongoose is monkey-patching ObjectId from the mongodb driver and so is compatible at runtime, but as noted earlier it is a typescript (types) issue

so feels like it should try and patch bson ObjectId namespace and add that _id property there as well, therefore it will align with JavaScript implementation?

mongodb's ObjectId (is from what i know) a alias of bson's ObjectId (a reexport)
https://github.com/mongodb/node-mongodb-native/blob/ca51feca25ab1413df7efe9e79b1de051ceb21eb/src/index.ts#L24-L39


TL;DR: at runtime those "3" ObjectId's are the same object, but in types mongoose is extending from the mongodb one
see

class ObjectId extends mongodb.ObjectId {
_id: this;
}

and classes are "downwards compatible", but not "upwards compatible" (the latter is which your code is trying to do: trying to assign a "lower" class to a "upper" one)

to clarify: with "upwards" and "downwards" i mean inheritance chain where the top is the class that extended from a "lower" class

this at least explains the Property '_id' is missing in type error, but i dont know about the error with property evidence, because i dont see code relating to that
The Only fix in mongoose i can see is that all occurrences of ObjectId requirements are replaced with the lower requirement of mongodb.ObjectId

@kYem
Copy link
Author

kYem commented Oct 7, 2022

I solved the issue with module augmentation, which is what I feel like mongoose should be doing as well, to match what it does in the runtime.

declare module "bson" {
  interface ObjectId {
    _id: this;
  }
}

@vkarpov15 vkarpov15 added this to the 6.6.8 milestone Oct 21, 2022
@vkarpov15
Copy link
Collaborator

I'm unable to repro this. The following script compiles correctly with Mongoose 6.6.5 and TypeScript 4.8.4:

import { model, Schema, Types } from 'mongoose';
import { ObjectId } from 'bson';

// 1. Create an interface representing a document in MongoDB.
interface IUser {
  name: string;
  email: string;
  organizationId: ObjectId;
}

// 2. Create a Schema corresponding to the document interface.
const userSchema = new Schema<IUser>({
  name: { type: String, required: true },
  email: { type: String, required: true },
  // And `Schema.Types.ObjectId` in the schema definition.
  organizationId: { type: Schema.Types.ObjectId, ref: 'Organization' }
});

const User = model<IUser>('User', userSchema);

const user = new User()
// Property '_id' is missing in type ('bson").ObjectID' but required in type 'import("mongoose").Types.ObjectId'. 
user.organizationId = new ObjectId()

Output shows no errors:

$ ./node_modules/.bin/tsc --strict gh-12537.ts
$

Can you please modify the script to demonstrate your issue?

@vkarpov15 vkarpov15 added the can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. label Nov 2, 2022
@vkarpov15 vkarpov15 removed this from the 6.7.1 milestone Nov 2, 2022
@github-actions
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
Copy link

This issue was closed because it has been inactive for 19 days since being marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2022
@kYem
Copy link
Author

kYem commented Jun 15, 2023

@vkarpov15 is possible re-open it again?

import { model, Schema, Types } from 'mongoose';
import { ObjectId } from 'bson';

// 1. Create an interface representing a document in MongoDB.
interface IUser {
  name: string;
  email: string;
  // This needs to be Types.ObjectId so it will be different types
  organizationId: Types.ObjectId;
}

// 2. Create a Schema corresponding to the document interface.
const userSchema = new Schema<IUser>({
  name: { type: String, required: true },
  email: { type: String, required: true },
  // And `Schema.Types.ObjectId` in the schema definition.
  organizationId: { type: Schema.Types.ObjectId, ref: 'Organization' }
});

const User = model<IUser>('User', userSchema);

const user = new User()
// Property '_id' is missing in type ('bson").ObjectID' but required in type 'import("mongoose").Types.ObjectId'.
user.organizationId = new ObjectId()

The key on the example was type (as recommended by the docs) are Types.ObjectId

  // This needs to be Types.ObjectId so it will be different types
  organizationId: Types.ObjectId;

// that should still allow to assign
user.organizationId = new ObjectId()

That will produce the error:
image

gh-12537.ts:23:1 - error TS2741: Property '_id' is missing in type 'import("/home/test/example/node_modules/bson/bson").ObjectID' but required in type 'import("mongoose").Types.ObjectId'.

23 user.organizationId = new ObjectId()
   ~~~~~~~~~~~~~~~~~~~

  node_modules/mongoose/types/types.d.ts:83:7
    83       _id: this;
             ~~~
    '_id' is declared here.


Found 1 error in gh-12537.ts:23

@hasezoey
Copy link
Collaborator

can confirm

reproduction repository & branch: https://github.com/typegoose/typegoose-testing/tree/mongoose12537


like i said in my earlier comment, these classes are the same at runtime, but in types they are only downwards compatible (ie a mongoose.Types.ObjectId can be casted implicitly to bson.ObjectId) but not upwards compatible (ie a bson.ObjectId cannot be casted implicitly to mongoose.Types.ObjectId)

a solution to this would be for mongoose to patch the bson namespace directly with this change

@hasezoey hasezoey removed the Stale label Jun 15, 2023
@hasezoey hasezoey reopened this Jun 15, 2023
@vkarpov15 vkarpov15 removed the can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. label Jun 15, 2023
@vkarpov15 vkarpov15 added this to the 7.3.1 milestone Jun 15, 2023
@kYem
Copy link
Author

kYem commented Jun 15, 2023

I do agree, given that mongoose is expanding the functionality it should also adjust the interface to make the two types compatible.

At the very least adding documentation about and how to solve it for the user, would save a lot of time.

Currently I'm using a declaration file, to do module augmentation, that will fix the issue.

Example file: types/bson/index.d.ts (or what ever your tsconfig setup is)

{
  "compilerOptions": {
    //....
    "typeRoots": [
      "./types",
      "./node_modules/@types"
    ],
    "paths": {
      "*": [
        "node_modules/*",
        "types/*"
      ]
    },
  }
}
import mongoose from 'mongoose';

/** Augment bson ObjectId due to Mongoose doing monkey patching, but not updating interface  **/
declare module "bson" {

  export class ObjectId extends mongoose.Types.ObjectId {
    _id: this;
  }
}

@hasezoey
Copy link
Collaborator

export class ObjectId extends mongoose.Types.ObjectId

i dont recommend to do this, remove the extends mongoose because this will create a cycle of bson.ObjectId -> mongodb.ObjectId -> mongoose.Types.ObjectId -> bson.ObjectId

@kYem
Copy link
Author

kYem commented Jun 15, 2023

Hmm, I feel like originally it was needed for something, but currently it works without 🤔 so will keep simpler version

It is required to gain access to all other properties

image

@hasezoey

This comment was marked as outdated.

hasezoey added a commit to hasezoey/mongoose that referenced this issue Jun 16, 2023
hasezoey added a commit to hasezoey/mongoose that referenced this issue Jun 16, 2023
@hasezoey
Copy link
Collaborator

It is required to gain access to all other properties

when i tested locally in my reproduction repository i could not originally produce the error, but when i tried to make a mongoose PR it appeared. Turns out that you need to do a import 'bson'; so that typescript does not overwrite types but merges them instead (i have found no documentation about this, but this resolved the issue for me)

@vkarpov15 vkarpov15 added this to the 7.4.0 milestone Jun 18, 2023
ziedHamdi pushed a commit to ziedHamdi/user-credits that referenced this issue Oct 5, 2023
  ● Test suite failed to run

    src/impl/mongoose/dao/MongooseDaoFactory.ts:45:5 - error TS2322: Type 'OfferDao' is not assignable to type 'IOfferDao<ObjectId, IOffer<ObjectId>>'.
      Types of property 'loadSubOffers' are incompatible.
        Type '(parentOfferId: ObjectId) => Promise<IMongooseOffer[]>' is not assignable to type '(parentOfferId: ObjectId) => Promise<IOffer<ObjectId>[]>'.
          Types of parameters 'parentOfferId' and 'parentOfferId' are incompatible.
            Type 'ObjectId' is missing the following properties from type 'ObjectId': _bsontype, id, toHexString, toJSON, and 3 more.

    45     return this.offerDao;

 Known issue:
 Automattic/mongoose#12711
 Automattic/mongoose#12537
@Gamionaire
Copy link

Gamionaire commented Oct 24, 2023

I solved this problem downgrade mongodb form v6 to mongodb 5.9.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants