Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.96k stars 3.84k forks source link

`defaults` option for queries #7287

Closed mhombach closed 3 years ago

mhombach commented 5 years ago

Do you want to request a feature or report a bug?

Possible bug or unexpected behaviour

What is the current behavior?

.find() overwrites fields that are undefined but have a default value with that exact value, making it inconsistent and not possible to see if such that value is set in the database or not. See #7269 for reference.

If the current behavior is a bug, please provide the steps to reproduce.

const mongoose = require("mongoose")
const { Schema } = mongoose;
const PersonSchema = new Schema({
  name: String,
  age: { type: Number, default: 0 }
})
const Person = mongoose.model("Person", PersonSchema)
mongoose.connect('mongodb://localhost:27017/test');

async function runTest() {
  // creating document with manual set value for "age"
  await Person.create({ name: "martin", age: 30 });

  // get document
  let entry = await Person.findOne();
  console.log(entry); // entry.age == 30

  // deleting the key "age"
  await Person.collection.update({ _id: entry._id }, { $unset: { age: 1 }});
  // attribute "age" is now not existing on that document in the database-storage

  let newResult = await Person.findOne();
  console.log(newResult); // attribute "age" is now existing again on the runtime object and has the default value of 0
}
runTest();

What is the expected behavior? The default value of an attribute should be used on creation or maybe even on update (https://mongoosejs.com/docs/defaults.html#the-setdefaultsoninsert-option) but NOT on pure reading a document from the database.

Please mention your node.js, mongoose and MongoDB version. node v8.11.3, mongoose 5.3.13, mongoDB latest version

vkarpov15 commented 5 years ago

This is something worth discussing more. Currently, this is by design. The spirit of this behavior is to apply defaults where possible, because setDefaultsOnInsert isn't always on. This way, you can use defaults to avoid an extra != null check if you want to use String#startsWith() or similar.

You can always use Document#$isDefault() to check whether age is set to the default value or not.

mhombach commented 5 years ago

@vkarpov15 i kinda agree that it's more writing when you need to null-check attributes and can't rely on it beeing an empty string or so. But the thing that made me create this issue is the pure fact, that "reading" a mongoose-document should, if no custom hooks are modifying it, return the object "as it is" from the database. The intent on pulling some document from the database is to know the values that are stored in the database. If there is an attribute that is undefined there will always be a reason why that value is saved as undefined into the database. In my opinion one should start rethinking there and not letting the database set "some value the nest time the data is pulled from the db". Scenario:

GirayEryilmaz commented 3 years ago

Any updates on this issue? We sometimes deliberately put empty objects to the database and count on them to be empty when we fetch them. But they are populated with the default values.

vkarpov15 commented 3 years ago

@GirayEryilmaz you can use Document#$isDefault() to check if a value is set using a default.

We'll add a new option to disable defaults on queries, since we already have a defaults option for documents in https://github.com/Automattic/mongoose/commit/9609e87418b97064adc98bcaa47f85a09d899c06 and #8271

vkarpov15 commented 3 years ago

TLDR; Model.findOne().setOptions({ defaults: false }) should skip applying defaults to the result document. See the changes from https://github.com/Automattic/mongoose/commit/9609e87418b97064adc98bcaa47f85a09d899c06 to learn about the defaults option for the Model() constructor.

hugomallet commented 3 years ago

Hello,

The default behaviour may be helpful for values like booleans. But for other kind of values like creation date, ids etc., it's not a desired behaviour. Especially if the default option is "setDefaultsOnInsert" only.

I think the default behaviour should be reversed. And we should be able to fill in defaults on demand when reading. If this change involves an annoying breaking change, i suggest adding an option on schema like :

uuid: {
    type: String,
    default: () => uuid(),
    setDefaultOnRead: false,
},
vkarpov15 commented 3 years ago

@hugomallet I don't understand your issue, can you please clarify?

hugomallet commented 3 years ago

@vkarpov15, my case was :

I added a new field scheduledAt to an existing model. This fields defaults to the current date at the time of the insert if not defined. Something like this :

scheduledAt: {
    type: Date,
    default: () => new Date(),
},

My issue is that the previous objects have an undefined scheduledAt field. But when i read, scheduledAt equals to the current date (read time) which is an unwanted value because it changes at every read. I expected to have undefined instead of an incorrect value.

vkarpov15 commented 3 years ago

@hugomallet you can create a middleware that sets defaults to false for all find and findOne operations:

schema.pre(['find', 'findOne'], function disableDefaults() {
  this.setOptions({ defaults: false });
});

Would that help?

IYCI commented 3 years ago

Hi, @vkarpov15 Thanks for adding defaults as a model constructor option!

In addition to Model.findOne().setOptions({ defaults: false }) Is it possible to also have defaultsOnRead: false added as a schema options property?

junjenfong commented 2 years ago

@hugomallet you can create a middleware that sets defaults to false for all find and findOne operations:

schema.pre(['find', 'findOne'], function disableDefaults() {
  this.setOptions({ defaults: false });
});

Would that help?

Doesn't seems to work on me. Thoughts? I have the exact same issue as hugomallet

vkarpov15 commented 2 years ago

@junjenfong please open a new issue and follow the issue template. "Doesn't work" isn't an actionable bug report.