alexedwards / scs

HTTP Session Management for Go
MIT License
2.09k stars 166 forks source link

Context again for a V3 version #115

Closed jum closed 2 years ago

jum commented 3 years ago

This is an attempt to have a clean pull request for a tentative V3 version that supports context as an argument for most calls. It also contains a GCP Firestore based Store implementation. This contains all the changes from my github.com/jum/scs fork, minus the scaffolding needed to make it run as a fork. This is intended be applied to a v3 branch of github.com/alexedwards/scs.

alexedwards commented 2 years ago

There's a lot in this PR that I really like. But I really don't want to make breaking changes to SCS or release a v3 unless there is really no other option.

What do you think of this as an alternative approach?

For example (completely untested, but hopefully you get the gist):

func (s *SessionManager) RenewToken(ctx context.Context) error {
    sd := s.getSessionDataFromContext(ctx)

    sd.mu.Lock()
    defer sd.mu.Unlock()

    if s.ContextStore != nil {
        err := s.ContextStore.Delete(ctx, sd.token)
        if err != nil {
            return err
        }
    } else {
        err := s.Store.Delete(sd.token)
        if err != nil {
            return err
        }
    }

    newToken, err := generateToken()
    if err != nil {
        return err
    }

    sd.token = newToken
    sd.deadline = time.Now().Add(s.Lifetime).UTC()
    sd.status = Modified

    return nil
}

If we went down this route, then we would need to document that if both ContextStore and Store are set, then the ContextStore is the one that will be used. But I think it would make it possible to move forward with integrating the Firebase store without any breaking changes, and also other store packages that would benefit from context support (PostgreSQL and MySQL in particular spring to mind) could be updated to implement the ContextStore interface over time.

I'd welcome your thoughts and feedback.

jum commented 2 years ago

I have to admit that I find the checking for ContextStore being nil quite ugly and error prone if it has to happen on every single possible call. My proposal instead would be to check for that condition at a prominent point that is called first (e.g. Load()), and if ContextStore is nil then make a StoreAdapter from the Store variable and put it into ContextStore.

I would guess that should cover most of the uses?

jum commented 2 years ago

I think with the recent changes in # https://github.com/alexedwards/scs/pull/120#issuecomment-975317558 this pull request is obsolete.