Permalink
Browse files

validate_parameters_match! checks are too aggresive.

According to the AMQP 0.9.1 spec if a queue is re-declared the following
parameters should be checked:

  :durable, :exclusive, :auto_delete, :arguments

and if an exchange is re-declaring then the following parameters should
be checked:

  :type, :durable, :arguments

Before this patch the entire parameters hash was being checked which
caused valid paramters to raise an AMQP::IncompatibleOptionsError
exception.
  • Loading branch information...
1 parent 191d231 commit 1de33d1a0dcba32e6dab70d90ee33629baa80272 Richard Heycock committed Apr 4, 2012
View
@@ -227,6 +227,8 @@ def initialize(connection = nil, id = self.class.next_channel_id, options = {},
# Read more about EM::Deferrable#callback behavior in EventMachine documentation. MK.
@channel_is_open_deferrable = AMQ::Client::EventMachineClient::Deferrable.new
+ @parameter_checks = {:queue => [:durable, :exclusive, :auto_delete, :arguments], :exchange => [:type, :durable, :arguments]}
+
# only send channel.open when connection is actually open. Makes it possible to
# do c = AMQP.connect; AMQP::Channel.new(c) that is what some people do. MK.
@connection.on_connection do
@@ -373,7 +375,7 @@ def direct(name = 'amq.direct', opts = {}, &block)
if exchange = find_exchange(name)
extended_opts = Exchange.add_default_options(:direct, name, opts, block)
- validate_parameters_match!(exchange, extended_opts)
+ validate_parameters_match!(exchange, extended_opts, :exchange)
block.call(exchange) if block
exchange
@@ -481,7 +483,7 @@ def fanout(name = 'amq.fanout', opts = {}, &block)
if exchange = find_exchange(name)
extended_opts = Exchange.add_default_options(:fanout, name, opts, block)
- validate_parameters_match!(exchange, extended_opts)
+ validate_parameters_match!(exchange, extended_opts, :exchange)
block.call(exchange) if block
exchange
@@ -597,7 +599,7 @@ def topic(name = 'amq.topic', opts = {}, &block)
if exchange = find_exchange(name)
extended_opts = Exchange.add_default_options(:topic, name, opts, block)
- validate_parameters_match!(exchange, extended_opts)
+ validate_parameters_match!(exchange, extended_opts, :exchange)
block.call(exchange) if block
exchange
@@ -703,7 +705,7 @@ def headers(name = 'amq.match', opts = {}, &block)
if exchange = find_exchange(name)
extended_opts = Exchange.add_default_options(:headers, name, opts, block)
- validate_parameters_match!(exchange, extended_opts)
+ validate_parameters_match!(exchange, extended_opts, :exchange)
block.call(exchange) if block
exchange
@@ -806,7 +808,7 @@ def queue(name = AMQ::Protocol::EMPTY_STRING, opts = {}, &block)
if name && !name.empty? && (queue = find_queue(name))
extended_opts = Queue.add_default_options(name, opts, block)
- validate_parameters_match!(queue, extended_opts)
+ validate_parameters_match!(queue, extended_opts, :queue)
block.call(queue) if block
queue
@@ -1260,15 +1262,13 @@ def self.method_missing(meth, *args, &blk)
end
-
protected
- # @private
- def validate_parameters_match!(entity, parameters)
- parameters.delete(:no_declare)
- unless entity.opts == parameters || parameters[:passive]
+ @private
+ def validate_parameters_match!(entity, parameters, type)
+ unless entity.opts.values_at(*@parameter_checks[type]) == parameters.values_at(*@parameter_checks[type]) || parameters[:passive]
raise AMQP::IncompatibleOptionsError.new(entity.name, entity.opts, parameters)
end
- end # validate_parameters_match!(entity, parameters)
+ end # validate_parameters_match!(entity, parameters, type)
end # Channel
end # AMQP
@@ -232,6 +232,18 @@
done
end # it
end # context
+
+ context "when exchange is re-declared with irrelevent parameters different from original declaration" do
+ it "doesn't raise an exception" do
+ @channel.direct("previously.declared.durable.direct.exchange", :durable => true)
+
+ expect {
+ @channel.direct("previously.declared.durable.direct.exchange", :durable => true, :header => {:random => 'stuff' })
+ }.to_not raise_error(AMQP::IncompatibleOptionsError)
+
+ done
+ end # it
+ end # context
end # describe
@@ -20,14 +20,17 @@
context "when queue is redeclared with different attributes" do
let(:name) { "amqp-gem.nondurable.queue" }
let(:options) {
- { :durable => false, :passive => false }
+ { :durable => false, :exclusive => true, :auto_delete => true, :arguments => {}, :passive => false }
}
let(:different_options) {
- { :durable => true, :passive => false}
+ { :durable => true, :exclusive => true, :auto_delete => true, :arguments => {}, :passive => false}
+ }
+ let(:irrelevant_different_options) {
+ { :durable => false, :exclusive => true, :auto_delete => true, :arguments => {}, :passive => false, :header => {:random => 'stuff' } }
}
- it "should raise AMQP::IncompatibleOptionsError" do
+ it "should raise AMQP::IncompatibleOptionsError for incompatable options" do
channel = AMQP::Channel.new
channel.on_error do |ch, close|
@callback_fired = true
@@ -39,5 +42,18 @@
}.to raise_error(AMQP::IncompatibleOptionsError)
done
end
+
+ it "should not raise AMQP::IncompatibleOptionsError for irrelevant options" do
+ channel = AMQP::Channel.new
+ channel.on_error do |ch, close|
+ @callback_fired = true
+ end
+
+ channel.queue(name, options)
+ expect {
+ channel.queue(name, irrelevant_different_options)
+ }.to_not raise_error(AMQP::IncompatibleOptionsError)
+ done
+ end
end
end # describe AMQP

0 comments on commit 1de33d1

Please sign in to comment.