Skip to content

Async validation#580

Closed
interlock wants to merge 3 commits intosequelize:masterfrom
interlock:async-validate
Closed

Async validation#580
interlock wants to merge 3 commits intosequelize:masterfrom
interlock:async-validate

Conversation

@interlock
Copy link
Copy Markdown
Contributor

This is an updated pull request for issue #275.

This PR actually breaks existing implementations, because we had to use promises as the return result. As a result, I suggest we create a 2.0 branch that implements this and other changes which may regress for people.

Docs would need to be updated for this update (if accepted in 2.0 branch, will provide this update).

Custom validators need to call next() or next(error_message) to operate correctly now.

model->validate() returns a promise, use like so

user.validate().success( function() {
  user.save();
}).error(function(errors) {
  // handle errors
});

Once accepted, it would be fairly easy to adjust a models save to call validate before save as well. There are a few PR's for that as well.

@mickhansen
Copy link
Copy Markdown
Contributor

Is it not possible to maintain support for sync validation? Afaik you can check the length on the function you receive, so you can check if its expected a callback param.

@mickhansen
Copy link
Copy Markdown
Contributor

Ah, the breaking change is how the validation result is returned, missed that one :)

@interlock
Copy link
Copy Markdown
Contributor Author

Ya, I'm not the original implementor of this solution; but, it looks like to get async support you can no longer trust the return result to return. Of course, perhaps we could wrap that in a function in the Chainer to fake it? That still leaves the actual validation() function which returns a promise now. No matter what, we break something.

Talking to sdepold_ on skype the other day and we both thought it would be a good idea to start a future branch for 2.0-wip and merge this in there. We could manage the 1.x series as far as it can go without major refactor and save big breaks for 2.0.

Thoughts?

@mickhansen
Copy link
Copy Markdown
Contributor

Seems like the way to go.

@interlock interlock mentioned this pull request May 17, 2013
@calmdev
Copy link
Copy Markdown

calmdev commented May 29, 2013

I'm pumped up and ready to test this. It's really sexy. Very nice work.

@sdepold
Copy link
Copy Markdown
Member

sdepold commented May 30, 2013

Hi dudes,

just wanted to note that I'm working on this and that I got the current result in features/async-validations. Most stuff is working but there are some minor breaking specs.

Greetings,
Sascha

@sdepold
Copy link
Copy Markdown
Member

sdepold commented May 30, 2013

PR merge done. still need to merge it into the milestones/2.0.0 branch

@sdepold
Copy link
Copy Markdown
Member

sdepold commented Jun 5, 2013

merge into the branch. thanks a lot!

@sdepold sdepold closed this Jun 5, 2013
@thanpolas
Copy link
Copy Markdown
Contributor

is it a requirement to decouple validation from saving?

From what i gather, the goal here was to make validators asynchronous...

Can't validation be piped back to saving so it doesn't break anything?

A separate method .validate() can still be exposed, should one want to call it separately. And we can add a new option for .save() which will skip validations so they won't run twice if thats the case?

Can we afford to rethink this?

// @sdepold @interlock @mickhansen @janmeier

@interlock
Copy link
Copy Markdown
Contributor Author

@thanpolas parts of that flow have been added in 1.7.0 but not upstreamed. Only exception to that would be it has no toggle for skipping validation.

It appears there has been a fairly large refactor of validation in the 1.7 series in general, so upstreaming those changes may not work. We are actually phasing out Sequelizer in our project in favour of another ORM right now, so not sure if we can dedicate time to work out an upstream merge for those validation updates.

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