Instatiating Aws::CloudFormation::Resource in Capistrano task raises DefinitionError

[fidothe]

Attempting to instantiate an Aws::CloudFormation::Resource in a Capistrano task explodes with a Aws::Resources::Errors::DefinitionError:

Aws::Resources::Errors::DefinitionError: unable to define method #delete, method already exists

Minimal reproduction case is here: https://github.com/fidothe/aws-sdk-resources-cap-bug. To demonstrate, download, bundle install and run any / all of:

bundle exec cap production problem:a
bundle exec cap production problem:b
bundle exec cap production problem:c

[trevorrowe]

Thank you for the failure example. I've reproduced the issue locally and I'll try to see what the culprit is.

[trevorrowe]

So the cause appears to be an interaction between capistrano and the SDK. In this case, I believe that the SDK is behaving properly, and that capistrano is doing a no-no.

The SDK is create a sub-class to Aws::Resources::Resource. When requiring the SDK with capistrano loaded, capistrano has done something to include extra base-classes:

puts Aws::Resources::Resource.ancestors
Object
Capistrano::DSL
Capistrano::DSL::Stages
Capistrano::DSL::Paths
Capistrano::DSL::Env
Capistrano::TaskEnhancements
Kernel
BasicObject

When the SDK then attempts to add methods to the resource class it chooses to raise an error if there are method name conflicts. In this case, Capistrano::DSL::Env has a #delete method defined and this is conflicting.

I would raise this as an issue against capistrano.

[trevorrowe]

For demonstration, I modified the lib/a.rb file, adding the following:

puts Object.ancestors

The output is:

Object
Capistrano::DSL
Capistrano::DSL::Stages
Capistrano::DSL::Paths
Capistrano::DSL::Env
Capistrano::TaskEnhancements
Kernel
BasicObject

For a little more background, the resource classes are built at runtime from a shared definition. There is an intentional check to ensure that a resource action or association name does not re-define any method already present. This protects against the definitions defining methods like #method, #object_id or anything else in the base class (e.g. #reload). The intention is to alert us of the conflict so we can provide renames for these conflicts.

I would ask someone more knowledgeable about Capistrano is it is possible to run it without it monkeypatching the world.

[fidothe]

Le sigh.

My guess is https://github.com/capistrano/capistrano/blob/5c9b27e90d7ac81e47c08c9a6c1c88c31fc66fac/lib/capistrano/dsl.rb#L64 is the culprit. Off the top of my head, I can't see why Rake::Task couldn't be extended instead. I'll raise this with them and see where we get to...

[fidothe]

...And then I looked at the Rake source code.

So, Rake doesn't instance_eval or class_eval or mess with the binding of the task blocks in any way. If you want to add DSL methods to a rake task, you have to extend Object. Sorry - there's not much Capistrano can really do here without Rake changing itself in a way which would almost certainly break half the world's Rakefiles.

Here's a 4-line change to Aws::Resources::Resource to make it inherit from BasicObject. All the tests pass, and it fixes the problem... Ought to work in 1.9+ (no dice in 1.8)

[trevorrowe]

I'm afraid that extending BasicObject would be problematic. There are lots of Ruby standard methods that are only available from Object:

(Class.new.instance_methods - Class.new(BasicObject).instance_methods).sort
:!~
:<=>
:===
:=~
:class
:clone
:define_singleton_method
:display
:dup
:enum_for
:eql?
:extend
:freeze
:frozen?
:hash
:inspect
:instance_of?
:instance_variable_defined?
:instance_variable_get
:instance_variable_set
:instance_variables
:is_a?
:itself
:kind_of?
:method
:methods
:nil?
:object_id
:private_methods
:protected_methods
:public_method
:public_methods
:public_send
:remove_instance_variable
:respond_to?
:send
:singleton_class
:singleton_method
:singleton_methods
:taint
:tainted?
:tap
:to_enum
:to_s
:trust
:untaint
:untrust
:untrusted?

I'm open to other options. I'll look around for some other possibilities and come back. As a temporary work-around, you can use aws-sdk-core only.

[trevorrowe]

What if the safe_define method only happened during test?

The primary goal is to ensure that the resource operation names don't conflict with Ruby standard object methods, like #send. If these conflicts are exposed strictly in development / test environments, then I don't really care if someone has monkey patched object with new functionality. Its fair that my class will clobber or hide that method.

Thoughts?

[trevorrowe]

I made a patch to test this and everything seems to work great. To test against my modified SDK, I added the following to the same repo:

diff --git a/config/deploy.rb b/config/deploy.rb
index dda9215..9e5f52d 100644
--- a/config/deploy.rb
+++ b/config/deploy.rb
@@ -1,6 +1,8 @@
 # config valid only for current version of Capistrano
 lock '3.4.0'

+$:.unshift('~/aws-sdk-v2/aws-sdk-resources/lib')
+$:.unshift('~/aws-sdk-v2/aws-sdk-core/lib')

 namespace :problem do
   task :a do

I'm happy enough with the solution, I'm going to push the fix to master and this should go out with the next release.

[fidothe]

oh sorry, i wasn't paying attention :-(

As I learnt on Friday, Object is just BasicObject with Kernel included...

(Class.new.instance_methods - Class.new(BasicObject) { include Kernel }.instance_methods).sort
=> []

I think your AWS_SDK_SAFE_DEFINE approach is fine, but I also really liked your keep-your-metaprogramming-fingers-out-of-my-sdk-objects approach from before - particularly given how dynamic your object definition approach needs to be for generating stuff the way you do.

I'd think that subclassing BasicObject and including Kernel would give you the best of both worlds – all the convenience methods from Object, but the safety of being disconnected from the main Object hierarchy. My #775 PR (which I, under no circumstances, think you should pull in as-is) passed all the tests just fine - without any other code modificiations. (Without including Kernel too, about 90 tests failed).

BasicObject was pretty much invented for complex metaprogramming needs like yours, after all...

[trevorrowe]

The purpose of #safe_define_method is to detect when the resource models are going to step on regular Ruby methods. If a module is mixed into Object that defines a #delete method and then a resource class needs to define #delete, I'm okay with that. The resource class will get the correct definition of delete. I'm not okay if a resource class for Amazon Simple Email Service were to define a #send method and clobber the default Ruby #send method from Object.

I'm open to re-evaluate this if we run into further issues. Subclasses BasicObject seems like it would also be a suitable situation, I'm just afraid of the unknown edge cases we might encounter.

[fidothe]

Sounds good - my main objection to the solution I proposed in #775 was the issue of edge cases...