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(model): add castObject() function that casts a POJO to the model's schema #12120

Merged
merged 2 commits into from Jul 26, 2022

Conversation

vkarpov15
Copy link
Collaborator

Fix #11945

Summary

We're adding a couple of model helpers that make it easier to access various Mongoose features without creating a full document. We already have Model.validate() and Model.applyDefaults(). Model.validate() does some casting, but doesn't return a casted object. This Model.castObject() function takes in a POJO, and casts it to the given schema, except for creating Mongoose-specific data structures like document arrays and subdocuments. castObject() should return a POJO.

Examples

const Test = db.model('Test', mongoose.Schema({
  _id: false,
  num: Number,
  nested: {
    num: Number
  },
  subdoc: {
    type: mongoose.Schema({
      _id: false,
      num: Number
    }),
    default: () => ({})
  },
  docArr: [{
    _id: false,
    num: Number
  }]
}));

const obj = {
  num: '1',
  nested: { num: '2' },
  subdoc: { num: '3' },
  docArr: [{ num: '4' }]
};
const ret = Test.castObject(obj);

ret.num; // 1 as a number
ret.nested; // POJO
ret.nested.num; // 2 as a number
ret.subdoc; // POJO
ret.subdoc.num; // 3 as a number
ret.docArr; // Vanilla JS array
ret.docArr[0].num; // 4 as a number

});
});

it('throws if cannot cast', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vkarpov15 I'm a bit hesitant whether this be the default behavior or not.

The thing is, the current alternative to castObject would be:

const user = new User({ age: 'I am not a valid number' });
const castedUser = user.toObject(); // does not throw, just ignores the invalid field

I'm anticipating that people will start refactoring their code to use the more performant Model.castObject(...) and assume it works the same way as .toObject().
I think it'd be nice if we can have both functions behave the same way at some point in the future.

What do you think? @vkarpov15

Copy link
Collaborator

Choose a reason for hiding this comment

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

But then again, there is a downside to that unified behavior:
For the Document#toObject() approach, you can always Document#validate to see if the document is valid or not, but for Model.castObject() if we don't throw an error, there won't be a simple way for people to know whether their input object is valid or not.

Not necessarily agreeing/disagreeing, thinking out loud here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AbdelrahmanHafez with your example, user.toObject() won't throw, but user.save() will. It isn't that Mongoose ignores the invalid value for age, it just won't throw until save(), unless you overwrite age with a valid value.

It may be nice to add an option to avoid throwing, and instead just not set invalid fields.

Copy link
Collaborator

Choose a reason for hiding this comment

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

user.save() scenario is not my concern. What I'm concerned about is the rarer case where people just want to cast an object, so they use this new Document => Document.toObject() hack to cast the object.

An option to control error handling would be nice. 👍

@vkarpov15 vkarpov15 merged commit a935534 into 6.5 Jul 26, 2022
@vkarpov15 vkarpov15 deleted the vkarpov15/castobject branch July 26, 2022 22:16
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