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

Inconsistent AccountID type #12

Open
ppacher opened this issue Oct 10, 2019 · 3 comments
Open

Inconsistent AccountID type #12

ppacher opened this issue Oct 10, 2019 · 3 comments

Comments

@ppacher
Copy link
Contributor

ppacher commented Oct 10, 2019

Hi,

first of all thanks for the awesome authn-server project. I just noticed that authn-go has some inconsistencies regarding the accountID type. In ImportAccount and int is returned while the other APIs all like to get a string. Though, the API it self returns the Account ID as a number.

I'd be happy to file a PR to fix that inconsistency but what is the type you actually want to expose to authn-go users? Since the API uses a number I guess an int would be more correct but that would break backwards-compatibility with v1.0.0.

@cainlevy
Copy link
Member

let's standardize on int to match the API.

much as it pains me, i think the golang way is to create new functions that return the correct type until cutting a v2 release. what does that mean here? 😬

@ppacher
Copy link
Contributor Author

ppacher commented Oct 11, 2019

That's a good question. If we start adding new methods that work with an int Account ID we might also consider adding support for context.Context. We could either add new functions to the existing client implementation or create a ClientV2 type (or similar). When adding functions to the existing client I'd follow the same approach golang did with the integration of context.Context.

func (c *Client) ImportAccountContext(ctx context.Context, user, pass string, locked bool) (int, error) {}
func (c *Client) GetAccountContext(ctx context.Context, accountID int) (*Account, error) {}
// ...

Though the name of the functions become somewhat awkward so I'd go with a new struct/interface definition. In the best case people using the new struct/interface type will not have too much breaking API changes when cutting a v2.

I'd also consider adding a warning to the README so users are aware of the new type and the upcoming type changes so they can either directly use int or at least know that they might need to perform database migrations when switching to v2.
What do you think?

@cainlevy
Copy link
Member

A new client with support for context.Context and improved types does seem like the best upgrade path. I'm not afraid of releasing a v2.0 but it'd be nice to have real-world usage of changes like this before proceeding.

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

2 participants