search

keratin / authn-go

Go client library for Keratin AuthN
https://github.com/keratin/authn-server
GNU Lesser General Public License v3.0
32 stars 10 forks source link

Inconsistent AccountID type #12

Open ppacher opened 4 years ago

ppacher commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.