Skip to content

Extract paged search result cookie handling logic #185

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

Closed
wants to merge 1 commit into from
Closed

Extract paged search result cookie handling logic #185

wants to merge 1 commit into from

Conversation

mynameisrufus
Copy link
Contributor

Extracted the rfc cookie find code in search into its own method to fix the
following offense that broke the build:

lib/net/ldap/connection.rb:379:3: C: Assignment Branch Condition size for search
is too high.

@jch
Copy link
Member

jch commented Jan 9, 2015

@mynameisrufus I thought rubocop was not going to cause build failures for existing code?

I think this could be a worthwhile refactoring, but I'm wary of doing a simple method extraction like this. Does this new method make sense standalone? Perhaps it does, but I think the method comment needs to be updated with a better explanation. Otherwise, we're just splitting and hiding behavior in more places and making the code harder to reason about.

jch pushed a commit that referenced this pull request Jan 16, 2015
Unblocks #183. There is a
proposed fix in #185, but it's
not ready yet.
@mynameisrufus
Copy link
Contributor Author

Sorry for late reply, I am on holiday.

I will try and figure out what this bit of code is for and change documentation.

@mynameisrufus
Copy link
Contributor Author

"we're just splitting and hiding behavior in more places and making the code harder to reason about" or is this encapsulating behaviour and making it easier to reason about?

@jch
Copy link
Member

jch commented Jan 19, 2015

@mynameisrufus I'm not saying your refactoring is hiding behavior. From your method name, it sounds like a good candidate for extraction, but the method comment should also work stand alone. The current description does not explain what a search cookie is, assumes there is a loop going on, and is generally tied to the caller. If I came across this, I would be confused:

  • When we get here, we have seen a type-5 response. If there is no

  • error AND there is an RFC-2696 cookie, then query again for the next

  • page of results. If not, we're done. Don't screw this up or we'll

  • break every search we do.

  • Noticed 02Sep06, look at the read_ber call in this loop, shouldn't

  • that have a parameter of AsnSyntax? Does this just accidentally

  • work? According to RFC-2696, the value expected in this position is

  • of type OCTET STRING, covered in the default syntax supported by

  • read_ber, so I guess we're ok.

@mynameisrufus
Copy link
Contributor Author

I have removed Austin Ziegler's 2010 code comments (8e9a8a4) and replaced it with a slimed down version with a link to the RFC, the RFC actually explains things, no point in repeating in comment.

@jch
Copy link
Member

jch commented Jan 27, 2015

👍 nice. @mtodd care to chime in?

break unless more_pages
cookie = next_search_result_cookie(result_pdu.result_code, controls)
rfc2696_cookie[1] = cookie unless cookie.nil?
break if cookie.nil?
Copy link
Member

Choose a reason for hiding this comment

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

Could reorder this and skip the unless cookie.nil?:

cookie = next_search_result_cookie(result.pdu.result_code, controls)
break if cookie.nil?
rfc2696_cookie[1] = cookie

@mtodd
Copy link
Member

mtodd commented Jan 29, 2015

Only minor nits, this extraction looks great! Thanks @mynameisrufus!

@mtodd mtodd changed the title Fix branch assignment offense in connection.rb Extract paged search result cookie handling logic Jan 29, 2015
Extracted the rfc cookie find code in searh into its own method to fix the
following offense that broke the build:

```
lib/net/ldap/connection.rb:379:3: C: Assignment Branch Condition size for search
is too high.
```
@mynameisrufus
Copy link
Contributor Author

@mtodd made the changes, ready for merge 🐐

@mtodd
Copy link
Member

mtodd commented Jan 29, 2015

@mynameisrufus yep, looks great! I have some time tomorrow to do some manual testing (because I don't think our existing test suite covers paged search results) before merging.

astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015
Unblocks ruby-ldap#183. There is a
proposed fix in ruby-ldap#185, but it's
not ready yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants