Ember Data, ActiveModelAdapter, and Validations

Hi, I have a few thoughts on the server side validations support in ember data through ActiveModelAdapter and wanted to see if anyone else thought similarly. @bradleypriest I think you may be the best person to ask about this as you worked on the ActiveModelAdapter in the first place, although perhaps not.

Mainly, I don’t think that the invalid state works that well. I’m loving the errors object that is automatically populated, however when validation errors come back the model is put into an invalid state. This makes sense but if you then try to submit the record again, you get an error like Attempted to handle event willCommit on <DS.Model> while in state root.loaded.updated.invalid.

I don’t think this semantic is right, because if we are delegating the validation to the server who’s to say it’s not valid now? I.e. because of a change external to this client, or e.g. a time based validation (only valid to create a record after 9am and the time just changed). It seems like it would be much better to allow willCommit in the invalid state, so that we can go back to the server and actually check if the record is valid now.

If you make any property change to the record, then the invalid state is exited. This means you are allowed to submit the record again, however again I think this is not the best way of handling things because you are now assuming any change makes the record valid when only the server can really know if that is the case. The main issue this causes is that if you are displaying some kind of error messages list above a form (something like rails’ error_messages_for), this list will disappear as soon as you change a field on the record, which is a jarring UX IMO.

I think a better approach would be:

  1. Allow willCommit in invalid states
  2. Invalid state persists until the record is saved again, at which point the errors are cleared and invalid state is exited unless server responds with more validation errors

Another minor issue that I have, although I’m not sure of an easy proposal to solve this, is that saving records that may have validation errors in an action is a bit ugly, because you need a no-op function in you then error handler to avoid everything breaking, e.g.:

  actions: {
    submit: function() {
      record.save().then(null, function() { /* noop */});
    }
  }

Obivously, the promise has to reject if the save was unsuccessful otherwise the success handler will be called even if the record is invalid, however I find this noop pattern generally dissatisfying and would prefer if maybe action handlers rescued DS.InvalidError rather than causing the app to break if you forget the noop error handler? Not so sure about this one.

Happy to work up a pull request if consensus can be reached on anything raised here.

3 Likes

Hi @alexspeller, I haven’t really had much of a chance to look through the Errors stuff, @tchak was championing the errors object. Although I do agree it could do with a little loving.

@alexspeller How did you fully circumvent the root.loaded.updated.invalid case? Transitioning the record to another state manually is causing other weird behaviors later, eg. when the record got saved, but the error object is still present on the record.

How are you transitioning the record manually?

Like record.transitionTo('root.loaded.updated')

@xeppelin you probably shouldn’t do that, you should use record.send(‘becameValid’) instead

@alexspeller yeah, i know it’s not how it should be handled, but practically nothing points to the right direction :frowning:

@xeppelin I think this pull request will address your issue, would appreciate a test and comment on the PR if it does:

https://github.com/emberjs/data/pull/1876

@alexspeller sorry for the late response! I tried with @tchak’s PR and it works beautifully. Thank you both!