secret
Created

Code Show and Tell: PolymorphicFinder

  • Download Gist
application_controller.rb
Ruby
1 2 3 4 5 6 7 8 9 10 11
class ApplicationController < ActionController::Base
private
def requested_purchaseable
PolymorphicFinder.
finding(Section, :id, [:section_id]).
finding(TeamPlan, :sku, [:team_plan_id]).
finding(IndividualPlan, :sku, [:individual_plan_id]).
finding(Product, :id, [:product_id, :screencast_id, :book_id, :show_id]).
find(params)
end
end
polymorphic_finder.rb
Ruby
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57
# Finds one of several possible polymorphic members from params based on a list
# of relations to look in and attributes to look for.
#
# Each polymorphic member will be tried in turn. If an ID is present that
# doesn't correspond to an existing row, or if none of the possible IDs are
# present in the params, an exception will be raised.
class PolymorphicFinder
def initialize(finder)
@finder = finder
end
 
def self.finding(*args)
new(NullFinder.new).finding(*args)
end
 
def finding(relation, attribute, param_names)
new_finder = param_names.inject(@finder) do |fallback, param_name|
Finder.new(relation, attribute, param_name, fallback)
end
 
self.class.new(new_finder)
end
 
def find(params)
@finder.find(params)
end
 
private
 
class Finder
def initialize(relation, attribute, param_name, fallback)
@relation = relation
@attribute = attribute
@param_name = param_name
@fallback = fallback
end
 
def find(params)
if id = params[@param_name]
@relation.where(@attribute => id).first!
else
@fallback.find(params)
end
end
end
 
class NullFinder
def find(params)
raise(
ActiveRecord::RecordNotFound,
"Can't find a polymorphic record without an ID: #{params.inspect}"
)
end
end
 
private_constant :Finder, :NullFinder
end
polymorphic_finder_spec.rb
Ruby
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56
require 'spec_helper'
 
describe PolymorphicFinder do
describe '#find' do
it 'finds the first given finder when present' do
individual_plan = create(:individual_plan, sku: 'abc')
 
result = PolymorphicFinder.
finding(IndividualPlan, :sku, [:individual_plan_id]).
find(individual_plan_id: 'abc')
 
expect(result).to eq(individual_plan)
end
 
it 'finds the first of several possible params' do
screencast = create(:screencast)
 
result = PolymorphicFinder.
finding(Product, :id, [:book_id, :screencast_id, :product_id]).
find(screencast_id: screencast.to_param)
 
expect(result).to eq(screencast)
end
 
it 'cascades when the first finder is not present' do
create(:individual_plan, sku: 'abc')
team_plan = create(:team_plan, sku: 'def')
 
result = PolymorphicFinder.
finding(IndividualPlan, :sku, [:individual_plan_id]).
finding(TeamPlan, :sku, [:team_plan_id]).
find(team_plan_id: 'def')
 
expect(result).to eq(team_plan)
end
 
it 'raises an exception for an unknown ID' do
create(:individual_plan, sku: 'abc')
 
finder = PolymorphicFinder.
finding(IndividualPlan, :sku, [:individual_plan_id])
 
expect { finder.find(individual_plan_id: 'def') }.
to raise_error(ActiveRecord::RecordNotFound)
end
 
it 'raises an exception without any ID' do
params = { 'key' => 'value' }
finder = PolymorphicFinder.
finding(IndividualPlan, :sku, [:individual_plan_id])
 
expect { finder.find(params) }.
to raise_error(ActiveRecord::RecordNotFound, /#{Regexp.escape(params.inspect)}/)
end
end
end

Interesting to see NullFinder chosen as a name, despite the advice given in your earlier Weekly Iteration episode. Perhaps MissingFinder or UnknownFinder?

I think there's some confusion here that results in not fully understanding the model. Are all of the models here (Section, TeamPlan, IndividualPlan, Product) separate tables? Why aren't there different controller endpoints to look each specific one up if they're separate? If they are actually polymorphic (as in Rails' sense of polymorphic), then just a find should work.

Also, another approach entirely could be a shortlink style approach, easily implemented via to_param, or just some simple parsing. For example:

  • /purchase/S1234 looks up Section with ID 1234,
  • /purchase/T4567 looks up TeamPlan with ID 4567,
  • etc.

At the point of extracting a class, and implementing a ton of extra logic on top of an already complicated controller scheme, there has to be a point where you step back, look up, and realize there's a way to escape out of this maze.

@defeated yeah, I think that's a good idea. MissingFinder would probably be better.

@qrush some of them are subclasses of Product, which is an STI model, and some are totally different models. It's also a little confusing, because in some places, the models are treated like any old Product, and in some places they're treated specially.

I think it's worth noting a few things about the background of this code:

  • The code I was working on had been extended a few times recently, and was a little sticky each time.
  • I was fixing a bug that had already recurred several times in that code.
  • The new code took me less than an hour to write and refactor.

Every implementation is a balancing act of overall complexity, unit complexity, overall readability, unit readability, ease of change, and robustness. We generally optimize for readability first, which is why the code was originally structured as an if statement. However, after fixing the same bug multiple times and having a hard time extending the code, I switched priorities and optimized for ease of change and robustness instead.

I think it's also worth talking about the future of the code:

  • We're planning on collapsing all of these models into Product soonish; when we do so, this problem (and therefore this class) will disappear.
  • If we weren't planning on collapsing them, I would have done a harder revamp of the way the models were used. We were using the build-in to_param and url_for behavior, because it's the least custom thing we could do. Now that things have gotten so complex, something like you're talking about with custom IDs or some custom route helpers would likely improve things.

Basically, this solution was an easy way to pull the complexity out of that controller, make it more testable, and make it more resilient to bugs. Although it's complex, I think it's a good middle ground that makes it more likely that the code will survive the transition to a single Product model without reintroducing the same bug one more time.

Based on the original code in the blog post, I would have refactored it more along these lines:

https://gist.github.com/pithyless/50457bdaadb21e88d282

The PolymorphicFinder creates unnecessary abstractions and hides the fact that we're just handling an unconventional use-case. In the end, which pattern will help a new developer grok the author's intention with the least amount of head-scratching?

Another solution is to just pass a parameter in the router and not have to guess anything.

resources :books do
  resource :purchase, purchaseable: :book
end

resources :screencasts do
  resource :purchase, purchaseable: :screencast
end

Hi,

@jferris I've have some case that is something like that

if params[:individual_plan_id] && params[:team_plan_id]
  Class.new(params)
else if params[:individual_plan_id]
  OtherClass.new(params)
else if params[:section_id]
  AnotherClass.new(params)

I've tried to use PolymorphicFinder in this case but I'm not getting it because it only verifies if at least one parameter is present.

I've tried to change but i'm stuck in these line that verifies only one parameter at time.

Do you have some ideas to fit my need?

Thanks!

Please sign in to comment on this gist.

Something went wrong with that request. Please try again.