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

Added search with channels first draft #319

Closed
wants to merge 2 commits into from

Conversation

stefanmcshane
Copy link
Contributor

Adapted #172 to be up to date with go mod

Fixes #135

Marked as drafter as this is missing tests, and is currently only implemented in v3/ (go mods)
@dsheehan101 can you take a look? The example provided should work for testing against an LDAP server.

@stefanmcshane
Copy link
Contributor Author

Need to further update SearchWithChannel to use #232 style requests

@cpuschma
Copy link
Member

cpuschma commented Apr 6, 2021

Nice work! After looking through v3/search.go, this chunk stood out a bit:

entry := new(Entry)
entry.DN = packet.Children[1].Children[0].Value.(string)
for _, child := range packet.Children[1].Children[1].Children {
	attr := new(EntryAttribute)
	attr.Name = child.Children[0].Value.(string)
	for _, value := range child.Children[1].Children {
		attr.Values = append(attr.Values, value.Value.(string))
		attr.ByteValues = append(attr.ByteValues, value.ByteValue)
	}
	entry.Attributes = append(entry.Attributes, attr)
}

Maybe that's just me, or this is completly out of scope for this PR, but for better readability maybe we should think about moving this chunk into it's own corresponding function? (Maybe we can even use make with a set length/capacity since we can determine the amount of attributes through the children anyways :) )

@cpuschma
Copy link
Member

cpuschma commented Apr 6, 2021

I didn't do anything in Go for a ~2 months, but briefly put something together and run some benchmarks

attribute count (children per packet): 64
goos: windows
goarch: amd64
pkg: go-playground
cpu: Intel(R) Core(TM) i5-2500 CPU @ 3.30GHz
BenchmarkAttributesInline-4        22606             55768 ns/op           23033 B/op        455 allocs/op
BenchmarkAttributesFunc-4          58704             19540 ns/op           10752 B/op        129 allocs/op

I will continue to tinker with it and finalize it over the next few days ^^

@stefanmcshane
Copy link
Contributor Author

@xNevo I totally agree, and thanks for the feedback. The idea in the draft was to get this working, then refactor for readability.
This is duplicated code from the search function. I think that when I have sufficiently tested the SearchWithChannel function, I will refactor Search to reduce the duplication.

If you want to make a PR with the benchmarks added in, we will welcome it!

@inv2004
Copy link

inv2004 commented Aug 22, 2022

@stefanmcshane Any chance to move it from draft into master? It is important part of modern ldap

@michele-deluca
Copy link

Any news of this?

@t2y
Copy link
Contributor

t2y commented May 5, 2023

I heard this is related to #422. I might be able to help with something (refactoring, testing, etc.) if required.

t2y added a commit to t2y/ldap that referenced this pull request Jun 1, 2023
t2y added a commit to t2y/ldap that referenced this pull request Jun 1, 2023
@t2y
Copy link
Contributor

t2y commented Jun 1, 2023

I recreate a new PR including these changes. Could you review it?

t2y added a commit to t2y/ldap that referenced this pull request Jun 4, 2023
t2y added a commit to t2y/ldap that referenced this pull request Jun 4, 2023
t2y added a commit to t2y/ldap that referenced this pull request Jun 4, 2023
t2y added a commit to t2y/ldap that referenced this pull request Jun 4, 2023
t2y added a commit to t2y/ldap that referenced this pull request Jun 4, 2023
t2y added a commit to t2y/ldap that referenced this pull request Jun 4, 2023
cpuschma pushed a commit that referenced this pull request Jun 30, 2023
* feat: add search with channels inspired by #319

* refactor: fix to check proper test results #319

* refactor: fix to use unpackAttributes() for Attributes #319

* refactor: returns receive-only channel to prevent closing it from the caller #319

* refactor: pass channelSize to be able to controll buffered channel by the caller #319

* fix: recover an asynchronouse closing timing issue #319

* fix: consume all entries from the channel to prevent blocking by the connection #319

* feat: add initial search async function with channel #341

* feat: provide search async function and drop search with channels #319 #341

* refactor: lock when to call GetLastError since it might be in communication
@cpuschma cpuschma closed this Jul 3, 2023
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.

Return search results into channel
5 participants