incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Toby Crawley <tcraw...@redhat.com>
Subject Bug in client/lib/deltacloud.rb#xml_to_class
Date Wed, 06 Oct 2010 19:57:19 GMT
I've been working on enhancing Deltacloud storage_volume support, and 
stumbled across a serious bug in the client. The issue is with 
deltacloud.rb#xml_to_class, and the way it handles actions that are 
defined in the xml.

For each object, it defines three methods on the class for that object:

* action!, where 'action' is the actual action (so stop!, destroy!, etc) 
- this performs the action
* actions - returns a list of action names
* actions_urls - returns a hash of { action => action_url }

The issue is that all three of these methods are defined on the class as 
closures over the action list for a particular object. So if you are 
iterating over a collection, *all* members of the collection will have 
the three methods defined for the *last* collection member, since the 
methods are defined at the class level. This results in:

irb> client.keys.collect {|x| [x.id, x.actions_urls]}
[
     [0] [
         [0] "somekey",
         [1] {
             "destroy" => 
"http://localhost:8080/deltacloud/api/keys/default"
         }
     ],
     [1] [
         [0] "default",
         [1] {
             "destroy" => 
"http://localhost:8080/deltacloud/api/keys/default"
         }
     ]
]

which means a call to client.keys.first.destroy! actually destroys 
client.keys.last.

xml_to_class has a good bit of meta-programming, and is difficult to 
decipher. Is there a reason why the client starts with a bare Class, and 
turns it in to Instance, Key, etc? I understand the motivation of taking 
some base class and creating Instance, Key, etc. dynamically, but 
wouldn't it make sense to have a base class that provided some 
functionality (written off the cuff, and untested):

class DeltaCloud::API::BaseObject

   def action_meta
     @action_meta ||= { }
   end

   def add_action(action, method, url)
     action_meta[action.to_s] = [method, url]
     # you could also define_method here instead of relying on 
method_missing (below)
   end

   def actions
     actions_meta.keys
   end

   def actions_urls
     action_meta.inject({}) { |accum, action, method_and_url| 
accum[action] = method_and_url.last; accum }
   end

   def call_action(action)
     method, url = action_meta[action]
     client.request(method.to_sym, url, {}, {})
     # would need to get status here
   end

   def method_missing(meth, *args, &block)
     if meth.to_s =~ /^(.*)!$/ and actions.include?($1)
       call_action($1)
     else
       super
     end
   end

end


DeltaCloud.define_class could then instead create a new class that 
subclassed the above class, and xml_to_class (for actions at least) 
becomes much simpler:

                 when "actions":
                   actions = []
                   attribute.xpath('link').each do |link|
                     actions << [link['rel'], link[:href]]
                     define_method :"#{link['rel'].sanitize}!" do
                       client.request(:"#{link['method']}", 
link['href'], {}, {})
                       @current_state = client.send(:"#{item.name}", 
item['id']).state
                       obj.instance_eval do |o|
                         def state
                           @current_state
                         end
                       end
                     end
                   end
                   define_method :actions do
                     actions.collect { |a| a.first }
                   end
                   define_method :actions_urls do
                     urls = {}
                     actions.each { |a| urls[a.first] = a.last }
                     urls
                   end

would become (ignoring the 'state' method for now):

                 when "actions":
                   attribute.xpath('link').each do |link|
                     obj.add_action(link['rel'].sanitize, 
link['method'], link[:href])
                   end

There is much more the base class would need to do in order to satisfy 
all of what xml_to_class, base_object, and other methods on the client 
set up, but you would end up with an implementation that would be more 
readable and more easily tested.

I'm certainly new to the Deltacloud code base (I really just started 
digging in to it yesterday), so let me know if I'm way off base.
Toby

Mime
View raw message