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

Refactor DirSync search process #458

Merged
merged 3 commits into from
Sep 6, 2023
Merged

Conversation

t2y
Copy link
Contributor

@t2y t2y commented Aug 15, 2023

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)

search.go Show resolved Hide resolved
examples_test.go Show resolved Hide resolved
Comment on lines +638 to +644
// NewRequestControlDirSync returns a dir sync control
func NewRequestControlDirSync(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a good name in RFC.

Controls sent by clients are termed 'request controls', and those sent by servers are termed 'response controls'.

https://datatracker.ietf.org/doc/html/rfc4511#section-4.1.11

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the function has been exposed in the latest release and is therefore considered immutable. I suggest an alias instead with the same function signature and add a mark to the current one to mark it as deprecated.

@t2y
Copy link
Contributor Author

t2y commented Aug 29, 2023

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

@t2y
Copy link
Contributor Author

t2y commented Sep 6, 2023

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

Copy link
Member

@cpuschma cpuschma left a comment

Choose a reason for hiding this comment

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

No need to be passive aggressive. In the end this is an open source project where nobody gets paid for writing or reviewing code. I'm immensely happy about the PRs, I'm really, but I'd ask you to keep that in mind please.

Anyways, the PR looks good and moves the code to be more in line with the general design. Could you please address the comment regarding renaming the already exposed function?

Comment on lines +638 to +644
// NewRequestControlDirSync returns a dir sync control
func NewRequestControlDirSync(
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the function has been exposed in the latest release and is therefore considered immutable. I suggest an alias instead with the same function signature and add a mark to the current one to mark it as deprecated.

search.go Show resolved Hide resolved
@t2y
Copy link
Contributor Author

t2y commented Sep 6, 2023

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

@cpuschma cpuschma merged commit 5a8863b into go-ldap:master Sep 6, 2023
@t2y t2y deleted the dirsync-async branch September 6, 2023 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants