Skip to content

Fix size option for ldap.search method #140

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

Jump to bottom
Merged
merged 7 commits into from
Oct 21, 2014
Merged

Fix size option for ldap.search method #140

merged 7 commits into from
Oct 21, 2014

Conversation

dzaporozhets
Copy link
Contributor

This PR fixes issue #75.

Problem

When I search for ldap users and use size option I got nil result if amount of users in LDAP more than number provided in size options. So if you want to get first 50 LDAP users you get nil because its more than 50 users :)

Describing problem in more simple way it looks like this for me:

Given I visit a shop to buy some beer
When I ask cashier to sell me 1 bottle of beer
Then he responds me: Size limit exceeded. 1297 beers available, 1 beer requested
And I return home with no beer

Solution

Just check for 4 result_code from LDAP and return result in this case too.

Possible improvemens

I dont like this 0 and 4 magic numbers but I afraid to touch code unrelated to my fix. If you ask I can add 4 as constant like LDAP_SIZE_LIMIT_EXCEEDED

Thank you!!!

@dzaporozhets
Fix size option for ldap.search method
2a2bf1a 
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
@dzaporozhets
Fix test for ldap.search return codes
40bcac3 
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
@dzaporozhets
Copy link
Contributor Author

@jch I will do :)

@jch
Copy link
Member

jch commented Oct 21, 2014

I dont like this 0 and 4 magic numbers but I afraid to touch code unrelated to my fix. If you ask I can add 4 as constant like LDAP_SIZE_LIMIT_EXCEEDED

👍 for a more readable constant. It's already defined higher in this file at https://github.com/ruby-ldap/ruby-net-ldap/blob/master/lib/net/ldap.rb#L329. You can do:

ResultStrings.value("Size Limit Exceeded")

I agree with you that I'd prefer the constant, but in the interest of keeping this PR focused, I think reusing the existing lookup is fine.

@jch jch self-assigned this Oct 21, 2014
@dzaporozhets
Remove unnecessary assert in ldap.search return code test
75cda5a 
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
@dzaporozhets
Avoid using magic number in ldap.search
9adc587 
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
@dzaporozhets
Copy link
Contributor Author

@jch I removed magic number and unnecessary assert. Anything else I can do?

@dzaporozhets
Avoid magic number in success case of ldap search
e8e0e28 
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
jch referenced this pull request Oct 21, 2014
@mtodd
Block specificity strikes again
a47e357
jch referenced this pull request Oct 21, 2014
@mtodd
Skip failing searchSize integration test, add explanatory test
f638ea7 
This disables a failing test that won't pass without changing the
behavior of Net::LDAP#search (and Net::LDAP::Connection#search).

This is due to sizeLimitExceeded being treated as a failure instead
of a deliberate termination of search results. Instead of returning
the partial results, we currently return none.

cc @jch @schaary
mtodd
mtodd reviewed Oct 21, 2014
View reviewed changes
lib/net/ldap.rb
when ResultStrings.key("Success")
# everything good
result_set
when ResultStrings.key("Size Limit Exceeded")
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 like to use a constant in this case. I didn't see one already defined, though, but I'd be in favor of one (or a few) for this.

Copy link
Member

Choose a reason for hiding this comment

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

👍 We can define the constants, and then define ResultStrings values in terms of those constants.

Copy link
Member

Choose a reason for hiding this comment

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

Heh, not to contradict @jch, we can worry about this in a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@mtodd
Copy link
Member

mtodd commented Oct 21, 2014

Thanks for doing this! It was also on my shortlist :)

One final thing: would like to see an assertion for the returned results matching the results yielded! A new test in https://github.com/randx/ruby-net-ldap/blob/fix-size-option-in-search/test/integration/test_search.rb would be perfect.

@dzaporozhets
Add additional integration test for ldap.search with result
b0bc215 
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
@dzaporozhets
Fix test_search_with_size case
f3a2515 
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
@dzaporozhets
Copy link
Contributor Author

Tests added. Build is green 😄. Anything else?

@dzaporozhets
Copy link
Contributor Author

Also thank you for fast review!

@mtodd
Copy link
Member

mtodd commented Oct 21, 2014

👍 diff looks good to me. @jch?

jch added a commit that referenced this pull request Oct 21, 2014
@jch
Merge pull request #140 from randx/fix-size-option-in-search
3320248 
Fix size option for ldap.search method
@jch jch merged commit 3320248 into ruby-ldap:master Oct 21, 2014
@jch
Copy link
Member

jch commented Oct 21, 2014

@randx @mtodd 🍻

mtodd
mtodd reviewed Oct 21, 2014
View reviewed changes
test/test_ldap.rb
assert_equal [entry], result
assert_equal [entry], payload[:result]
assert_equal "(uid=user1)", payload[:filter]
assert_equal result.size, payload[:size]
Copy link
Member

Choose a reason for hiding this comment

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

👌 perfect!

@dzaporozhets
Copy link
Contributor Author

Awesome! Thank you @jch and @mtodd. It was cool

@dzaporozhets
Copy link
Contributor Author

Is it planned any gem release soon?

@mtodd
Copy link
Member

mtodd commented Oct 21, 2014

@randx yep! See #125. No firm date set, but I expect soon.

@mtodd mtodd mentioned this pull request Oct 22, 2014
Closed
jch pushed a commit that referenced this pull request Oct 22, 2014
remove skip fixed by #140
7baeda3
astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015
@jch
Merge pull request ruby-ldap#140 from randx/fix-size-option-in-search
853aeaa 
Fix size option for ldap.search method
astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015
remove skip fixed by ruby-ldap#140
23fa271
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jch jch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dzaporozhets @jch @mtodd