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

Extend URI Parsing Specs #69

Merged
merged 7 commits into from
Dec 13, 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
47 changes: 32 additions & 15 deletions lib/amq/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,32 @@ module Settings
# @see AMQ::Client::Settings.configure
def self.default
@default ||= {
# server
:host => "127.0.0.1",
:port => AMQ::Protocol::DEFAULT_PORT,
# TCP/IP connection parameters
host: "127.0.0.1",
port: AMQ::Protocol::DEFAULT_PORT,
auth_mechanism: [],

# login
:user => "guest",
:pass => "guest",
:vhost => "/",
# authentication parameters
user: "guest",
pass: "guest",
vhost: "/",

# ssl
:ssl => false,
# client connection parameters
frame_max: (128 * 1024),
heartbeat: nil,
connection_timeout: nil,
channel_max: nil,

:frame_max => (128 * 1024),
:heartbeat => 0
}
# ssl parameters
ssl: false,
verify: false,
fail_if_no_peer_cert: false,
cacertfile: nil,
certfile: nil,
keyfile: nil
}.freeze
end


# Merges given configuration parameters with defaults and returns
# the result.
#
Expand All @@ -43,9 +51,18 @@ def self.default
# @option settings [String] :user ("guest") Username to use for authentication.
# @option settings [String] :pass ("guest") Password to use for authentication.
# @option settings [String] :ssl (false) Should be use TLS (SSL) for connection?
# @option settings [String] :timeout (nil) Connection timeout.
# @option settings [String] :broker (nil) Broker name (use if you intend to use broker-specific features).
# @option settings [Fixnum] :frame_max (131072) Maximum frame size to use. If broker cannot support frames this large, broker's maximum value will be used instead.
# @option settings [Integer] :heartbeat (nil) Heartbeat timeout value in seconds to negotiate with the server.
# @option settings [Integer] :connection_timeout (nil) Time in milliseconds to wait while establishing a TCP connection to the server before giving up.
# @option settings [Fixnum] :channel_max (nil) Maximum number of channels to permit on this connection.
# @option settings [Array] :auth_mechanism ([]) SASL authentication mechanisms to consider when negotiating a mechanism with the server. This parameter can be specified multiple times to specify multiple mechanisms, e.g. `?auth_mechanism=plain&auth_mechanism=amqplain`.
# @option settings [Boolean] :verify (false) Controls peer verification mode.
# @option settings [Boolean] :fail_if_no_peer_cert (false) When set to true, TLS connection will be rejected if client fails to provide a certificate.
# @option settings [String] :cacertfile (nil) Certificate Authority (CA) certificate file path.
# @option settings [String] :certfile (nil) Server certificate file path.
# @option settings [String] :keyfile (nil) Server private key file path.
#
# @option settings [String] :broker (nil) Broker name (use if you intend to use broker-specific features).
#
# @return [Hash] Merged configuration parameters.
def self.configure(settings = nil)
Expand Down
9 changes: 7 additions & 2 deletions lib/amq/uri.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@
module AMQ
class URI
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think if we try to conform URI Ruby standard library hierarchy and extract separate AMP::URI::AMQP and AMP::URI::AMQPS classes?

amq uri

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any real benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could simplify the code and make it more object-oriented instead of procedural in my opinion. Because AMQP URI is just another kind of generic URI, contributors, that are familiar already with URI (especially as it's a standard library), intuitively could rely on known public API.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how having 4 new classes would simplify things. We are not preparing this library for inclusion into the standard library.

Moving classes and constants around means Bunny and amqp gem will have to be updated, and their older versions won't be compatible with newer version of amq-protocol, which has been the case for years.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry but "more object-oriented code" for a couple of constants is a solution in search of a problem.

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 see, appreciate your open point of view, thank you 🙇

# @private
AMQP_PORTS = {"amqp" => 5672, "amqps" => 5671}.freeze
AMQP_DEFAULT_PORTS = {
"amqp" => 5672,
"amqps" => 5671
}.freeze

private_constant :AMQP_DEFAULT_PORTS

DEFAULTS = {
heartbeat: nil,
Expand All @@ -30,7 +35,7 @@ def self.parse(connection_string)
opts[:user] = ::CGI::unescape(uri.user) if uri.user
opts[:pass] = ::CGI::unescape(uri.password) if uri.password
opts[:host] = uri.host if uri.host
opts[:port] = uri.port || AMQP_PORTS[uri.scheme]
opts[:port] = uri.port || AMQP_DEFAULT_PORTS[uri.scheme]
opts[:ssl] = uri.scheme.to_s.downcase =~ /amqps/i # TODO: rename to tls
if uri.path =~ %r{^/(.*)}
raise ArgumentError.new("#{uri} has multiple-segment path; please percent-encode any slashes in the vhost name (e.g. /production => %2Fproduction). Learn more at http://bit.ly/amqp-gem-and-connection-uris") if $1.index('/')
Expand Down
173 changes: 149 additions & 24 deletions spec/amq/uri_parsing_spec.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
require "amq/uri"

# https://www.rabbitmq.com/uri-spec.html
# http://www.ietf.org/rfc/rfc3986.txt
# https://tools.ietf.org/rfc/rfc5246.txt

RSpec.describe AMQ::URI do
describe ".parse" do
subject { described_class.parse(uri) }
Expand All @@ -12,7 +16,140 @@
end
end

context "path" do
describe "host" do
context "present" do
let(:uri) { "amqp://rabbitmq" }

it "parses host" do
expect(subject[:host]).to eq("rabbitmq")
end

context "%-encoded" do
let(:uri) { "amqp://r%61bbitmq:5672" }

it "parses host", pending: "Need to investigate, why URI module doesn't handle %-encoded host component..." do
expect(subject[:host]).to eq("rabbitmq")
end
end
end

if RUBY_VERSION >= "2.2"
context "absent" do
let(:uri) { "amqp://" }

# Note that according to the ABNF, the host component may not be absent, but it may be zero-length.
it "falls back to default nil host" do
expect(subject[:host]).to be_nil
end
end
end

if RUBY_VERSION < "2.2"
context "absent" do
let(:uri) { "amqp://" }

# Note that according to the ABNF, the host component may not be absent, but it may be zero-length.
it "raises InvalidURIError" do
expect { subject[:host] }.to raise_error(URI::InvalidURIError, /bad URI\(absolute but no path\)/)
end
end
end
end

describe "port" do
context "present" do
let(:uri) { "amqp://rabbitmq:5672" }

it "parses port" do
expect(subject[:port]).to eq(5672)
end
end

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

it "falls back to 5672 port" do
expect(subject[:port]).to eq(5672)
end
end

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

it "falls back to 5671 port" do
expect(subject[:port]).to eq(5671)
end
end
end
end

describe "username and passowrd" do
context "both present" do
let(:uri) { "amqp://alpha:beta@rabbitmq" }

it "parses user and pass" do
expect(subject[:user]).to eq("alpha")
expect(subject[:pass]).to eq("beta")
end
end

context "only username present" do
let(:uri) { "amqp://alpha@rabbitmq" }

it "parses user and falls back to nil pass" do
expect(subject[:user]).to eq("alpha")
expect(subject[:pass]).to be_nil
end

context "with ':'" do
let(:uri) { "amqp://alpha:@rabbitmq" }

it "parses user and falls back to "" (empty) pass" do
expect(subject[:user]).to eq("alpha")
expect(subject[:pass]).to eq("")
end
end
end

context "only password present" do
let(:uri) { "amqp://:beta@rabbitmq" }

it "parses pass and falls back to "" (empty) user" do
expect(subject[:user]).to eq("")
expect(subject[:pass]).to eq("beta")
end
end

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

it "falls back to nil user and pass" do
expect(subject[:user]).to be_nil
expect(subject[:pass]).to be_nil
end

context "with ':'" do
let(:uri) { "amqp://:@rabbitmq" }

it "falls back to "" (empty) user and "" (empty) pass" do
expect(subject[:user]).to eq("")
expect(subject[:pass]).to eq("")
end
end
end

context "%-encoded" do
let(:uri) { "amqp://%61lpha:bet%61@rabbitmq" }

it "parses user and pass" do
expect(subject[:user]).to eq("alpha")
expect(subject[:pass]).to eq("beta")
end
end
end

describe "path" do
context "present" do
let(:uri) { "amqp://rabbitmq/staging" }

Expand Down Expand Up @@ -44,45 +181,33 @@
end
end

context "with trailing escaped slash" do
context "with trailing %-encoded slash" do
let(:uri) { "amqp://rabbitmq/%2Fstaging" }

it "parses vhost as string with leading slash" do
expect(subject[:vhost]).to eq("/staging")
end
end
end

context "absent" do
let(:uri) { "amqp://rabbitmq" }
context "%-encoded" do
let(:uri) { "amqp://rabbitmq/%2Fstaging%2Fcritical%2Fsubsystem-a" }

it "fallbacks to default nil vhost" do
expect(subject[:vhost]).to be_nil
end
end
end

context "username and passowrd" do
context "present" do
let(:uri) { "amqp://alpha:beta@rabbitmq" }

it "parses user and pass" do
expect(subject[:user]).to eq("alpha")
expect(subject[:pass]).to eq("beta")
it "parses vhost as string with leading slash" do
expect(subject[:vhost]).to eq("/staging/critical/subsystem-a")
end
end
end

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

it "fallbacks to nil user and pass" do
expect(subject[:user]).to be_nil
expect(subject[:pass]).to be_nil
it "falls back to default nil vhost" do
expect(subject[:vhost]).to be_nil
end
end
end

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

Expand All @@ -97,7 +222,7 @@
context "absent" do
let(:uri) { "amqp://rabbitmq" }

it "fallbacks to default client connection parameters" do
it "falls back to default client connection parameters" do
expect(subject[:heartbeat]).to be_nil
expect(subject[:connection_timeout]).to be_nil
expect(subject[:channel_max]).to be_nil
Expand Down Expand Up @@ -136,7 +261,7 @@
context "absent" do
let(:uri) { "amqps://rabbitmq" }

it "fallbacks to default tls options" do
it "falls back to default tls options" do
expect(subject[:verify]).to be_falsey
expect(subject[:fail_if_no_peer_cert]).to be_falsey
expect(subject[:cacertfile]).to be_nil
Expand Down