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

feature: "new" alternative that disables defaults down the entire tree (incl. subdocs) #8271

Closed
captaincaius opened this issue Oct 23, 2019 · 9 comments
Labels
discussion If you have any thoughts or comments on this issue, please share them! enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@captaincaius
Copy link
Contributor

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

feature

What is the current behavior?

For both persistent models and browser-side models, there's no way to disable setting defaults when new'ing up a document.

The reason you'd want to do this is something like this:

  • you have a main schema for a model
  • you've derived another schema from that one that contains only the fields a given user should be able to modify (foreign keys and internally managed fields stripped)
  • you want to do something like this to return a sanitized representation of the body (no illegal fields, and all fields schema-validated) for e.g. a PUT handler:
const doc = new EditableFieldsOnly(req.body);
if(doc.validateSync(undefined, {validateModifiedOnly: true}) !== undefined) {
  // throw somethin' will ya?
}
return doc.toObject({transform: stripIds});

The problem is that new will always add defaults. So that won't do quite what you want it to do, as it'll clobber the existing document in the DB with defaults.

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

What is the expected behavior?

  1. A Model-like class that only has
  • a constructor (which simply does what the "client-side documents" guide says)
  • a different method returns an object just like new does, but without defaults
  1. Model itself having the second method added (since it's free)

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

node 10, mongoose master

@vkarpov15
Copy link
Collaborator

Hmm you might be able to do something like this with Schema#eachPath():

schema.clone().eachPath((pathname, schematype) => {
  delete schematype.options.default;
});

That should work I think.

On the other hand, why don't you just use update validators and updateOne()? Update validators are designed for this exact case: PUT requests with some paths defined, so cast and validate the paths that are in the update, and ignore the rest.

@vkarpov15 vkarpov15 added discussion If you have any thoughts or comments on this issue, please share them! enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature labels Oct 28, 2019
@captaincaius
Copy link
Contributor Author

I'm currently doing something similar to what you're suggesting, but on the schema definition object.

The reason I can't use update validators is because this is to make short-lived "documents" not to-be-saved to the database. They're just for creating a sanitized-against-schema copy of a request's body like so:

  1. New up a mongoose document using the request body
  2. If it doesn't validate, reject the request
  3. If it does, toObject() the document and use the resulting POJO through the rest of the request.

actual code here: https://github.com/captaincaius/hipthrusts/blob/master/src/mongoose.ts#L77

Anyway feel free to close this if you think my use-case isn't worth adding stuff to mongoose - I can always maintain a schema-specific-document factory within my project instead.

@vkarpov15
Copy link
Collaborator

Here's an idea: how about supporting defaults as an option to a model constructor?

// Create doc, skip all defaults
const doc = new UserModel(req.body, null, { defaults: false });

Would that work for your use case?

@captaincaius
Copy link
Contributor Author

Thanks so much for the reply and for giving this some thought!

Yeah that would totally work - to benefit my use-case, it would need to be on the Document constructor itself, but I think to make the example code you posted it end up coming with that for free anyway.

FYI my workaround at this point is just 13 lines of code https://github.com/captaincaius/hipthrusts/blob/master/src/mongoose.ts#L45 so I'm starting to wonder if this request is worth dumping the extra maintenance burden onto mongoose. If it's anything more than a one-liner I'd say for me personally it's ok to ditch this.

@vkarpov15
Copy link
Collaborator

Not running defaults should only be a few extra lines. I'll keep this open for now since it is no rush, but I won't be actively working on it for now.

@vkarpov15 vkarpov15 added this to the 5.x Unprioritized milestone Nov 6, 2019
@adamreisnz
Copy link
Contributor

Any update on this one? It would be great if we could disable defaults, as it's causing an issue in one of my plugins and the easiest way to fix would be if we could new a model without defaults.

@vkarpov15
Copy link
Collaborator

@adamreisnz not yet, but we'll prioritize this.

@adamreisnz
Copy link
Contributor

Awesome, thanks for pushing this through!

@vkarpov15
Copy link
Collaborator

Always happy to help @adamreisnz , cheers 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion If you have any thoughts or comments on this issue, please share them! 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