Skip to content

Bug fixes to 0.9.0 #168

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Bug fixes to 0.9.0 #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion lib/net/ldap/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,12 +562,13 @@ def search(args = nil)
result_pdu || OpenStruct.new(:status => :failure, :result_code => Net::LDAP::ResultCodeOperationsError, :message => "Invalid search")
end # instrument
ensure

# clean up message queue for this search
messages = message_queue.delete(message_id)

# in the exceptional case some messages were *not* consumed from the queue,
# instrument the event but do not fail.
unless messages.empty?
unless messages.nil? or messages.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Were you able to reproduce this problem reliably? Would love to add a test case for this scenario.

Use || instead of or here.

I tend to only use unless in cases with single conditions and switch to the more explicitly but easier to reason about if conditions otherwise. This would be:

if !messages.nil? && !messages.empty?

This is probably fine, though, since it's still a very simple case.

Copy link
Member

Choose a reason for hiding this comment

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

This scenario is possible, but it's really hard to test. message_queue will always return a hash, but the delete can return nil if message_id isn't in that hash. I tried testing this with stubs, but queued_read will loop forever trying to read additional pdf's, so this code is never reached. In real life, the socket would terminate at some point, and this ensure block would run, but I don't know how to simulate that in a test.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed style in f046564

instrument "search_messages_unread.net_ldap_connection",
message_id: message_id, messages: messages
end
Expand Down
12 changes: 10 additions & 2 deletions lib/net/ldap/filter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,10 +644,18 @@ def match(entry)
end

##
# Converts escaped characters (e.g., "\\28") to unescaped characters
# If the argument is a string, converts escaped characters (e.g., "\\28") to unescaped characters.
# If the argument is a number, just return as-is.
# Otherwise, an exception is thrown and the rhs argument is rejected.
# ("(").
def unescape(right)
right.gsub(/\\([a-fA-F\d]{2})/) { [$1.hex].pack("U") }
if defined? right.gsub
right.gsub(/\\([a-fA-F\d]{2})/) { [$1.hex].pack("U") }
elsif right.is_a? Fixnum
right.to_s
else
raise ArgumentError, "Did not know how to convert argument \"#{right}\" into the rhs of an LDAP filter"
end
Copy link
Member

Choose a reason for hiding this comment

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

I'd be comfortable with always calling right.to_s.gsub instead of introducing conditional branching.

Copy link
Member

Choose a reason for hiding this comment

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

Also, add a simple test to make sure this works! :)

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in b133b31

end
private :unescape

Expand Down