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

Inconstancy with .set("key", undefined) in middlewares #12155

Closed
2 tasks done
Jule- opened this issue Jul 26, 2022 · 2 comments
Closed
2 tasks done

Inconstancy with .set("key", undefined) in middlewares #12155

Jule- opened this issue Jul 26, 2022 · 2 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@Jule-
Copy link

Jule- commented Jul 26, 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.4.1

Node.js version

16.14.2

MongoDB server version

4.2.3

Description

Depending on how you define your update payload, it will result in different behaviours in Query middlewares when you try to set some property to undefined with this.set("key", undefined) method.

  1. without $set: setting to undefined have no effect and will use property value from update object
  2. with $set: setting to undefined will effectively remove property value from update object

NB: I have not checked for Document middlewares but it could be the same.

Steps to Reproduce

const testSchema = new Schema({
  key: { type: String, unique: true, sparse: true },
  // [Use Case] `unique` and `sparse` not necessary just here because it
  // illustrate a real case scenario impacted by this issue.
});

testSchema.pre('findOneAndUpdate', function (next) {
  if (this.get('key') === '') this.set('key', undefined);
  return next();
});

const Test = mongoose.model('Test', testSchema);

// [Use Case] Getting empty string from web client and you do not want to
// rely on usage to clean the update object and ensure what is/should be
// bound to the scheme definition

await Test.findOneAndUpdate({}, { key: '' });
// => { $set: { apiKey: '' } }

await Test.findOneAndUpdate({}, { $set: { key: '' } });
// => { $set: {} }

Expected Behavior

I would expect that both usage of query.set('key', undefined) results in the same generated query update { $set: {} }.

@Jule-
Copy link
Author

Jule- commented Jul 26, 2022

mongoose/lib/query.js

Lines 2031 to 2044 in 62f5c33

Query.prototype.set = function(path, val) {
if (typeof path === 'object') {
const keys = Object.keys(path);
for (const key of keys) {
this.set(key, path[key]);
}
return this;
}
this._update = this._update || {};
this._update.$set = this._update.$set || {};
this._update.$set[path] = val;
return this;
};

Here we can see that regarding the failing use case we have this._update equals to { key: "" } and set will change it to {key: "", $set: { key: undefined }}.

Then I guess that the query resolution will default top-level field assignments into $set which loses the information that the middleware wants to set key to undefined.

@vkarpov15 vkarpov15 added this to the 6.5.2 milestone Jul 26, 2022
@IslandRhythms IslandRhythms added the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label Aug 8, 2022
@IslandRhythms
Copy link
Collaborator

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

const testSchema = new mongoose.Schema({
    key: { type: String, unique: true, sparse: true },
    // [Use Case] `unique` and `sparse` not necessary just here because it
    // illustrate a real case scenario impacted by this issue.
  });
  
  testSchema.pre('findOneAndUpdate', function (next) {
    if (this.get('key') === '') this.set('key', undefined);
    return next();
  });
  
  const Test = mongoose.model('Test', testSchema);
  
  // [Use Case] Getting empty string from web client and you do not want to
  // rely on usage to clean the update object and ensure what is/should be
  // bound to the scheme definition
  

  async function run() {
    await mongoose.connect('mongodb://localhost:27017');
    await mongoose.connection.dropDatabase();
    await Test.findOneAndUpdate({}, { key: '' });
    // => { $set: { apiKey: '' } }
    
    await Test.findOneAndUpdate({}, { $set: { key: '' } });
    // => { $set: {} }
  }

  run();

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