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

Do not pass objectId, updatedAt, createdAt to beforeSave hooks on object create. #587

Merged
merged 2 commits into from
Feb 23, 2016

Conversation

nlutsenko
Copy link
Contributor

Cloud Code never passed these on beforeSave, so move them to after beforeSave hooks run to make sure we don't pass them.
This unbreaks isNew(), since we are not going to have objectId anymore.
Also updated the test to accommodate for the behavior change.

Fixes #107

cc @wangmengyan95 since it looks like you changed this explicitly at some point and tests blame to you.

@simonbengtsson
Copy link
Contributor

This is great for backwards compatibility I guess, but I kind of liked the new behavior. Especially with the req.original object. Would be nice to somehow unbreak isNew() without removing objectId etc from req.object, but that might be wishful thinking.

@nlutsenko
Copy link
Contributor Author

You still will get req.original on beforeSave when the object is going to be updated.

@nlutsenko
Copy link
Contributor Author

Let me expand a little bit more here:
The only piece this affects is a beforeSave hook when there was no object ever created and you are getting a new object - where now you'll get an object without an id, updatedAt, createdAt.
This shouldn't affect the new semantics for original vs object on afterSave hooks.

@gfosco gfosco assigned nlutsenko and unassigned gfosco Feb 23, 2016
nlutsenko added a commit that referenced this pull request Feb 23, 2016
Do not pass objectId, updatedAt, createdAt to beforeSave hooks on object create.
@nlutsenko nlutsenko merged commit 1f707a9 into master Feb 23, 2016
@nlutsenko nlutsenko deleted the nlutsenko.cc.test branch February 23, 2016 06:13
@wangmengyan95
Copy link
Contributor

LGTM!

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.

5 participants