0

My app has an STI model:

# file: app/models/metered_service.rb
class MeteredService < ActiveRecord::Base
  ...
end
# file: app/models/metered_services/pge_residential.rb
class PGEResidential < MeteredService
  ...
end
# file: app/models/metered_services/sce_residential.rb
class SCEResidential < MeteredService
  ...
end

and a schema that support STI:

# file: db/schema.rb
create_table "metered_services", :force => true do |t|
  t.integer  "premise_id"
  t.string   "type"
end

MeteredService is a nested resource (though that's not really relevant to this question):

# file: config/routes.rb
resources :premises do
  resources :metered_services    
end

So here's the deal: To create a MeteredService, the user chooses one of its many sub-classes in a pulldown list. The form returns the class name to the MeteredServicesController#create in params['metered_services']['class'] as a string. Now we need to create the proper subclass.

The approach I'm taking works - sort of - but I'm wondering if this is the best way:

def create
  @premise = Premise.find(params[:premise_id])
  MeteredService.descendants()  # see note
  class_name = params["metered_service"].delete("class")
  @metered_service = Object.const_get(class_name).new(params[:metered_service].merge({:premise_id => @premise.id}))
  if @metered_service.save
    ... standard endgame
  end
end

What I'm doing is stripping the class name out of params['metered_service'] so I can use the remaining parameters to create the metered service. And the class_name is resolved to a class (via Object.const_get) so I can call the .new method on it.

The MeteredServices.descendants() call is there because of the way caching is done in development mode. It works, but it's really ugly - see this question for an explanation of why I'm doing it.

Is there a better / more reliable way to do this?

Community
  • 1
  • 1
fearless_fool
  • 33,645
  • 23
  • 135
  • 217
  • Be careful calling `const_get` on user input. they could theoretically pass in anything. Maybe you could filter with a whitelist? – John Gibb May 25 '11 at 05:14
  • I just figured out that although I can't do `MeteredService.new(:type => 'PGEResidential')`, I **can** do `MeteredService.new(...) {|m| m.type = 'PGEResidential'}`, which (astonishingly) seems to do the right thing and (nod to @John Gibb) ends up being safe, since the type is checked upon saving. – fearless_fool May 25 '11 at 05:52

1 Answers1

0

As John Gibb said in his comment, your main problem is security. You must filter the classes through your approved white list.

The solution you gave in your comment is also not perfect. First an instance of MeteredService is created, and then you just change a text property with a name of another class. The instance you are working with is still the base class. This may lead to some problems if you, for example, define some validations on the descending class.

Do something like this:

AVAILABLE_CLASSES = {"PGEResidential" => PGEResidential,
                     "SCEResidential" => SCEResidential } # You may automatize this

def create
  #....
  class_name = params["metered_service"].delete("class")
  if c = AVAILABLE_CLASSES[class_name]
    @metered_service = c.new(params[:met...
  else
    handle_error_somehow
  end
  ...
Arsen7
  • 12,522
  • 2
  • 43
  • 60
  • giving you the check, since your approach would work. I ended up abandoning the STI model. MeteredService is now an ordinary model, and what used to be sub-classes are now simple (non-AR) classes. Saves lots of headaches. – fearless_fool Jul 19 '11 at 17:40