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

Global default required configuration #6662

Closed
joeytwiddle opened this issue Jul 4, 2018 · 8 comments
Closed

Global default required configuration #6662

joeytwiddle opened this issue Jul 4, 2018 · 8 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature

Comments

@joeytwiddle
Copy link
Contributor

joeytwiddle commented Jul 4, 2018

#6578 (comment)

mongoose.set('required', true) to make all fields required: true by default.

I know Valeri was not so keen on this idea, so I will try to provide my justification below...

(I brought my comments to this new issue because I thought it might be clearer in a fresh thread.)

@joeytwiddle
Copy link
Contributor Author

joeytwiddle commented Jul 4, 2018

I can imagine that developers who have been using mongoose for some time will find this counter-intuitive.

But I love setting required: true globally at the start of a project. Let me explain why...

I have worked with developers who create a schema without troubling to mark which fields are required:

const userSchema = new Schema({
  name: String,             // should have been marked required
  email: String,            // should have been marked required
  favouriteColor: String,   // optional, ok
});

By the time I realise that they missed the option, it's too late: our database now has invalid documents in it! (We have users without names, or without email addresses, because buggy code never threw any errors.)

Now on the front end, I cannot trust that user.name.length will always work. Occasionally it might throw an error.

Furthermore, sometimes I will look at a schema and I have no idea which fields should have been marked required and which should not. It is ambiguous.

I would rather force all fields to be required by default. Then if the developer really does want some fields to be optional, they can add required: false to the schema. (They will usually do this quickly, after trying to create their first minimal test document.) By this stage, the intent of the schema code is very clear.

So I find enabling this feature helps us to keep our DB and schemas in order.

(In fact, when I write a schema, I always write required: true or required: false. That makes it clear that I have thought about it, and made a decision. And that code will always have the same behaviour regardless of the global setting.)


But anyway, no worries if I haven't convinced you. I can continue to use it in our projects, without requiring a special patch.

I am not sure what the down-sides might be, probably because I haven't been using mongoose in that way.


For anyone who needs it, this is the code I used in the past to achieve "required by default":

const makeFieldsRequiredByDefault = true;

function createModel (modelName, schema) {
  if (makeFieldsRequiredByDefault) {
    // If the developer adds a field to the schema, without specifying its `required` setting, we will make it default to true.
    for (const pathName of Object.keys(schema.paths)) {
      const fieldSpec = schema.paths[pathName];
      const options = fieldSpec.options;
      if (options.type === Array || Array.isArray(options.type)) {
        // We don't want to mark Arrays as 'required' anyway, because Mongoose interprets that as "must contain at least 1 entry".  (Although that behaviour might change in v4.)
        continue;
      }
      if (options.required === undefined) {
        //console.log(`Marking ${name}.${pathName} as required.`);
        schema.path(pathName).required(true);
      }
    }
  }

  return db.model(modelName, schema);
}

@vkarpov15 vkarpov15 added this to the Parking Lot milestone Jul 4, 2018
@vkarpov15 vkarpov15 added the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Jul 4, 2018
@vkarpov15
Copy link
Collaborator

Worth considering, thanks for the detailed justification. My concern is more just API consistency. The mongoose.set('strict', 'throw') approach is consistent with options like bufferCommands, where the mongoose global can modify defaults that schema options can override. But we don't have a precedent for a mongoose global option that modifies a default for schema types.

@marcelh-gh
Copy link

@vkarpov15
There might be another use case for a global schema setting.
I have a rather large amount of schemas, many of them nested into each other.
Most of them have getters and setters.
To have those executed when necessary I need schema.set('toObject', { getters: true });.
Without a global setting this has to be done for every schema. A global setting that could be modified
locally would be much more convenient.

Since this exists already:

'objectIdGetter': true by default. Mongoose adds a getter to MongoDB ObjectId's called _id that returns this for convenience with populate. Set this to false to remove the getter.

...I think it would not be inconsistent to have more global settings like this.
Any thoughts?

@vkarpov15
Copy link
Collaborator

That's a good idea, I opened up an issue to track.

@libook
Copy link

libook commented May 7, 2019

I always made mistakes on spelling "required". It is hard to find out. It may be very helpful if there is an option can make all fields required on default. I can also set "required: false" on fields when I need. And this kind of spelling issues won't affect data in database.

@vkarpov15
Copy link
Collaborator

@vkarpov15 vkarpov15 removed this from the Parking Lot milestone Mar 17, 2020
@joeytwiddle
Copy link
Contributor Author

joeytwiddle commented Apr 23, 2020

Thanks!

I see that not all types have the set function (for example DocumentArray and Embedded do not).

Therefore, for my use case, I think I can set all fields of all types required by default, like this:

if (makeFieldsRequiredByDefault) {
  const allMongooseTypes = Object.values(mongoose.Schema.Types);
  allMongooseTypes.forEach(type => {
    if (typeof type.set === 'function') {
      type.set('required', true);
    }
  });
}

@vkarpov15
Copy link
Collaborator

Thanks for pointing that out @joeytwiddle, we will add that in a future release 👍

But yes that should work for setting all types to be required by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

4 participants