go-ldap / ldap

Basic LDAP v3 functionality for the GO programming language.
Other
2.25k stars 355 forks source link

Refactor DirSync search process #458

Closed t2y closed 1 year ago

t2y commented 1 year ago

I'm now developing an application to connect the Windows AD server and get entries. Thanks to #436, I can use DirSync() method to search. As my requirement, I have to handle several ten thousand entries. This is not so big, but waste of memory at once. I noticed #440 provides a search asynchronous feature, so I can search without getting all entries at once.

I looked into the code in DirSync() method and tested it. I refactored some minor changes for maintainability.

About DirSyncAsync() method

I made it as below. It works in my environment.

// DirSyncDirSyncAsync performs a search request and returns all search results
// asynchronously. This is efficient when the server returns lots of entries.
func (l *Conn) DirSyncAsync(
    ctx context.Context, searchRequest *SearchRequest, bufferSize int,
    flags, maxAttrCount int64, cookie []byte,
) Response {
    control := NewControlDirSyncForEncode(flags, maxAttrCount, cookie)
    searchRequest.Controls = append(searchRequest.Controls, control)
    r := newSearchResponse(l, bufferSize)
    r.start(ctx, searchRequest)
    return r
}

However, I can implement this by myself like this. So, what do you think about whether we provide this short-cut helper or not? I made this to confirm dirsync search works with asynchronous, so DirSyncAsync() method is not important for me.

control := NewControlDirSyncForEncode(flags, maxAttrCount, cookie)
searchRequest.Controls = append(searchRequest.Controls, control)
r := SearchAsync(ctx, searchRequest, 64)
t2y commented 1 year ago

@cpuschma Could you review this? I don't have a strong opinion. I just want to confirm other opinions.

t2y commented 1 year ago

Is anyone interested in this PR? I'm glad to determine whether we provide DirSyncAsync or not.

t2y commented 1 year ago

@cpuschma Welcome back! Thank you for your review. I added a migration function with the deprecated message.