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

Model instantiation fails silently when invalid attributes are passed to the constructor (0.9.9.) #1961

Closed
jevakallio opened this issue Dec 17, 2012 · 32 comments

Comments

@jevakallio
Copy link

@jevakallio jevakallio commented Dec 17, 2012

I'm not clear how I'm supposed to catch validation errors when initializing a model with attributes, as in:

var model = new Model({foo:'bar'});

I've studied the v0.9.9. source code, and I cannot figure out how to catch the validation error in these cases. The instantiation fails silently. See jsFiddle for an example of the problem.

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Dec 17, 2012

@jashkenas has recently said that the idea behind the validate method is to ensure a model never exists in an invalid state. The beauty of the validate method though is that you determine what exactly an invalid state is. In your case, you're probably better off running Model.prototype.validate(attrs); before even creating a new invalid model or var model = new Model(); model.set(attrs, {error: ...});

@dperrymorrow
Copy link

@dperrymorrow dperrymorrow commented Dec 17, 2012

the documentation says that validate is called on set, and save.
could you just define the properties in a set call after instantiation and error handling setup?

    var ValidationTestModel = Backbone.Model.extend({
      validate: function(attrs) {
        if (attrs.foo !== 'foo')
           return 'foo should be foo';
      }
    });

    var instance = new ValidationTestModel();
    instance.on('error', function(model, error) {
        console.log(error);
    });                       
    instance.set({ foo: 'not bar' });      
@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

@caseywebdev, @dperrymorrow, thanks for the replies, but I don't think either are reasonable solutions for the following reasons:

  1. The current implementation actually allows me to create a model in an invalid state, if I define an invalid state as missing a required field (which must be one of the most common validation scenarios).
  2. Having to directly call a prototype method before every instatiation smells. Besides I can't get a correct this context value to the validate-method, if I don't have an instance to call it on. Also a smell.
  3. Having to create the model without attributes and setting attributes separately doesn't only allow me to create a model in invalid state, it forces me to do so (again, if I define a validation rule which states that a model must have foo defined.)
  4. Backbone supports constructor initialization explicitly. It should not require a workaround.

The new, strict handling of validation is a breaking change in many use cases as it is. In our codebase models are created with attributes more often than not. The more I think about it, the clearer it seems to me: for constructor initialization to fail silently without having any reasonable way of handling it isn't just a usage problem, it's a bug.

@dperrymorrow
Copy link

@dperrymorrow dperrymorrow commented Dec 18, 2012

so you're saying that if you define validation rules, you feel that the following should happen?

var ValidationTestModel = Backbone.Model.extend({
    validate: function(attrs) {
      if (attrs.foo !== 'foo') return 'foo should be foo';
    }
 });

 var instance = new ValidationTestModel({foo: 'bar');
 console.log(instance);
// => undefined

if everyone is in agreement then i could do a pull request on this.

@tgriesser
Copy link
Collaborator

@tgriesser tgriesser commented Dec 18, 2012

Just added a proposed fix in #1963 - an error function on the constructor's options should be called to handle the error in this case.

@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

@dperrymorrow - It's a good question what should happen. I'm not sure if it's even possible to return undefined from a constructor when using new. Consider:

function Foo() {
    return undefined;    
}                    
Foo.prototype.protoProperty = 'bar';

var isntance = new Foo();
instance.protoProperty // -> bar

If the instantiation must fail, it should be by throwing an Error. For my use case that might be appropriate, but it would prevent creating models with partial attributes, which I'm sure is a common use case by itself.

IMO the changes to validation handling in 0.9.9 should be rolled back, especially the validation at constructor level, and the removal of silent:true in Model.set.

@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

@tgriesser, thanks. Your solution would fix the immediate issue of catching the error.

It however doesn't address the inconsistency of the whole validation scheme, which I've outlined in my previous comment. It's still entirely possible to create invalid models, if there are required fields whose presence is validated.

@andornaut
Copy link

@andornaut andornaut commented Dec 18, 2012

I agree with @fencliff and @dperrymorrow, Validation at construction time breaks many common and valid use cases.

Often models are constructed with incomplete data, then data is added, then the model is saved. The only automatic / enforced validation should occur just prior to save.

This change breaks my apps, and I imagine that it would break many others.

@tbranyen
Copy link
Collaborator

@tbranyen tbranyen commented Dec 18, 2012

I think the common use case is when you have minimal validation and then do something like:

var user = new UserModel({ id: 5 }); // Is now in an invalid state.
user.fetch();
@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

@tbranyen: Yes, that is a great example of a common pattern that is now broken in 0.9.9. Fleshing it out:

var UserModel = Backbone.Model.extend({
    validate: function(attrs) {
        if(!attrs.username) return 'username required';
    }
});

var user = new UserModel( { id: 5 }); // -> id is never set because username is missing   
user.fetch()                          // -> fails because user.id is undefined
@tgriesser
Copy link
Collaborator

@tgriesser tgriesser commented Dec 18, 2012

Yep I do the same thing @tbranyen mentioned a lot, guess I just don't have validations there for this to be an issue yet - great point.

@Yahasana
Copy link

@Yahasana Yahasana commented Dec 18, 2012

Relative #1930

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Dec 18, 2012

I use this pattern, but I only validate values that exists...

...
validate: function (attrs) {
  if (attrs.username != null) {
    // validate
  }
}
...
var  user = new User({id: 1});
user.fetch();
// all good

When I actually want to validate the username against user input, I ensure at least an empty string is passed in so the validation occurs. This is easy with forms because the default $('input').val() is simply ''. So user.set({username: $('input.username').val()}); will run the validation but user.set({username: null}); will not. It works for me.

@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

I'm not sure what concrete issues were being solved by the validation changes in 0.9.9, but to me it seems the stricter validation rules are causing more problems than they are solving. They limit the use cases of Model.validate in general, and make it difficult to upgrade from 0.9.2 in particular.

I'm aware of @jashkenas 's position in #1930, but I would ask you to reconsider these changes.

Because I'm not aware of the reasoning behind the original change, I don't have a great suggestion on how to solve the same less obtrusively.

@tgriesser
Copy link
Collaborator

@tgriesser tgriesser commented Dec 18, 2012

@fencliff I think they stemmed from #1477

@andornaut
Copy link

@andornaut andornaut commented Dec 18, 2012

Well, this is a significant change to the meaning of "valid". It now means "state that a model can never be in"; whereas previously it referred to state that could not be persisted.

I would guess that most people would assume / expect the previous meaning.

For me, at least, the new validate is much less useful.

@dperrymorrow
Copy link

@dperrymorrow dperrymorrow commented Dec 18, 2012

i agree with @andornaut
consider the following in Ruby on Rails
pretend that class person validates to make sure that name is not blank.

person = Person.new({:name => ""})
person.save
# => false
person.isValid?
# => false

you can create an object that is invalid, but just as @andornaut said, you cant persist it.

@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

I think the behavior of 0.9.2 where Model.set accepted options.silent (and the constructor used that option) was a good way of handling this issue.

If we could return to the previous implementation, Issue #1477 could be solved easily (although not very elegantly) with an additional set option validate:true, which would "override" silent:true. To be used internally, as follows:

silent:false && validate:false // -> validate and send events (default use case)
silent:false && validate:true  // -> validate and send events (default use case, validate:true has no effect)
silent:true && validate:true   // -> validate, don't send events (for reset)
silent:true && validate:false  // -> don't validate, don't send events (for constructor)

In effect this would lead to an API and behavior which are compliant to both 0.9.2. and #1477 and enable versatility for the usage of validate.

I do admit it is ugly.

@wyuenho
Copy link
Contributor

@wyuenho wyuenho commented Dec 18, 2012

@fencliff I'm curious, does your model and validation allow just new Model()?

@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

@wyuenho Currently it does allow new Model(), because I'm still using 0.9.2, which doesn't trigger validation in the constructor. But on many models the validation would fail, if triggered.

This is because, as @andornaut put it so well, I've made the assumption that validation should prevent me from persisting the model, not creating it.

@wyuenho
Copy link
Contributor

@wyuenho wyuenho commented Dec 18, 2012

I have a slightly different interpretation of what validate() should do. Client-side validation to me is completely different from server-side validation. Server-side validation typically validates data as a whole and disallows persistence if the data as a whole isn't valid.

Client-side validation is different because you typically only validate partial data as your user fills up a form. A form that fills in 1 field correctly should be valid on the client-side so the user can proceed to the next field, but invalid on the server-side because the data as a whole is incomplete.

So I guess in your case you are actually doing server-side validation on the client-side am I right?

When I do validation on the client-side I typically do what @caseywebdev does but I find it quite tedious for longer forms so these days I don't even use validate() anymore and just do server-side validation, but I'm just lazy.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Dec 18, 2012

If we (once again) changed the validate semantics to have nothing to do with set, and only cause an error when you tried to call save ... would that be something that folks would generally approve of?

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Dec 18, 2012

I definitely use validations on set, not just save. Make it optional in set (default to true) and use {validate: false} as the default in the constructor?

@philfreo
Copy link
Contributor

@philfreo philfreo commented Dec 18, 2012

If we (once again) changed the validate semantics to have nothing to do with set, and only cause an error when you tried to call save ... would that be something that folks would generally approve of?

I don't necessarily know which is better, but I'd be able to use backbone validation more often (and revert some of my app changes I had to make for 0.9.9) if this was the case

@andornaut
Copy link

@andornaut andornaut commented Dec 18, 2012

@jashkenas Yes, definitely.

@caseywebdev One issue is that set is used in the constructor and IMO, validation should not run inside a constructor (for several reasons, enumerated above). So, this would require a 0.9.2-like workaround for object construction.

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Dec 18, 2012

@andornaut That's why I said use this.set(attrs, _.extend({validate: false}, options)); in the constructor.

@andornaut
Copy link

@andornaut andornaut commented Dec 18, 2012

@caseywebdev Sounds good to me.

@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

@jashkenas: I would very much approve. To support @caseywebdev's (and presumably many others') use case the validation on set should be configurable and default to true to support backwards compatibility.

In addition to passing validate:true|false to set, it would be great if the default would be configurable on the Model prototype itself, something like:

//disable globally
Backbone.Model.prototype.validateSet = false;

//disable for specific model
var FooModel = Backbone.Model.extend({
    validateSet: false
});

This would make the consumer code much more terse and less kludgey in an application where we know that Models or a Model should never be validated on set.

@wyuenho
Copy link
Contributor

@wyuenho wyuenho commented Dec 18, 2012

@jashkenas I think only calling validate() during save would definitely be a lot less confusing and fits in with what people typically think validation should do. It's more consistent too because now you won't have a case where your data is valid everywhere, except during bootstrapping. Now you have a case where your data can be invalid at any time, but will only persist if valid.

But people do seem to use validation during set a lot too, I don't validate during set much so no preference on that.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Dec 18, 2012

If folks in this thread want to be really super helpful -- it would be great if y'all would take @tgriesser's patch over in #1972, try it in your apps, and let us know (over in #1972) if it works well for you.

@jevakallio
Copy link
Author

@jevakallio jevakallio commented Dec 18, 2012

Tested #1972, it seems to work and covers all the problems discussed in this issue. I also tested the previously closed #1477 for regressions, and it seems to work also when passing validate:true into reset.

Thanks again, all.

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Dec 19, 2012

Looks like we're all good here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants