incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: Bug in client/lib/deltacloud.rb#xml_to_class
Date Tue, 12 Oct 2010 12:37:20 GMT
On 06/10/10 15:57 -0400, Toby Crawley wrote:

> 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

Yes, that's true. I know that xml_to_class method is monstrous and it can
cause serious troubles like action_url issue. I'm totally open for
splitting this method to smallest chunks, which will be more maintainable
and at least readable.

I like idea of having:

   'BaseObject' (For holding 'reals', 'hardware_profiles' and stuff like this)
   |
   -> 'ActionObject' ('instances', 'buckets', 'volumes', 'keys')

So 'BaseObject' should hold all resource which is some kind of 'static',
means they doesn't have .action! methods. And 'ActionObject', which will
add 'actions'. Let me know what do you think.

> 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.

Absolutely agree.

   -- Michal

-- 
--------------------------------------------------------
Michal Fojtik, mfojtik@redhat.com
Deltacloud API: http://deltacloud.org
--------------------------------------------------------

Mime
View raw message