
Rails – The Missing Parts – Policies - nottombrown
http://eng.joingrouper.com/blog/2014/03/20/rails-the-missing-parts-policies/
======
mjbellantoni
If I can overly generalize, and as a person who generally loves Ruby and
Rails, I'll suggest that there are two schools of Rails app development
emerging.

The first is what I'll term the "Classic School" or "DHH School" characterized
by full-blown utilization of all Rails magic such as AR/AC callbacks, pretty
skinny but not obsessively skinny controllers, an eschewance of service
objects and more.

The "Emerging School" relies less on some aspects of Rails magic, and
introduces patterns like services/interactors/DCI and seems to be more focused
on building a domain models which attempt to be less coupled to the framework.

I think the Grouper folks are in the second camp given their previous blog
post about interactors which showed up here on HN. Also, I think the desire to
encapsulate authorization into a reusable object seems slightly more in the
spirit of the emerging school.

What I find odd is that they'd offer a solution for authorization which is so
bound up in Rails magic. It seems to me authorization is really part of the
domain model and as such you would expect to find that code in whatever gets
called by the controller (which I would expect to be something like an
interactor, but in this blog post is just plain old Rails code.)

~~~
jonathanwallace
Similar to [http://words.steveklabnik.com/rails-has-two-default-
stacks](http://words.steveklabnik.com/rails-has-two-default-stacks) ?

~~~
ritchiea
Actually I disagree in a big way. I think it's a mistake to conflate choices
in tooling like HAML/HTML & mySQL/pg with architecture choices.

Your architecture choices change the way you write your application code and
how you utilize the Rails framework. On the other hand HAML is not going to
change the way you write markup, it just provides a different syntax that some
people prefer. Postgres has features people like that mySQL lacks. MiniTest &
Rspec are different syntaxes for testing. These three choices have very little
to do with how you use Rails & write your app code, they just happen to be
made by the same people who advocate big architecture changes.

I fall in a camp where I use HAML, postgres & Rspec but I also use fat models
and skinny controllers. I have a few really small services in my apps but I
try to reserve them for cases when the standard Rails pattern is extremely
ugly rather than actively seeking out replacements for the Rails way.

------
dhh
Ensuring that the Ticket is valid is obviously a domain model concern. Policy
objects can be a fine idea when they're swappable and you need to allow for
multiple different policies. This is not one of those cases.

Here's a much simpler approach that keeps the validation logic in the domain
model and uses features that Rails has had since the dawn of the framework:
[https://gist.github.com/dhh/9672827](https://gist.github.com/dhh/9672827)

~~~
dhh
On a separate note, I feel like this article series might better be titled
"The Missing Parts of Our Knowledge of Basic Rails Features".

Reinventing basic features doesn't make your Rails deployment "advanced", it
just makes it convoluted. There's no bonus prize for introducing fancy
patterns to situations that do not call for them.

Further more, here's the definition of the Active Record pattern, as described
by Martin Fowler: "An object that wraps a row in a database table or view,
encapsulates the database access, and adds domain logic on that data". The key
part is that last sentence.

So a quote like what follows simply misunderstands the purpose the Active
Record pattern: "A recurring theme in these posts is that ActiveRecord models
should be very simple interfaces to a datastore – the User model should be
responsible for validating and persisting attributes of a user, like name and
date-of-birth, and not much else."

See the example diagram (which I actually drew for Martin back before even
starting Rails!):
[http://www.martinfowler.com/eaaCatalog/activeRecord.html](http://www.martinfowler.com/eaaCatalog/activeRecord.html)
\-- it includes domain logic methods like "getExemption", "isFlaggedForAudit",
and so forth. Exactly like the pattern describes.

You're not a beautiful and unique snowflake.

~~~
tomblomfield
Thanks for taking a look - I certainly respect your viewpoint.

I guess where we diverge is the _amount_ of domain logic which lives on an
ActiveRecord model.

My experience is relatively limited, but in 4 or so years of professional
Rails development, across many different codebases, the overwhelming majority
of problems have been caused by bloated models with too many responsibilities.

You clearly have more experience with Rails, but your solution always seems to
be "If you were smarter, you wouldn't have these problems with my framework".
I've seen this problem recur again and again across multiple teams &
codebases.

We're seeking to address that problem with these concepts that have been
successfully used in other frameworks & languages.

~~~
dhh
Yes, when stumbling across bad code, the first instinct should be: how can I
make this simpler. Not how can I wrap this bad code in more convoluted
patterns. Don't use a big word when a small one will do.

Additionally, I find that the key problem with bloated models stems from
missing domain models (often has-many-through models). Not from moving the
logic of the existing models into more noun classes. In your example here, the
purpose of the ticket is to tie a user to a grouper: that's exactly the place
to put logic that governs that connection!

Finally, what gets my goat is this notion that these patterns are necessitated
by "advanced deployments". As it was some law of nature that when your app
hits a certain size, you have to introduce this litany of misfit patterns.
That's like arguing that if your book is longer than 300 pages, it must also
use really big words and complex sentence structures. What?

The solution to large applications is to double down on simplicity, not give
up on it. It is even more important to get the basics right. Execute them
beautifully before even contemplating to freewheel from there.

~~~
nthj
> Additionally, I find that the key problem with bloated models stems from
> missing domain models (often has-many-through models). Not from moving the
> logic of the existing models into more noun classes.

When I first started learning Rails 4 years ago, I watched a video of a talk
you gave where you refactored a kludgy piece of code to use an additional
resource/model. The code unraveled before our eyes.

I still remember that as a Neo moment.

~~~
highpixels
Don't suppose you have a link to that video? Or remember any more details so I
could dig it up?

~~~
nthj
I think it was this one, but unfortunately I can't find the whole video.

[https://www.youtube.com/watch?v=mp4z2eK1Avw](https://www.youtube.com/watch?v=mp4z2eK1Avw)

[http://johannesbrodwall.com/2007/02/27/crud-rest-
rails/](http://johannesbrodwall.com/2007/02/27/crud-rest-rails/)

------
danso
Oh boy, is DHH going to jump into this too?

As an intermediate Rails developer...I don't understand why this if/else
forest:

    
    
        if user.blacklisted? # They've been banned for bad-behaviour
          fail! “You can't book a Grouper at this time”
        elsif grouper.full?
          fail! “This grouper is full, please pick another date”
        elsif grouper.past?
          fail! “This grouper has already occured!”
        elsif user.has_existing_grouper?(grouper)
          fail! “You’re already going on a Grouper that day”
        elsif ticket.confirmed?
          fail! “You’ve already confirmed this grouper”
        end
    

\-- can't simply be part of the Ticket object? The Ticket clearly has a
relation to User and Grouper and talking to those objects (i.e. `if
user.not_blacklisted? && user.not_booked(self.date)`) don't violate
Demeter...how is making a policy object cleaner than just using a Concern that
is included into Ticket? The controller then just needs to ask for
`ticket.confirmable?`

edit: OK, not ask for `ticket.confirmable?`, but rather, try to `save` the
ticket and the Ticket constructor/validators can pass on the errors to the
controller.

~~~
tomblomfield
I think ultimately it's a philosophical difference; the problem with very many
large Rails deployments is that the ActiveRecord models are the core of the
application, and DHH's conception of Rails seems to encourage this.

This tends to cause a lot of problems - it's hard to exercise a single model
in tests without having a very large number of associated objects present.
This also tends to require them to be in the database. This makes your tests
very slow, and your application very tightly coupled and hard to refactor.

If you rethink your Rails application and make models responsible for _only_
data validation & persistence, you tend to avoid these kinds of problems. You
then need somewhere else for the "core" of your application logic to reside.
We suggest these new concepts are Interactors, Policies (and Decorators - more
to come in a new blog post).

~~~
phillmv
It sounds like you're letting "ease of test maintenance" drive the
architecture of the rest of your app; I'm inclined to accept that
incentivizing test creation will lead to more confidence in the face of
changes but am somewhat unconvinced it's 1:1 with "less coupled, more
maintainable" a priori.

Case in point, I'm really uncomfortable with class names with verbs. Typing
CanConfirmTicketPolicy.new(ticket: ticket).allowed? smells bad to my fingers.

It strikes me that you haven't eliminated the coupling? because the
CanConfirmTicketPolicy still depends on different domain objects. Kind of by
definition, you can't remove it because it's explicitly operating on User,
Group and Ticket; the main difference to me seems that they're easier to mock?

I would argue that this ought to be either an explicit inter domain class that
models the interaction between Users, Group and Tickets - some appropriate
noun like Purchase or GroupActivity or whatever - that can then be solely
responsible and reused as appropriate.

You have a bit of text that explains your decisions,

>We could move the logic to a controller mixin, or define the method on the
ApplicationController, but it would still not be available in our view

Could you not just define a helper method, then? Helpers are available in both
views and controllers if memory serves, are mixed in by default, are equally
easy to test as your verbed policy object and have the (minor) added benefit
of not polluting your namespace while being equally easy to reason about -
it's unclear to me how can_confirm_ticket?(ticket) would necessarily be
inferior.

If you're really interested in putting these in models (which is also
acceptable) then a regular concern namespaced to your preferred model would
work just as well.

Am I missing something? I would like to understand your use case but I don't
seem to get it.

~~~
mnarayan01
> It sounds like you're letting "ease of test maintenance" drive the
> architecture of the rest of your app; I'm inclined to accept that
> incentivizing test creation will lead to more confidence in the face of
> changes but am somewhat unconvinced it's 1:1 with "less coupled, more
> maintainable" a priori.

I've thought of making a similar comment on _many_ other articles but never
have. This is actually the first time I've ever even _seen_ anyone else saying
it. I wonder if we're relatively unique or if many other people feel the same
way and simply (like myself) don't actually comment on it.

~~~
Jeff_Dickey
Difficulty of testing is a generally reliable indicator of a design that's in
deep pain. [ _Growing Object-Oriented Software, Guided by Tests_
]([http://www.growing-object-oriented-software.com](http://www.growing-object-
oriented-software.com)) is easily one of the two or three most influential
technical books I've read in my career — and the book's only five years old.

When I'm spelunking through some dank and dimly-lit Rails-model God Objects, I
often fantasise about how things would be different if certain influential
developers had read that book or one like it before setting out, and had taken
the lessons to heart. Then I swap the batteries in my headlamp, and continue
my descent.

------
swanson
How is a Policy different than an ActiveRecord::Validator? Could this
TicketPolicy not just be a validator extracted to it's own file (and tested on
it's own)?

[http://api.rubyonrails.org/classes/ActiveModel/Validator.htm...](http://api.rubyonrails.org/classes/ActiveModel/Validator.html)

~~~
karmajunkie
Policies (also known as strategies) aren't altogether different from
Validators, though they're a bit more flexible. The real difference is that
validation doesn't belong on your database interface, and its the chief reason
why AR can only be used for domain driven design in the most basic of
circumstances (NB: this isn't the same as saying that database constraints are
a bad thing.) Validators are hardcoded into models; in that regard, they're
only slightly more flexible than using validations directly on the model. Like
Rails' notion of concerns, they're really about code geography, not
architecture or design.

Policies, on the other hand, are far more flexible (assuming your application
interface supports them, or they're used to wrap a block, or some other
gatekeeping mechanism) because they can be swapped out. Imagine a circumstance
in which you want one set of validations for creation by a normal user, and
another set of validations when the creation of an instance occurs through a
privileged API; yet another set of rules for creation by an admin, and so
on...

You _could_ implement those as a series of validators with an :if option on
the validates call, and if you want to stick with Rails' canon, go right
ahead. But in my opinion (and having made this mistake before) you're simply
staving off an inevitable point at which your dependence on Rails has
hamstrung your ability to iterate code.

------
dpeck
related, Pundit provides some similar functionality in a very minimal package
[https://github.com/elabs/pundit](https://github.com/elabs/pundit)

~~~
planckscnst
We've been using Pundit with great success in our organization. We use it
along with a few other objects described below.

We have abilities, which is just a table full of strings like "modify users".
The policy (from Pundit) looks these up to find whether an actor (user,
typically) can do something to a model.

We have roles, which is just a user-definable collection of abilities, so you
might define an 'admin' role that has every permission, and a 'reporter' role
that has the ability to run reports and not much else.

We have permissions, which includes a role, a manager, and a manageable. The
manager is the actor, or the thing asking to do something to another object;
the managable is the object the actor is trying to act upon.

There is a "Managable" concern that gets included in any object that may be
guarded by policies, and a "Manager" concern that gets added to any object
that might be an actor. The Manager concern sets up the associations gives us
methods like "#has_ability_on(ability, managable)", and "#has_ability)", which
is useful for deciding whether to show gui widgets. These methods are how
Pundit looks up the abilities one (manager) object has on another (managable)
object.

This simple setup has allowed us to greatly simplify our application.

~~~
dpeck
Curious is you have some more details about this written up anywhere or some
code on github?

I'm just using Pundit in a "real" app for the first time where strict user
types won't be enough and I need to have a role based approach. I'm doing
something similar to what you have now, but a couple steps back in my design
process, and the managable concern sounds promising.

~~~
planckscnst
That would be a good idea for a writeup. I'll have to get that done. For now,
I have a couple more details.

We're using the closure_tree gem to give a hierarchy to the permissions; every
permission can have a parent, which may further limit which abilities are
allowed on a managable by a manager. This means that if user A has abilities
"1,2,3" on an object, when he grants a permission to user B, while he can
create the permission with whatever abilities he wants (maybe abilities
"2,3,4"), the parent of that user B's permission is user A's permission, which
restricts the real abilities user B has so that user B effectively has only
abilities "2,3" on that object. To do this, we get the whole ancestor chain
and take the intersection of all the abilities of each permission.

We also have organization objects; these are the owners of everything in the
system - they own the users, the roles, the permissions, etc. Organizations
may also be a manager or a managable; in fact, an organization automatically
gets a permission with itself as both the manger and the managable, with every
ability. We also use closure_tree with organizations: when an organization is
the managable part of a permission, that permission applies to everything the
organization owns, including all descendants.

So we have permissions with a hierarchy where we look upward at its ancestors
to limit its abilities, and organizations with a hierarchy where we look down
to find the scope that permission operates on.

~~~
dpeck
awesome. I'd definitely like to see a full writeup if you ever do one, email
is in my profile.

------
yxhuvud
While more interesting than the first part, it should be noted that the policy
does not solve the same problem as the one they had originally, where
different fail reasons led to different URLs.

If you only intend to redirect to one url in case of fail, then using normal
validations (possibly on a service object implementing
ActiveModel::Validations will suffice and produce as simple code as their
solution.

Which isn't saying that I dislike it, only that it is quite equivalent with a
separate service model without controller integration.

~~~
danso
Yeah, I agree with you...I think the implication is that the original
implementation was bad for various reasons, among them, redirecting to
different URLs based on error. But if in fact, that kind of redirection needed
to be done (which would seem to be a mild violation of best-OOP practices),
then I'd agree, a Policy object that lived outside of the Model and the
Controller would be needed.

However, the OP apparently realized in the refactoring that it made more sense
to simply have two paths...or setup a convention so that `redirect` can infer
the correct error page from the error itself. If the OP had insisted that the
Policy keep the path logic, I'm sure DHH would jump all over that as being
bad-design-looking-for-a-solution.

~~~
tomblomfield
Yes - you're absolutely right. We perhaps could have made this trade-off more
explicit.

We didn't want the Policy object to know about the redirect paths, so we opted
to cut down the number of responses available - we simply redirect_to :back by
default, but this can be configured in the controller.

------
Dorian-Marie
I would use CanCan ability.rb file for that:
[https://github.com/CanCanCommunity/cancancan](https://github.com/CanCanCommunity/cancancan)

~~~
bwilliams
My problem with CanCan is that when you begin to have more complicated access
logic ability.rb becomes a giant mess.

It's already a file where you just throw in all of your authorization logic
anyways so it always feels a bit unruly once you get beyond basics.

~~~
Jeff_Dickey
I agree. The first app we wrote using CanCan, the `ability.rb` file (and the
dozen files we factored out of that) grew to be... _significant_.

I _love_ the idea of Pundit because it decouples all that as much as seems
practical. I'm about to find out if theory informs practice or not...

------
jvans
I think this is a responsibility that lies in the model. What if you decide to
allocate tickets through a different model? You have to remember to include
this policy everywhere. Another approach i think is reasonable is creating a
class for this on the model level.

[https://gist.github.com/jvans1/9745395](https://gist.github.com/jvans1/9745395)

