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

Raise Net::LDAP::ConnectionRefusedError when new connection is refused. #213

Merged
merged 5 commits into from
Sep 18, 2015
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
2 changes: 1 addition & 1 deletion lib/net/ldap/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def initialize(server)
rescue SocketError
raise Net::LDAP::Error, "No such address or other socket error."
rescue Errno::ECONNREFUSED
raise Net::LDAP::Error, "Server #{server[:host]} refused connection on port #{server[:port]}."
raise Net::LDAP::ConnectionRefusedError, "Server #{server[:host]} refused connection on port #{server[:port]}."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I came to think no need to wrap Errno::ECONNREFUSED in this situation.
@mtodd @jch How do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I prefer not wrapping the error because Errno::ECONNREFUSED is more descriptive and appropriate. In fact, I'd rather we didn't rescue any wrap any of the errors in this block. However, this would be a breaking change in the API and require changes to the caller. We're not at 1.0, but this does seem like a nasty surprise. How do you feel about making a PR that shows deprecation warnings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you feel about making a PR that shows deprecation warnings?

sounds great to me 👍

rescue Errno::EHOSTUNREACH => error
raise Net::LDAP::Error, "Host #{server[:host]} was unreachable (#{error.message})"
rescue Errno::ETIMEDOUT
Expand Down
17 changes: 16 additions & 1 deletion lib/net/ldap/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,22 @@ class Error < StandardError; end

class AlreadyOpenedError < Error; end
class SocketError < Error; end
class ConnectionRefusedError < Error; end
class ConnectionRefusedError < Error;
def initialize(*args)
warn_deprecation_message
super
end

def message
warn_deprecation_message
super
end

private
def warn_deprecation_message
warn "Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When an user use this exception, this will show this exception will be removed. how about this? @jch

Copy link
Member

Choose a reason for hiding this comment

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

This looks good. Could you add a test that captures stderr and asserts that this message is being printed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, @jch ! I did it.

end
end
class NoOpenSSLError < Error; end
class NoStartTLSResultError < Error; end
class NoSearchBaseError < Error; end
Expand Down
18 changes: 18 additions & 0 deletions test/test_ldap_connection.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
require_relative 'test_helper'

class TestLDAPConnection < Test::Unit::TestCase
def capture_stderr
stderr, $stderr = $stderr, StringIO.new
yield
$stderr.string
ensure
$stderr = stderr
end

def test_unresponsive_host
assert_raise Net::LDAP::Error do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
Expand All @@ -14,6 +22,16 @@ def test_blocked_port
end
end

def test_connection_refused
flexmock(TCPSocket).should_receive(:new).and_raise(Errno::ECONNREFUSED)
stderr = capture_stderr do
assert_raise Net::LDAP::ConnectionRefusedError do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
end
end
assert_equal("Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead.\n", stderr)
end

def test_raises_unknown_exceptions
error = Class.new(StandardError)
flexmock(TCPSocket).should_receive(:new).and_raise(error)
Expand Down