Skip to content

Conversation

@koushik2506
Copy link
Contributor

@koushik2506 koushik2506 commented Jul 15, 2018

Hi,
So here is a PR for #177.
I have a added a NewErrorWithDN function in errors.go which creates a Error object with MatchedDN populated.
Corresponding changes to pass on this error in operations
Added a test in ldap_test with a search on a wrong base to print out the matchedDN error.

error.go Outdated
}

// NewError creates an LDAP error with the given code and underlying error
func NewError(resultCode uint8, err error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just change this and drop NewErrorWithDN() to have this PR only go into the upcoming v3... @liggitt what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Error structure, gets a MatchedDN populated, only if the values returned by getLDAPResultCode() is put into it. If you drop NewErrorWithDN(), then NewError() would have to take 3 arguments. This would make code blocks like:

packetResponse, ok := <-msgCtx.responses
if !ok {
    return NewError(ErrorNetwork, errors.New("ldap: response channel closed")
}

Needing an empty string argument for MatchedDN.

Can we make getLDAPResultCode into something like GetLDAPError which will return the full Error object and do a err != nil check. So places like this:

if packet.Children[1].Tag == ApplicationAddResponse {
    resultCode, resultDescription, matchedDN := getLDAPResultCode(packet)
    if resultCode != 0 {
        return NewErrorWithDN(resultCode, errors.New(resultDescription), matchedDN)
    }
 }

Would become

if packet.Children[1].Tag == ApplicationAddResponse {
    err := GetLDAPError(packet)
    if err != nil {
        return err
    }
 }

Then we can retain NewError as is, and do away with NewErrorWithDN().

Copy link
Member

Choose a reason for hiding this comment

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

I like this suggestion - do you want to update this PR accordingly?

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