Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse URI Query Parameters #67

Merged
merged 1 commit into from
Dec 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion lib/amq/uri.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,23 @@ class URI
# @private
AMQP_PORTS = {"amqp" => 5672, "amqps" => 5671}.freeze

DEFAULTS = {
heartbeat: nil,
connection_timeout: nil,
channel_max: nil,
auth_mechanism: [],
verify: false,
fail_if_no_peer_cert: false,
cacertfile: nil,
certfile: nil,
keyfile: nil
}.freeze

def self.parse(connection_string)
uri = ::URI.parse(connection_string)
raise ArgumentError.new("Connection URI must use amqp or amqps schema (example: amqp://bus.megacorp.internal:5766), learn more at http://bit.ly/ks8MXK") unless %w{amqp amqps}.include?(uri.scheme)

opts = {}
opts = DEFAULTS.dup

opts[:scheme] = uri.scheme
opts[:user] = ::CGI::unescape(uri.user) if uri.user
Expand All @@ -26,8 +37,28 @@ def self.parse(connection_string)
opts[:vhost] = ::CGI::unescape($1)
end

if uri.query
query_params = CGI::parse(uri.query)

normalized_query_params = Hash[query_params.map { |param, value| [param, value.one? ? value.first : value] }]

opts[:heartbeat] = normalized_query_params["heartbeat"].to_i
opts[:connection_timeout] = normalized_query_params["connection_timeout"].to_i
opts[:channel_max] = normalized_query_params["channel_max"].to_i
opts[:auth_mechanism] = normalized_query_params["auth_mechanism"]

%w(verify fail_if_no_peer_cert cacertfile certfile keyfile).each do |ssl_option|
if normalized_query_params[ssl_option] && uri.scheme == "amqp"
raise ArgumentError.new("Only of use for the amqps scheme")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message doesn't clarify what is "of use" (can be used). Please add it.

Copy link
Contributor Author

@Tensho Tensho Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, what does sound the best?

  1. 'verify' TLS option is applicable only for 'amqps' schema
  2. 'verify' option should be used only within 'amqps' schema
  3. TLS options are relevant only for 'amqps' schema

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The option 'verify' can only be used in URIs that use amqps for schema". Arguably it should be a warning since non-TLS connections would simply ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind to convert it to the warning, but an exception could attract attention to the possible misspelling of the schema. Maybe you already have a convention in this or other ruby-amqp libraries, so it will be natural to keep consistent behavior across all of them.

Copy link
Contributor Author

@Tensho Tensho Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, it was so fast. I feel like talk too much ^_^
Leave the exception or change to the warning?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did change the message in 50dbf6b but because I cannot push that to your repo, it won't show up here.

Copy link
Contributor Author

@Tensho Tensho Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed when fetched upstream ^_^ That's OK. BTW, is it preferable to contribute from forked repo or straight to the initial repo? There is no CONTRIBUTION guide in amq-protocol ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have the permissions to push to the mainline. I see no reason to not pull from and branch off of it. I don't really care as long as the PR is against the correct branch and is something I'd accept.

else
opts[ssl_option.to_sym] = normalized_query_params[ssl_option]
end
end
end

opts
end

def self.parse_amqp_url(s)
parse(s)
end
Expand Down
76 changes: 76 additions & 0 deletions spec/amq/uri_parsing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,80 @@
expect(val[:vhost]).to be_nil # in this case, default / will be used
end
end

subject { described_class.parse(uri) }

context "schema 'amqp'" do
context "query parameters" do
context "present" do
let(:uri) { "amqp://rabbitmq?heartbeat=10&connection_timeout=100&channel_max=1000&auth_mechanism=plain&auth_mechanism=amqplain" }

specify "parses parameters" do
expect(subject[:heartbeat]).to eq(10)
expect(subject[:connection_timeout]).to eq(100)
expect(subject[:channel_max]).to eq(1000)
expect(subject[:auth_mechanism]).to eq(["plain", "amqplain"])
end
end

context "absent" do
let(:uri) { "amqp://rabbitmq" }

it "fallbacks to defaults" do
expect(subject[:heartbeat]).to be_nil
expect(subject[:connection_timeout]).to be_nil
expect(subject[:channel_max]).to be_nil
expect(subject[:auth_mechanism]).to be_empty
end
end

context "tls parameters" do
%w(verify fail_if_no_peer_cert cacertfile certfile keyfile).each do |tls_param|
describe "'verify'" do
let(:uri) { "amqp://rabbitmq?#{tls_param}=true" }

it "raises ArgumentError" do
expect { subject }.to raise_error(ArgumentError, /Only of use for the amqps scheme/)
end
end
end
end
end
end

context "schema 'amqps'" do
context "query parameters" do
context "present" do
let(:uri) { "amqps://rabbitmq?heartbeat=10&connection_timeout=100&channel_max=1000&auth_mechanism=plain&auth_mechanism=amqplain&verify=true&fail_if_no_peer_cert=true&cacertfile=/examples/tls/cacert.pem&certfile=/examples/tls/client_cert.pem&keyfile=/examples/tls/client_key.pem" }

it "parses parameters" do
expect(subject[:heartbeat]).to eq(10)
expect(subject[:connection_timeout]).to eq(100)
expect(subject[:channel_max]).to eq(1000)
expect(subject[:auth_mechanism]).to eq(["plain", "amqplain"])
expect(subject[:verify]).to be_truthy
expect(subject[:fail_if_no_peer_cert]).to be_truthy
expect(subject[:cacertfile]).to eq("/examples/tls/cacert.pem")
expect(subject[:certfile]).to eq("/examples/tls/client_cert.pem")
expect(subject[:keyfile]).to eq("/examples/tls/client_key.pem")
end
end

context "absent" do
let(:uri) { "amqps://rabbitmq" }

it "fallbacks to defaults" do
expect(subject[:heartbeat]).to be_nil
expect(subject[:connection_timeout]).to be_nil
expect(subject[:channel_max]).to be_nil
expect(subject[:auth_mechanism]).to be_empty
expect(subject[:verify]).to be_falsey
expect(subject[:fail_if_no_peer_cert]).to be_falsey
expect(subject[:cacertfile]).to be_nil
expect(subject[:certfile]).to be_nil
expect(subject[:keyfile]).to be_nil
end
end
end
end
end