search

shaj13 / go-guardian

Go-Guardian is a golang library that provides a simple, clean, and idiomatic way to create powerful modern API and web authentication.
MIT License
559 stars 56 forks source link

Consider changing ID() method in auth.Info interface to UserID()? #48

Closed leggettc18 closed 4 years ago

leggettc18 commented 4 years ago

I was trying to use this library in my project and ran into a naming collision of sorts. I have a few GORM models including a base Model and a User model. Model includes a few fields that get used in all of my other models, such as ID, created at, updated at, etc. My other models then have something like this.

type User struct {
    Model

    Email string
    HashedPassword []byte
}

What seemed appropriate to do at the time was add the functions from your auth.Info interface to my User model so that I could just pass my User into functions like Append, for example, or return my User model instead of a DefaultUser instance from my validateLogin function. That part works perfectly, but it came with other compromises.

One of the functions in the auth.Info interface is ID(). One of my fields in my base Model (and by extension my User model and indeed all my models) is also ID. Obviously I can change the name of the field, and that's what I've done for now, but I either name it Id which works but causes go-lint to yell at me, or name it something like ModelID which just feels... weird. I could also not use the base Model in my User model and manually add the necessary fields from the base Model, naming the id field UserID in the process, but that creates code duplication and means if I have to add to or edit the base Model I also most likely have to edit the User model. Not too big of a deal but a bit of a code smell.

What I would like to propose is renaming the ID() method on the auth.Info interface to UserID(), as it not only avoids this issue but also makes it match better with the UserName() method. Of course if you're privy to information that I'm not on why that wouldn't work I absolutely understand, and I can work around the issue with either one of the methods I described above or writing some functions to convert my User models to DefaultUser instances, but I figured it was worth having the discussion.

shaj13 commented 4 years ago

@leggettc18 agree, any other library may also conflict with your code when you try Composition in Golang, FYI, this minor change break changes in all the downstream projects. Right now there is no intent to accept the suggested changes. may we target this in V2.

as you said you can rename the field or try another approach like decouple the model from the info and use wrap mechanism. 

type User struct {
    Model

    Email string
    HashedPassword []byte
}

// UserInfo implemnts auth.Info
type UserInfo struct {
     user User
}

func (u *UserInfo) ID() string {
    return u.user.ID 
}
// ...  all auth.info methods
leggettc18 commented 4 years ago

Sounds good, yeah makes sense that there's no way around this that wouldn't be a breaking change. I just figured that having an ID field in Models would be common enough to consider the change for the future.

shaj13 commented 4 years ago

@leggettc18 v2 released, the ID method renamed to GetID(). closing this.