Unable to use search result entries on size/time limit exceeded error post 2.5.1 #271
Comments
|
I understand the attraction of accessing partial results that were gleaned before an error occurred. Nevertheless, I think it's better to use the error to inform a change in approach to the API (i.e. modify the query to retrieve more targeted results, or reconfigure so that the size limits are increased) than to have fuzzy error/not-error results. Hard limits make for more robust API's, which I think is part of the spirit of the idiom that discourages partial values with errors. I won't plan on changing this unless I am convinced otherwise. |
If the server configuration is an input to your application and you want to verify its validity, I do not see how querying every user in the server to test presence of an attribute is practical. It looks like an alternative approach might be to set a paging control with a page size of 1 and finish after the first result. |
Interesting; I don't understand this fully - I'm not seeing the connection between the server configuration as an input, and specific attributes of individual records. Maybe a concrete example might help. I would imagine querying with paging and limiting your results to only the attributes you care about would almost always keep you far from most sane configuration limits. All in all, it sounds like paging is the right solution here as you mention. |
|
Suppose you are implementing the 'Test LDAP connection' button from https://docs.github.com/en/enterprise/2.21/admin/user-management/using-ldap#configuring-ldap-with-your-github-enterprise-server-instance The admin entering the configuration might not be very familiar with the server being used or may be attempting integration with a server that's not currently in the supported list. They have entered the 'User ID' field incorrectly and as part of the test you want to give them a clear error that this attribute is not present for the users in the configured server. How would you do this? The server may have a very large number of users and groups. I notice in their documentation that there is also a warning to make sure that the server supports paging, I would prefer universal support. |
|
What is the actual query being issued for the 'Test LDAP connection' button? It seems unlikely that any test query would generate such a large payload that it hits a normal LDAPResultSizeLimitExceeded error. It sounds like you're saying you have to query and return all users in the system in order to determine that an attribute is missing? That seems odd to me. |
|
|
|
Neither of those examples seems likely to cause timeout or size limit errors because the results are limited to one record ( I realize now that I think you're using the client library as a diagnostic/exploratory tool - which I think is perfectly reasonable, but not the same use case as a production "plumbing" library which is closer to my intent for this library. I'm happy to continue this dialogue outside of this issue (and reconsider the decision if necessary), but this isn't the best venue for coming to agreement or understanding. Feel free to email me at johnweldon4@gmail.com |
|
After reading your email @blakewedwards (thank you), and digging more into this issue I have a different perspective:
With my new perspective, I think the right approach to address this would be:
When an error is encountered, the library consumer can then continue to extract useful information from the error object. For example: if r, err := ldap.Search(req); err != nil {
if lerr, ok := (err).(*ldap.Error); ok {
packet := lerr.Packet
// do whatever with the raw packet
}
}@go-ldap/committers any comments, objections, feedback? |
* Extend ldap.Error to include reference to response Address #271 * Just return the result It's even more inelegant to try to wrap the limit errors and re-parse the packet for the result. We'll keep the most recent packet in the `Error` as a useful addition, but we'll just return the result in any error case even though it's counter to common Go idioms.
|
Resolved in version v3.2.2 |
I think commit 437615c#diff-059dc0366ae402fcb2b70107da19b22d removed the ability to access search result entries when a size or time limit is exceeded. This capability seems useful to me in circumstances such as testing configuration despite it being contrary to idiomatic behavior on error.
I.e.
The text was updated successfully, but these errors were encountered: