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

3.4.5 has breaking API changes from 3.4.4 #463

Closed
natefinch opened this issue Sep 7, 2023 · 3 comments
Closed

3.4.5 has breaking API changes from 3.4.4 #463

natefinch opened this issue Sep 7, 2023 · 3 comments

Comments

@natefinch
Copy link

This PR #433 changed the signature of Conn's Close method to add an error to the return value. This is a breaking API change that should not have been made in a patch release.

We use this in some of our services at GitHub, and it makes confusing error messages and extra work for us when a patch version update of a dependency suddenly causes compiler errors.

It's not super hard to adjust callers to the new signature, but I hope the authors of this library will be more careful about such changes in the future. API signatures should not be changed willy-nilly.

@johnweldon
Copy link
Member

Hi @natefinch - long time no talk :)

You're right; I missed that in this MR - I've tried to be careful about breaking API changes, but this slipped by.

Thanks for raising this.

(//cc @go-ldap/committers )

@natefinch
Copy link
Author

Hi John! Good to bump into you. I thought I saw your name in the contributors list :)

@cpuschma
Copy link
Member

cpuschma commented Sep 8, 2023

The goal was to give the programmer the possibility to know that the call Close worked. In the implementation at that time there would have been a panic and a recover in the library without any feedback to the programmer, the message would have gone directly to the console.

Code that looks like this:

func main() {
	conn, _ := ldap.DialURL("ldap://localhost:389")
	_ = conn.Bind("test@localhost", "123456")
	defer conn.Close()
	
	searchResult, err := conn.Search(...)
	if err != nil {
		log.Fatalln(err)
	}
	fmt.Println(searchResult.Entries)
}

will still work, but linters will indicate that an error is returned if the library in go.mod is updated. On 2022-04-21 in #372 the minimum Go version was raised to 1.14 (see @johnweldon's comment #367 (comment)), which already has support for Go Modules in the format known today. So unless you explicitly use "go get -u" to update the library, the impact of this should be small. Additionally, since you wrote that this was done in a minor release: The go-ldap project is not really the most active one, also because the LDAP standard has changed little to nothing for about 10 years. I see the chances that we will ever make a new major release as extremely low to implement breaking changes, also because the decision to take the major release level "3" was due to the fact that the LDAPv3 standard is supported.

Nevertheless, you're absolutely right, changes to the signature should be avoided and kept to an absolute minimum if necessary.

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

No branches or pull requests

3 participants