Best practice for monkeying with params?

[tovodeverett]

After a lot of trial and error, web searching, and generally stumbling around in the dark, I came up with the following approach for specifying the contents of h.params when testing Draper Decorators with RSpec. This assumes that one is using something like let(:params) { {some_param: 'some_value'} } to set up the desired params contents.

before(:each) do
  Draper::ViewContext.current.params.merge!(params)
end

after(:each) do
  Draper::ViewContext.current_controller = nil
  Draper::ViewContext.current = nil
end

I figured out the first part pretty quickly, but it took me a long time to realize that my tests were poisoning each other. So many things get automagically torn down when done from a before(:each) block that I made the mistake of assuming that would happen here. Once I realized that no cleanup was happening, I figured out the cleanest way to clean stuff up would be to wipe out both current_controller and current so that they could be repopulated automatically by Draper.

Perhaps this is just Draper's way of telling me I really shouldn't be using params in my decorators, but #184 seems to imply that I'm not the only one doing this. If that's the case, shouldn't there be a less obscure way to test that modifying params has the desired effect on the decorator?

[kristianmandrup]

IMO it is a clear anti-pattern to send the full params hash to a decorator. Much like sending the full current context to a method or object and say, "hell, you figure out what to do with this mess!!".

A better much pattern is to send it only those arguments it needs for its functionality. This is in line with the Command pattern and many others. Unfortunately Rails has a history of a very "dirty" approach to such matters, which eventually always leads to a lot of grief and misery whenever a project scales up or new team members come aboard and start "hacking". I believe the time has come to start cleaning up and try to avoid such anti-patterns in the future. Strong params is just one example of this. Thick bloated models was always a crazy idea!

[steveklabnik]

I also agree that sending the full params is bad. The point of a controller is to coordinate between components, so I wouldn't think it's a good idea. That said, if we don't do something, people will keep running into things.

Maybe we should just #dup the arguments so this doesn't happen?

[tovodeverett]

I have to say, I agree with Kristian, but I'm bouncing ideas around inside my skull trying to figure out a good solution. While it makes it more explicit to have the View passing parameters to the Decorator instead of the Decorator sneaking around and snagging them out of the view context, it still feels wrong for the View to be snagging them out of the view context.

It seems like the preferred approach is that the Controller goes and gathers everything necessary, sets up an instance variable, and then hands that single instance variable to the View whereupon the View has everything it needs to render things.

Also, I've been using the decorates_before_rendering gem so that my Controller has to be explicit if it wants to talk to the decorator, but I don't have to explicitly decorate the instance variable before rendering. With that in mind, I'm thinking of using an approach like this in my controller:

@article = Article.find(params[:id])
@article.decorate.params = params.slice(:foo, :bar)
@article.decorate.current_user = current_user

And then in the Decorator, the various "helpers" have access to the limited context information set up in the Controller. This way the View doesn't have to snoop around in the Controller trying to snag things and pass them to the Decorator - in effect, the Controller is "decorating the decorator". It hands over a Decorator to the View that is guaranteed to have everything it needs for the View to do its job properly. The View's job is overall layout, and the Decorator's job is all the fancy rendering needed for that layout. It's not the View's job to give the Decorator all the information - that information is supposed to be provided by the Controller.

All I have to do then is define the right set of attributes in ArticleDecorator. If I'm really aggressive, I'd even mark the setter as public but the getter as private (since only the Decorator should need to access those passed parameters).

Of course, this approach might have issues with collections of Decorators. Perhaps there should be a way to set some sort of context information on a Draper::DecoratedEnumerableProxy that would automatically be accessible to everything underneath it. Furthermore, if I've got a decorates_association scenario, I need that context to get passed along to the child objects as well. For instance, in my world current_user is really important - that's what has the role information for declarative_authorization. Of course, tt doesn't help that declarative_authorization doesn't know about the Decorator, and so it's still going to be hunting for it in the Controller. Hmmm - one more thing to monkeypatch!

Thoughts?

[steveklabnik]

It seems like the preferred approach is that the Controller goes and gathers everything necessary, sets up an instance variable, and then hands that single instance variable to the View whereupon the View has everything it needs to render things.

This is a direction I'd like Draper to move in eventually. https://gist.github.com/3886837 and https://gist.github.com/3856921

I don't like mutator (setter) methods personally. Constructors should create a valid object.

[kristianmandrup]

I added some comments to the gist. Perhaps a project like https://github.com/solnic/virtus could be useful for managing/validating attributes ;) Seen it used it other contexts and projects. I think it is a bit dangerous to take the approach of having a whole, complex Decorator tree the way @tovodeverett sketched it out, but I understand the need to do something like that. However for this scenario, I think something like Cells is better, where each "widget" gets its own mini controller and view. The widget controller then extracts the information needed for its view, then renders the view. You can even setup communication (data exchange) between the widgets... To me, a Draper decorator is mainly to avoid partials and vastly simply the views for each widget and make them more reusable and testable and less "dirty".

[haines]

@tovodeverett:

It seems like the preferred approach is that the Controller goes and gathers everything necessary, sets up an instance variable

Agreed. You can pass your sliced params in to the decorator constructor (or, equivalently, Model#decorate), which accepts an options hash.

@article = Article.find(params[:id]).decorate(params.slice(:foo, :bar))

@article.options # => {foo: ..., bar: ...}

I don't think the way you use those setters will work because Model#decorate is no longer memoized in 1.0, you need to set your ivar to the decorated object if you want to talk to the decorator.

Perhaps there should be a way to set some sort of context information on a Draper::DecoratedEnumerableProxy that would automatically be accessible to everything underneath it

Indeed, there is. CollectionDecorators, as they are now called, also have this options hash, which gets passed to each individual decorator they contain.

[tovodeverett]

I can't sleep (2:40 AM here), so I might as well write instead of thinking about writing.

One thing I've been thinking about ever since I decided to throw in my hat with Rails is elevating fields (i.e. attributes) to being full objects.

A decade ago, I developed a web framework for ASP using Perl. It was incredibly inefficient, both due to run-time performance issues and due to the need to compile the whole stack for every request (although I spent a lot of time developing lazy-loading and lazy-instantiation techniques to deal with this). There was very little MVC separation, but there was a lot of MVC-like thinking, and one of the core ideas was that each field was a class/object. For performance reasons, attribute storage was done in a hash, but all access to that hash was mediated through a field object. The field objects were implemented using Class::Prototyped, an implementation of Self semantics in Perl that Ned Konz and I co-developed, so each field could override methods. My initial version of my framework used Class::SelfMethods, which was sort of like Ruby's singleton methods, and when Ned came up with his first version of C::P he wrote to me and over a very feverish week the two of us rewrote the whole thing a dozen times. Later I added some really funky stuff to C::P in the form of slot attributes - think of them as decorations for methods.

Each field had a full set of methods that all interacted to handle things like rendering (in a whole bunch of contexts), normalizing user input, validation, retrieving from params, security (who can view this attribute, who can edit it), etc. Of course, there was a whole class hierarchy of different field object classes along with some custom ones like:

• Latitude/Longitudes - it handled displaying in different formats, and could parse a whole range of text formats from the user, including decimal degrees, degrees + decimal seconds, etc.
• Unitized values - it stored things in Metric, but would display in either Metric or US units, handled significant figures, would accept values in either format if the user appended the unit (and would default to whatever the user's desired format was), etc.
• Meta-fields - these wouldn't be defined in the database, but could aggregate data from other fields for display and/or editing, and could inform the query engine regarding what fields they depended on, so if you requested one in a query display the underlying engine knew what to put in the SELECT clause.

I had a world where I would define a view layout once for both #show, #edit, and #new and it consisted of a lot of things like @article.label_and_render(:attribute_name). That call was responsible for doing the right thing - when viewing a record, it would render it appropriately (including whatever HTML formatting was desired), when editing or creating a record it would render the right input element (unless the field was read-only for that user, in which case it would fall back to the view data), etc. I could create tabular output from a group of records by simply providing a list of columns, sort information, etc. and the system would render it, both in view and edit mode.

Rails has all these "helpers", both the ones we define and the ones Rails provides. But the helpers are just a bunch of procedural code to help us out and they don't really seem to address the issue. Draper helps clean up the ones we define, but we're still relying on the procedural ones Rails provides. Where is the MVC for each field?

Anyway, I'm wondering if implementing something like that either as part of or on top of Draper might make sense. I've looked at Cells, but I'm thinking it may be a bit too much if I try to define a Cell for every field in my system! The vast majority of the customized logic when using my framework was related to the View and Controller (although I didn't know them as such at the time), and from that perspective it seems like Draper might be a viable place to implement that concept.

[tovodeverett]

@haines: Once I get some more sleep, I'll get to work on cutting over my code to using the beta version of Draper - I'm realizing that 0.18.0 is really just the first sketch and that it makes a lot more sense to be on the github version.

I was wondering last night about the memoization of #decorate. I realized that memoization is a bad idea if the Decorator has state, and came to the conclusion that the only viable solution is to remove memoization (which has already been done, if I just switch to the github version). I think the approach I'm leaning towards is:

article = Article.find(params[:id])
@article = article.decorate(foo: 1, bar: 2, . . .)
article.model_method
@article.decorator_method

But I'm still debating. That lets my Controller use the local variable when talking to the Model and the instance variable when talking to the Decorator. Or maybe I should use:

@article = Article.find(params[:id]).decorate(foo: 1, bar: 2, . . .)
@article.source.model_method
@article.decorator_method

The first is perhaps a bit confusing, but once you know the pattern it's succinct. The latter is a bit more explicit, but it's more verbose. I need to think about Controllers a bit - I'm debating whether they should primarily interact with the Decorator or the Model.

Does the options hash get passed to association decorators?

For what it's worth, my current use of Rails is for an internal photo archiving/sharing site. I have 27 GB of family photos and videos from the past decade along with an indexing/display system running off my home server, and I'm rewriting the front-end in Rails (it's currently a bunch of ugly Perl code reading from YAML files that slowly grew over the past decade). So I'm perfectly happy using beta code in it. Once I've got that all finished and used it as a learning experience, I have a startup to go implement!

[kristianmandrup]

"may be a bit too much if I try to define a Cell for every field in my system!"

Hmm, this has nothing to do with Cells. Not what it is used for at all.

I would suggest you look at this: http://apotomo.de/ and especially this example here: http://apotomo.de/peters-guide.html

Then imagine taking the display view code from Peters guide and using Draper for the heavy lifting there!
Now you have a clean, flexible OO design done right IMO.

I would also look at Focused Controller to better encapsulate your controller actions ;)

[steveklabnik]

Regardless, I think this issue can be settled: Draper isn't responsible for what kind of craziness you do with messing around in your Rails environment. Since we allow you to set options for your decorator when you create it, that's how this should be done.

[tovodeverett]

I'm going to propose an idea for "encouraging" good behavior. Part of the problem is that helpers have access to #controller and the #params hash, so users (like me) may have written helpers that just directly access params. When we port those to Draper, we probably don't rethink everything - we're just happy enough to be getting namespaces for our helpers. It's only when we go to test that things get nasty, but because Draper isn't preventing us from calling h.params, we figure it's a testing problem, not a design smell.

Perhaps there should be a user configurable option to set h as either strict or permissive. Permissive would match the current behavior - h gets access to every helper. Strict would restrict h to "procedural" helpers - methods like h.content_tag and so forth whose output is solely dependent upon the passed parameters. There would be no access to helpers that rely on context or state - methods like h.controller, h.params, etc. Attempts to access the latter would result in useful debug messages that would direct the user to the appropriate section of the Readme for education on best practices for passing state to the Decorator during instantiation in the controller.

The Permissive mode would model the current behavior - one gets access to everything from h. My suggestion would be that Strict mode be the default, but that the documentation explain how to enable Permissive mode (along with lots of warnings about how it's a bad idea and how difficult it makes testing).

[steveklabnik]

Hmmm. Okay, I'll re-open this for discussion...

[kristianmandrup]

Why would you want helper modules in the first place? Using helpers has always been a bad design idea. I think a lot or developers have noted this... much better to use real "helper" objects. You can find tons of critiques of Rails misuse of helpers and how it leads to really ugly design, bloated namespaces and violating Single Responsibility and such. Just my thoughts.

[steveklabnik]

The whole point of Draper is to replace helpers, @kristianmandrup ;) That said, Rails' built-in helpers are very good. Like link_to. That's what accessing them is for.

[tovodeverett]

Unfortunately, there's a huge single, polluted, evil namespace that mixes:

• Rails built-in helpers for generating HTML (link_to, content_tag, safe_join, etc.) that know about html_safe? and that may be useful for safely generating syntactically correct HTML.
• Rails built-in helpers for accessing params, controller, etc.
• User defined helpers, which in turn fall into two categories
  • Things that look like the first group (i.e. generic code snippets for generating HTML from passed parameters).
  • Things that should be moved into a Decorator (i.e. code specific to a model for rendering specific fields).
• Helpers that come from other libraries (i.e. Formtastic, which I'm considering investigating)

Things in the first category are very useful, and preventing users from using them is likely to result in hand-coded HTML that is not safe. Things in the second category are dangerous. Things in the other two categories are probably OK - users will move methods into Decorators as they convert their codebases, and I see no reason to prevent users from trying to mix Draper and things like Formtastic.

There could even be a three-way switch on the helpers in the second category - Strict, Warn, and Permissive.

[kristianmandrup]

:) so true.

Perhaps contextualizing the "helpers" via contextualizecould help solve/address this.

https://github.com/kristianmandrup/contextualize

There really is a need to only include certain modules/functionality in certain contexts, not to pollute the namespace just in case it is needed in some context.

This is the main problem as I see it. But categorising all this mess into some general categories could be a first fix I guess.

[steveklabnik]

I'm thinking that this is going to add a lot of complication for not a huge amount of benefit. I'd be willing to check out a PR for this, but I'm gonna give it a close otherwise.

[tovodeverett]

If and when I decide I feel strongly enough about this, I'll go work on it and submit a pull request. But more than likely I'll come to the same conclusion you just did!