Rails – The Missing Parts – Policies(eng.joingrouper.com) |
Rails – The Missing Parts – Policies(eng.joingrouper.com) |
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.)
In a nutshell, almost all DCI/service object blog posts I have read seem to have almost all of their issues addressed by a liberal use of `concerns`.
In the end, I think we're all talking about how to separate "concerns/responsibilities/dependencies" into atomic units so our code is easier to reason about; it turns out my user object does a Ton Of Things and it becomes more maintainable once we split it up.
The schism in my mind comes more from random blog post declaring "We have seen the truth! and it is $new_jargon_here" and then DHH pops out of the wilderness and crankily declares "yes, yes, this is why we introduced xyz, jeez."
What strikes me most in the end is that most of the random blog posts seem to lack a coherent theory for why their code has improved, rendering most of the conversation around it moot. DHH on the other hand has strong, and well formed opinion on the subject which is why he can occasionally come through and refactor it along his mental model.
This is all to say: I have rather strong, if currently unarticulated, opinions on how to think about code organization, and I tend to be more on DHH's side when it comes to these debates. Furthermore, I'm increasingly convinced we're all talking about the same things using different words.
DHH / standard-Rails seems to encourage you to fit things into Models, Views or Controllers. Models got too fat, and so Concerns were introduced. This is problematic, because you still have massive god-models, but now their code is split between a dozen different files, called "Concerns". The main problem is that by making ActiveRecord models the core of your application, you generally give each model too much responsibility & too much knowledge of its associated objects. This means it's very hard to break down & recompose the different functionality into new objects. A model only functions correctly if all of its associated objects are present. This is tight coupling.
The alternative view is to introduce several new single-responsibility objects like Interactors/Service-Objects (and Policies, as here) which should contain the core of your application's business logic. These Service Objects are the opposite of concerns - they do not extend the API of models. They exist to control the interactions between the models. The models become dumb interfaces that deal with data validation and persistence. Because they know hardly anything about the other objects in the system, and have very limited APIs, they can be passed around easily, and re-composed into more complex interactions. They are also incredibly simple to test. The same applies to Interactors - they should be broken down into small units of behaviour with very limited APIs, which can then be passed around easily and composed into more complex interactions. This is loose coupling.
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.
But I don't think that moving towards more single-responsibility objects means you have to give up all that Rails offers - particularly the preference for concise, human-readable APIs and convention-over-configuration. Ie, we don't need tonnes of boiler-plate just because we want to use more objects.
We put this permission check in the controller and not the interactor because it seems like the controller should generally be responsible for authentication & permissioning. Once you get to the interactor, it should just be told "Perform this interaction". Not "check if you can perform it, and then perform it".
They are overlapping camps but not exactly the same.
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
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
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.
http://api.rubyonrails.org/classes/ActiveModel/Validator.htm...
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.
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.
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.
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.
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.
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.
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.
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...
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 -- it includes domain logic methods like "getExemption", "isFlaggedForAudit", and so forth. Exactly like the pattern describes.
You're not a beautiful and unique snowflake.
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.
Agreed on so many level. I see so many new gems that does the exact same thing as what rails do by default. I've spent some times browsing Rails source. It took me a while and as a side effect, the number of gems I am using now is a fraction of what I used when I started.
You can't build libraries and abstraction on top of Rails if you don't understand rails to begin with.
Edit: I should probably note that I _don't_ think you should be "required" to help out like this at all. Simply developing/sharing Rails is certainly _way_ more than I'm doing for other people. I'm just saying if you _are_ going to comment on this stuff, it would be nice if you transferred some of your greater understanding with your comments.
I respect your opinion but it seems that you aren't really addressing the advantages the article is claiming their design has.
The likelihood of change is much different for the policies than for the basic domain model attributes of a ticket, and this to me is the key reason to prefer the policy design that I'd like to hear you address objectively.
Based on your comment about swappable policies, I'd guess that your position is to wait until you need to pull it out before you do. But if you can see a likely change coming and there is no cost to designing for it, why not do it? Is your bone of contention with the idea that we can assign reasonable probabilities to what is likely to change; with the idea that there is no cost to pulling the policy object out now; with both of those things; or with something else entirely?
The more you know...
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).
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.
It seems that the logical ontology is based on ruby's objects. Unfortunately, this makes the solution much more convoluted than a simple function exported in a module.
But both leave a lot to be desired. I personally think in both cases they are a code smell, and we ought to either introduce an object whose explicit job it is to worry about these inter domain interactions or pile it up as a mixin into the appropriate domain models.
case
when user.blacklisted?
# ... code ...
when grouper.full?
# ... code ...
# ... and so on ...
endRails encourages you to use the "primitives" it ships with, because they've all been more or less designed to work together, and I think this is what DHH is getting at - you get a lot of free behaviour, and in his mind you need a rather strong reason to be throwing them away.
>This is problematic, because you still have massive god-models, but now their code is split between a dozen different files, called "Concerns".
When you put it that way, that still sounds identical to your policies and interactors - you've just chosen a different nomenclature.
Both your approach and concerns are reactions to the way we've been shoving "responsibilities" into our domain objects. When we talk about coupling, we're talking about our ability to reason about our business logic in isolation; generally, the fewer atomic items we have to think about, the easier it is to maintain.
Concerns are just a DSL for writing Ruby mixins; I recently upgraded an older app and discovered I could port it all over by renaming my app/modules folder to app/concerns and I was basically done. So, the differentiating aspect rests in your preferred mental model for breaking up the business logic into these "atomic units".
When you talk about disliking AR models, to my ears it sounds like what you're really saying is "I hate the overhead of hitting the database when I'm writing tests", which is a different idea altogether from "these semantic units ought to only be responsible for serialization".
To me, it's not necessarily relevant if this one object can be persisted; what matters is that it's semantically relevant in my model of the domain/business/etc.
So, at the end of the day there is a ton of logic that is more meaningful when associated with my "users"; but it's an enormous pain in the ass to have huge swaths of my User model governing totally different bits of unrelated functionality. In this circumstance, splitting off the different unrelated bits of functionality is a net win, from both testing and maintenance.
Meanwhile, if we find that there is no good place for stuffing this bit of functionality… then we're probably missing a whole new domain model and we didn't even know it, and imho we're better off encapsulating those behaviours into an independent model.
This is to say: policies as presented in your post strike me as a more convoluted way to achieve this separation of concerns already afford to us by regular ruby mixins/classes.
The Rails Way is fine for a prototype, or for an app that is so well-understood that the team know from the beginning will have sharply limited change in certain very-well-known areas. It's safe to say that the vast majority of the apps I've seen in my career do not fall into that category.
The disparity here is between the MVC paradigm and these "emergent" groups being discussed.
The problem is that they don't have a coherent solution. Rather, there are 100 little "solutions" running around with no central philosophy behind them. That's a Bad Thing.
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.
Eg, a ConfirmGrouper.perform object (http://eng.joingrouper.com/blog/2014/03/03/rails-the-missing...)
Uncle Bob's talk gives good reasons for this pattern: http://www.confreaks.com/videos/759-rubymidwest2011-keynote-...
Basically, your data-persistence objects (ActiveRecord models) become more composable because they do fewer things and rely less on associated objects being present to work properly.
Your "interactor" objects which perform "verb"-style activities can also be broken down into very small units with single-responsibility and then composed with each other to perform more complex interactions.
Putting all of this logic into models makes it very very hard to break apart units of behaviour and re-compose them.
This is what I mean by "less coupled, more maintainable"
I agree with this entirely, and elsewhere in this thread have said so, and I'm not advocating fat models.
I'm more questioning the precise way in which you've chosen to perform your refactoring. To me, that-method-in-partcular seems like a code smell, and I've not been convinced by your blog post compared to the alternate methods I've suggested in my comment. Does that make sense?
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.
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.
I've spent a lot of time thinking about this subject, and have only recently begun to feel confident enough to speak with some authority on the subject.
I think people aren't thinking critically about their code in quite the right way, despite their blog posts on the topic, but I've hitherto been unable to compose my thoughts outside of comment boxes.
The function itself can be decomposed into smaller functions.
If you need extensibility, you could make the function composable and register additional pieces to the policy. Most codebases don't need this sort of extensibility.
Simple constructs & consistent, accurate, precise naming is preferable to complicated architectures.
I've yet to catch up on my lisp and fully grok the "functional paradigm" and, admittedly, passing around consts is less elegant than passing around function pointers, but what you're describing sounds isomorphic to me in terms of code organization. Look at the blog post; he's just passing a one method class, which… is basically a less elegant function.
Javascript has functions, from the function keyword. Node.js uses commonjs. In commonjs, every file is a module. You can use
module.exports = <value>;
You can access the module by using require.
var moduleValue = require("path/to/module");
It's powerful because it doesn't have the namespace collisions that Ruby has. Everything is not global all the time. modules are also an elegant way of holding private state using closures.
Having a class with a single method is the Ruby way of categorizing this method within the domain.
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.
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.
I don't think this analogy applies. If my house is full of things, I prefer to put them in small, well recognizable boxes, instead of in one gigantic mess. This is simplicity too.
The file name contains dash(-) replace them with slash(/) as github doesn't like path.
https://gist.github.com/pothibo/9673715
I hope you like it ;)
Edit: It had a shit loads of typos, I believe I have fixed most of them.
My fork had two objectives. The first was to nail the argument of "What do I do if my model has 1.2k LOC". Look at it as an extension of your solution.
The second was to show that before using Policy, there is mechanism in rails that you can use to extract bit of code away from the model (concerns, validators, etc.)
So yeah, you could say this was a future coding for possibly-if moons are aligned- extension points ;)
To reiterate, I start with in-model validation 100% of the time and move from there as needs be.
Fundamentally I have an issue with this whole approach, if you have bloated domain objects then organise them better (as DHH mentions a common problem is not having enough classes in your domain). Also, given the choice between bloated objects and logic separated from it's information I would take bloated objects any day.
Either you want to use MVC or you want to go in a different direction. Fine. But don't blame Rails for being a good MVC implementation. Instead, formulate your own, new "philosophy" about how this all should work.
While I don't generally acknowledge the validity of "Before you criticize, think of something better", in this case I think it's appropriate. You appear to be critical of the whole MVC philosophy. And that's FINE. No problem. But before you go criticizing Rails I think you should formulate your own philosophy of how it all should work instead, rather than blaming Rails for doing what it is supposed to do, and doing it well.
Twenty-six (26) lines of model code that are dedicated to validating this specific "confirmation" validation.
Given that validation is just one of the concerns of models built with The Rails Way, imagine sprinkling all of the other code that a model will have in-and-around these 26 lines of code. The central purpose of those 26 lines of code vanish into the fog of the rest of the model, creating the mystical behaviors that are common in some Rails applications.
I totally understand the concern about preemptive architecture, but once the code count for a given behavior creeps from 4, to 8, to 12... it's time to organize it into some sort of central place that's isolated from the rest. I mean, gosh, we know how this works in the real world, right?
Dev A: Here's my 26 lines of validation code to the model, DHH says it's the right thing to do.
Dev B (hours later): I need to add a little bit of behavior here, but I'm only adding a little bit of code. I'm not going to try to deduce what all of that code does, so I'll just tack mine on the bottom.
Dev C (days later): I have to add a bit of code... hmm... this is sort of a mess, I see mostly validation code but I also see some behavior. I'm not touching any of it. I'll just patch my stuff here... and here!
[iterate over and over, you get a mess of a Rails app]
I've seen this happen to multiple Rails applications... Rails applications built by experienced developers who know Rails in-and-out.
Plopping a bunch of code in something as important as a Rails model and leaving the cleanup for the next dev isn't very polite.