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
