Re: [Rails-core] A concern about rails validators, breaking MVC in complex projects

[Matt Jones]

On Feb 22, 2013, at 3:53 AM, dosad...@gmail.com wrote:
>
> https://gist.github.com/bbozo/5006180
>
> This would allow us to handle highly specific controller-introduced validators inside the controller and without adding unnecessary bloat in other areas (usually in unit test factories and with_options :if => some_roundabout_way_of_detecting_which_controller_youre_on block spam in the model).

What's wrong with something like this:

class SomeModel
attr_accessor :validate_the_thingy

validate :make_one_particular_and_unique_thingy_go_red, :if => :validate_the_thingy
end

in the controller:

@some_model = SomeModel.some_factory_method(params)
@some_model.validate_the_thingy = true


In both cases, the validation method is sitting around on the model.

I'd argue that this "breaks MVC" *less* than the solution in your Gist, since this keeps the flag separate from its implementation, rather than having the controller poke extra validations (by method name) directly into the model instance.

If you've *really* got bunches of wildly divergent validation setups, I'd agree with the suggestion on the ticket to actually create a (plain Ruby) mediator object and use the ActiveModel APIs.

--Matt Jones

[Jonathan Lozinski]

If you're unit testing, just create a user bypassing validation:

User.create! :validate => false

That is if you even need a user object at all rather than a stub

To me validations are just fine in models (as in specifying which apply) along with relationships to other entities.  That's pretty much all I want in them though

Sent from my iPhone

On 23 Feb 2013, at 03:55, "dosad...@gmail.com" <dosad...@gmail.com> wrote:

Hi all :)

Brought this issue to you from here: https://github.com/rails/rails/issues/9362

Problem is, that rails validations as they are now have a dual nature, they are something that:

1. helps keep the basic model data integrity (model concept, validate uniqueness of email on user for purpose of password reset, for example)
   
2. is instrumental in making a particular thingy in a particular controller go red if it isn't clicked (controller / view concept, password_confirmation matches password, for example)

and this creates problems.

For example, imagine a hypothetical case where a user may enter his password either through password/password_confirmation or through submitting the salt & hash.

The crypted password and salt fields get presence validators on them because it makes no sense not to have them, this is a model thing, it's also a user input on another form, yet more reason to handle it as a validator and not as a not null constraint or something equally restrictive.

The password (and especially) password_confirmation fields only make sense on the one form that handles user input through them (and consequently acceptance test of that form), but they force you to jump through hoops in the unit tests and in the form that doesn't need those two virtual fields to create a new user record.

This is just a simple and silly example, things get a lot more twisted when porting legacy apps where you need to maintain exact frontend behaviour as well as vaguely similar db structure so mapping can be done successfully.

~~~~~~~~~~~~~~~

Tl;dr bottom line is that validators are model concepts used to do stuff in view/controller.

While this is conceptually a pretty clear MVC violation, it works great for simple CRUD and even more complex cases, as long as the (controller) input attributes are consistent, I don't think this should change, it's pretty KISS for most purposes.

However, when controller input attributes aren't consistent then usual smells of breaking MVC start emerging: from increasing complexity and variety of factories in unit tests to increasing "validates :if => ...." and factory-for-controller method bloat in the model.

One solution would be to implement a callback injection mechanism that can be applied to the instance and not to the class, for example:

https://gist.github.com/bbozo/5006180

This would allow us to handle highly specific controller-introduced validators inside the controller and without adding unnecessary bloat in other areas (usually in unit test factories and with_options :if => some_roundabout_way_of_detecting_which_controller_youre_on block spam in the model). 

Another possibly more complicated solution would be to start working in direction of making validate* methods available on the instance level, not sure what would be a KISS way of doing this though.

Thanks for listening :) Thoughts?

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Nick Sutterer]

Borna- the problem is that UI forms and model API validations are two different layers. Rails mixes that, again. So basically you need two different validators. There are some gems that provide you with a form object that does the UI stuff like password_confirmation without interfering with the model's validators.

[Borna Novak]

Hi all,

thanks for the input :)

Imagine having a Transaction model with a double digit number of input controllers where most of those are tied to this or that payment API with slightly diverging additional fields (one asks for pan, other for pan and cvv, the third one for IBAN only, forth for...),

1. the PORO form approach with included validators means having dozens of additional form subclasses implemented so you could handle these little differences which mostly come down to one or two extra attributes per form, it also means having to check out the model, controller, form class and view if there's an error with the attribute list
2. the with_options :if => should_validate_attribute_x? approach is equivalent to the gist approach, but less invasive and doesn't require practically reimplementing elements of the validation stack, thank you Matt, very nice thought
   

However, the form class approach seems The Way to go in general - it definitely feels cleaner, I'm just not 100% bought its the way to go in this particular case, too many special snowflakes. Also, the form class approach seems both dispersed and repetitive in the attribute list mantra department.

At the moment for any given controller action you list the attributes in the form builder, then vast majority of them get listed in the strong_params permit(*args) method, then they get repeated in the delegator/accessor dance + model integration boilerplate in the form class and a part of all of this is already implemented in the model, all of it is quite monotonous, verbose and prone to copy/paste/type errors - and all of this to plug one additional validator in,

while it is cleaner then shoehorning validators onto the instance and watching them interact with model validators with funny side effects in views ("oops you missed 20 fields" on a 5-field form, for example), it does feel kind of fishy.

There's a definite smell about it, it feels un-rails-ish and I can't shake away the thought that some of these things can be unified / dryed up with better organization of the way model, controller and view interact, perhaps the most obvious point would be stronger coupling between the form builder class (from the view) and the form class (from the controller) - after all,

1. anything that's in the form must end up in the params hash received by the form class
   
2. and all of the attributes in the params hash need to have some kind of setter counterpart in the form class,
   
3. also certain elements of the controller environment would arguably end up in the form class (strong parameter protected things like current user, for example)

Thoughts?

@Nick, you mentioned that there are gems that tackle this issue?

@Mrloz, "validate: false" has an unwanted side-effect of skipping *_validate hooks which often end up holding generator methods, this is an antipattern though, one of the ways people go around the "validator in form + multiple form inputs" issue while blindly sticking to the "thin controller, oh boy is that one FAT model" principle

Thanks,

Borna

[Nick Sutterer]

I'd go with a separate form object but that's just "me" ;-) My friend Evan Light just recently pointed me to https://github.com/ClearFit/redtape

[Borna Novak]

You're right,

remaining coupled to the model, even in this case, brings more trouble/smell then it's worth. Redtape is an interesting take on the problem, but at least at first glance doesn't feel like a step in the right direction: model coupling is still strong, it doesn't seem to resolve the validator duality (and view-model coupling it generates), and I'm not a fan of "mapping frameworks".

The form class solution should be something that responds to new, valid, save and errors. The input attributes of the form object initializer would ideally have some sound default mapping to the underlying model / models, both in terms of getters/setters and in terms of delegating validators and the expected result of the form class would a valid (set of) model instance(s).

For example,

if Model is a model class and Form is a straight&simple form class of that model and Form validates attributes a and b, and Model validates attributes b and c, then a Form instance should be valid if and only if both a in Form and b in Model and Form are valid as well as the Model instance itself (this would happen if Form instance doesn't set up Model instance correctly).

Arguably, it would have access to the controller context, deal directly with the params hash and handle attribute filtering, hopefully making that whole attribute accounting thing a bit dryer, there's quite a bit of repetitive coding going on atm in that area.

Something in these lines looks like the way to go in the long run, at least for me (if anything meaningful comes up I'll gemify it). Anyway, this looks like something missing out from rails core, or is at least missing from the general rails ecosystem atm.


Borna

[Nick Sutterer]

* Parameter filtering is a controller issue. Rails-core proposes strong_parameters and the like, I personally prefer a representer. However, what comes out in the end is a parameter hashed passed to the form object.
* Validating the form by delegating to various models should happen in a separate form object. You're absolutely right when you say Rails is missing something here.

[Borna Novak]

I made yesterday a small-ish abstract class, which got left in the office -_-, its working something in the lines of https://gist.github.com/bbozo/5036937

Ofc, one can integrate this with form_for to get appropriate helper behavior for read only fields and whatnot

As for the second point:

* Parameter filtering is a controller issue. Rails-core proposes strong_parameters and the like, I personally prefer a representer. However, what comes out in the end is a parameter hashed passed to the form object.

As far as I understood, the strong parameters were implemented to stop people from submitting attributes that were not intended to be changed through this or that particular form on this or that particular controller action, if there is such a thing as a form class then it can just as well be within its domain to explicitly define its input/output which would reduce the attribute list repetition and resolve the issue that strong_parameters want to fix, like in the gist.

Any ideas why any of this could be a bad idea?


Borna