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

fix(document): fix ObjectId conversion for external schemas #11841

Merged
merged 8 commits into from
May 28, 2022

Conversation

coyotte508
Copy link
Contributor

@coyotte508 coyotte508 commented May 27, 2022

Summary

When dealing with external schemas, i.e. a mongoose schema created inside another module, instanceOf may not work.

This results in the value of ObjectIds in the schema being converted to strings when saving to the database.

Examples

I have a monorepo project with pnpm. One module lib contains a schema, which I use to create a model in my program.

Both my program and the imported lib module have the same bson, mongoose and mongodb versions.

When dealing with external schemas, i.e. a mongoose schema created inside another module, instanceOf may not work.

This results in the value of ObjectIds in the schema being converted to strings when saving to the database.
@coyotte508

This comment was marked as outdated.

@Uzlopak
Copy link
Collaborator

Uzlopak commented May 28, 2022

Can you search the whole codebase for instanceof ObjectId, please?

@coyotte508
Copy link
Contributor Author

coyotte508 commented May 28, 2022

Can you search the whole codebase for instanceof ObjectId, please?

I didn't replace a few:

  • in cast/objectid.js, it's not necessary (but could improve performance) - feel free to correct it
  • in the docs, not familiar enough to know what to do there

Same for Decimal128

Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 👍

@Uzlopak
Copy link
Collaborator

Uzlopak commented May 28, 2022

LGTM!

@Uzlopak Uzlopak merged commit aece69f into Automattic:master May 28, 2022
@AbdelrahmanHafez AbdelrahmanHafez added this to the 6.3.5 milestone May 28, 2022
noseworthy added a commit to noseworthy/mongoose that referenced this pull request Jun 3, 2022
PR Automattic#11841 changed a whole bunch of checks for `instanceof ObjectId` to
use the `isBsonType()` function instead. Unfortunately some were missed
in the `./lib/schema/objectid.js` file.

Convert these last couple checks to the new format. I grepped the code
for `instanceof oid` to see if I got all the ones of this style and
couldn't find anymore.

This was found because my nested document with required ObjectIds
started failing validation when I loaded the documents from JSON files
in my tests using `bson.EJSON` after the release of 6.3.4 -> 6.3.5.
noseworthy added a commit to noseworthy/mongoose that referenced this pull request Jun 3, 2022
PR Automattic#11841 changed a whole bunch of checks for `instanceof ObjectId` to
use the `isBsonType()` function instead. Unfortunately some were missed
in the `./lib/schema/objectid.js` file.

Convert the required check for `objectid` to use both `instanceof` and
`isBsonType()` to ensure that all ObjectIds are valid.

This was found because my nested document with required ObjectIds
started failing validation when I loaded the documents from JSON files
in my tests using `bson.EJSON` after the release of 6.3.4 -> 6.3.5.
noseworthy added a commit to noseworthy/mongoose that referenced this pull request Jun 3, 2022
PR Automattic#11841 changed a whole bunch of checks for `instanceof ObjectId` to
use the `isBsonType()` function instead. Unfortunately some were missed
in the `./lib/schema/objectid.js` file.

Convert the remaining `instanceof` checks in `objectid.js` to use the
`isBsonType()` check instead so that all `ObjectId`s created from any
`bson` modules should validate/cast properly.

This was found because my nested document with required ObjectIds
started failing validation when I loaded the documents from JSON files
in my tests using `bson.EJSON` after the release of 6.3.4 -> 6.3.5.
noseworthy added a commit to noseworthy/mongoose that referenced this pull request Jun 3, 2022
PR Automattic#11841 changed a whole bunch of checks for `instanceof ObjectId` to
use the `isBsonType()` function instead. Unfortunately some were missed
in the `./lib/schema/objectid.js` file.

Convert the remaining `instanceof` checks in `objectid.js` to use the
`isBsonType()` check instead so that all `ObjectId`s created from any
`bson` modules should validate/cast properly.

This was found because my nested document with required ObjectIds
started failing validation when I loaded the documents from JSON files
in my tests using `bson.EJSON` after the release of 6.3.4 -> 6.3.5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants