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

feat: allow top level dollar keys with findOneAndUpdate(), update() #13786

Merged
merged 3 commits into from Aug 29, 2023

Conversation

vkarpov15
Copy link
Collaborator

Summary

Right now, MongoDB allows storing top-level keys that start with $, but our castUpdate() function doesn't allow top-level dollar keys.

'use strict';
  
const mongoose = require('mongoose');
mongoose.set('debug', true);

void async function main() {
  await mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');
  //await mongoose.connection.dropDatabase();

  await mongoose.connection.collection('test').insertOne({ $set: [1,2, 3, 4] });
  console.log(await mongoose.connection.collection('test').find().toArray());
  console.log('Done');
}();

Examples

@vkarpov15 vkarpov15 added this to the 7.5 milestone Aug 26, 2023
hasezoey
hasezoey previously approved these changes Aug 27, 2023
@hasezoey
Copy link
Collaborator

since when does mongodb allow that? because mongodb 4.4 tests fail with "not allowed"

@hasezoey hasezoey dismissed their stale review August 27, 2023 08:33

tests failing, mongodb 4.4 seemingly does not support it

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.

I second hasezoey's concern, I feel like we should either:
A) err on the side of caution for now and keep blocking adding keys that start with $.
B) Make it conditional, by fetching the server information and making sure it supports those keys.

A more performant alternative approach to B is to keep things as is in the PR, but add error-handling that throws an error an error along these lines: mongo does not allow storing keys that start with a $ sign, please upgrade your mongodb server version to version x.y or later.

@vkarpov15
Copy link
Collaborator Author

It looks like MongoDB 5 was the first version to support top-level keys that start with $.

I disagree that we should be checking the MongoDB server version for 2 reasons:

  1. Historically, Mongoose has never relied on MongoDB server version. As new features are introduced, we support them, and rely on the MongoDB server to throw an error if you use a feature that a particular version of the MongoDB server doesn't support. For example, we didn't implement server version checks for change streams, transactions, etc.
  2. It is entirely possible to have multiple versions of MongoDB in one replica set, or even have one version of a mongos proxy in front of a different version of mongod replica set. So "what is the current MongoDB version?" is a more nuanced version than one might think when you're talking about a library that connects to MongoDB and enables/disables functionality based on version. And opens the door to us having to test what happens during MongoDB server version upgrades.

@AbdelrahmanHafez
Copy link
Collaborator

Fair points, we can just leave it up to the server to throw the proper errors when needed while supporting the feature for those who have more recent versions.

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

looks good to me, but this should definitely be mentioned in the changelog and in the documentation that only mongodb 5 and up support top-level $ keys

@vkarpov15 vkarpov15 merged commit 29de9c4 into 7.5 Aug 29, 2023
42 checks passed
@vkarpov15 vkarpov15 deleted the vkarpov15/handle-top-level-dollar-keys branch August 29, 2023 16:26
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