Allow committing a record whilst in invalid state

[alexspeller]

If the server returns a 422 response with errors, the record enters
the invalid state. This means that it is impossible to save the record
again without setting all of the invalid properties at least once.

Sometimes this makes sense, but if the server is the arbiter of
validity then often it does not, especially if validity of properties
is interdependant or based on external conditions.

Example:

Server validates firstName + lastName must be at least 10 characters
combined.

Server returns {errors: {firstName: "too short", lastName; "too Short"}}

Now set firstName to be 20 characters long. The record is valid according
to the server, but calling record.save() results in an error:

Attempted to handle event `willCommit` on <DS.Model> while in state root.loaded.updated.invalid

This patch allows save() to be called on an invalid record, meaning
the server can decide if it's valid or not.

[alexspeller]

Related discussion

[axelguiloff]

This would be really helpful. Our team has run into issues with this numerous times that we've had to work around. As Alex mentioned, it seems like the API should ultimately be responsible for ensuring validity of data.

[alexspeller]

@tchak I think you may be interested in this?

[tchak]

@alexspeller I understand your point, but I do not like the idea of adding a new api to errors. Why not just call the regular clean an make the record valid ? This is how rails do it.

[alexspeller]

@tchak short answer: that doesn't work

Long answer: calling clear on the errors triggers becameValid, which transitions the record to the uncommitted state (it's currently in inFlight. If validation errors come back from the server it needs to stay in the invalid state. Even if somehow I could hack around this to make it work, I don't think it's semantically correct (i.e. it should transition directly from inFlight to invalid if there are validation errors).

Would you be happier with something like:

  clear: function(keepState) {
    if (get(this, 'isEmpty')) { return; }
    get(this, 'content').clear();
    if (!keepState) {
      this.trigger('becameValid');
    }
  }

[alexspeller]

@tchak I just changed it to that because I think it's a better api anyway - does this assuage your concerns?

[tchak]

@alexspeller I am not sure I get your point. Why the record is in inFlight state ? We are talking about a record with errors : invalid record in invalid state. I will try to look at the state chart later today to find out the case you talking about.

[alexspeller]

@tchak are you saying that in every application you should always do this in your code:

record.errors.clear();
record.save();

That seems like a bad api. If we're going by rails you can always call record.save on an ActiveRecord object without worrying about it, it will return the correct result even if the save was attempted before. I think that throwing an error about states when doing simple stuff on a record is bad from a developer use point - I'm pretty sure that @wycats mentioned in the past that if you get an error thrown about being in the wrong state from basic application code it should be considered a bug.

In short, I think this should "just work" and the impact is so minimal that I don't understand why you wouldn't want it to work for such a common use case. Any time you do validation, the UI is likely to make it easy to submit the record multiple times and this will be an edge case that you have to educate new developers about making the library harder to use.

[alexspeller]

@tchak here's where I read that, State errors that happen today are because of bugs in Ember Data itself.

So according to @wycats apparently record.save() throwing an error about states should be considered a bug.

[tchak]

@alexspeller I agree with you that .save() should always work.
I am just saying that I see no reason why we could not clear the state prior to save if record is invalid. It should "just work" without you even noticing it.

[tchak]

@alexspeller just tried the following change in model.js :

  adapterWillCommit: function() {
    if (!get(this, 'isValid')) {
      get(this, 'errors').clear();
    }
    this.send('willCommit');
  },

Your tests included in this PR passes... Am I missing something ?

[alexspeller]

@tchak It's more likely that I've missed something - I'm going to have another look

[tchak]

@alexspeller
tchak@950f8cf

[alexspeller]

@tchak yes that's much nicer, sorry I misunderstood what you were saying before.

Closing in favour of tchak/data@950f8cf

[tchak]

@alexspeller sorry for not being more clear :) and thank's a lot for pointing the problem.
I just came up with even better solution (I updated my commit)