allow pass context.Context to Store methods without breaking changes

[joesonw]

This would allow pass context.Context to Store methods without interrupting current non-Context version usage.

[alexedwards]

Thanks, I really like this solution. Minimal alterations to the existing code and no breaking changes :+1:

[jum]

Is there any information on how to use All() for the IterableStore interface with this new solution? I am attempting at getting gcloud firestore working and it appears to be not covered yet.

[jum]

Also, there should at least be an interface declaration anywhere (for documentation purposes) as to what methods a store implementation is supposed to support. The fact that these methods are hidden away deep in the source code does not appear to be making it easier.

[joesonw]

Iterate does not take a Context parameter. In order to pass a Context to All, there has to be a new method, like ‘IterateCtx`.

On Tue, Nov 23, 2021 at 10:22 PM Jens-Uwe Mager @.***> wrote:

Is there any information on how to use All() for the IterableStore interface with this new solution? I am attempting at getting gcloud firestore working and it appears to be not covered yet.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alexedwards/scs/pull/120#issuecomment-976606038, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMPI4OZ7WEZR7THZV2SHNDUNOPQ5ANCNFSM5GR6ZECA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

Thanks, Qiaosen Huang.

[joesonw]

Yeah, I should have documented it better and introduce a new interface for providers with methods supporting Context.

On Tue, Nov 23, 2021 at 10:26 PM Jens-Uwe Mager @.***> wrote:

Also, there should at least be an interface declaration anywhere (for documentation purposes) as to what methods a store implementation is supposed to support. The fact that these methods are hidden away deep in the source code does not appear to be making it easier.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alexedwards/scs/pull/120#issuecomment-976611329, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMPI4PKP3XBBJWU5CIAURTUNOP7ZANCNFSM5GR6ZECA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

Thanks, Qiaosen Huang.

[jum]

Well, I do have an implementation of AllCtx() in my upcoming google cloud firestore pull request. I hope that would satisfy any future developments in that area.

[jum]

I have one additional question here: How I am supposed to use my store that has only context methods as a store in SessionManager.Store? I did not find a method of casting a CtxStore as a Store that will not cause the go compiler to complain about missing methods that my context based store will never be able to have.

[jum]

Ok, I have found the answer, not too happy about what has to be done, but it works. See # https://github.com/jum/scs/commit/ee2886fc99c1d3206c1be25b4be9f3f22a6cf2ad

[joesonw]

Ok, I have found the answer, not too happy about what has to be done, but it works. See # jum@ee2886f

I would recommend something like following:

func (m *FireStore) Find(token string) ([]byte, bool, error) {
    return m.FindCtx(context.Background(), token)
}

[jum]

I did think about it before I added the panic's, and my conclusion was that it would hide a programming error, so better see it late at runtime than never. For example in on one occasion I would not see log output properly grouped in stack driver logging as there was a wrong context used.

[gandaldf]

I like @jum's "fail-fast" approach too, @alexedwards what do you think? I think would be useful to write some documentation about AllCtx() too, or I'm missing it?

[alexedwards]

@jum @gandaldf Leave the AllCtx() issue with me for a couple of days. I'm aware that it needs to be addressed, but I want to think through the best way to integrate it with session.Iterate(). Whatever happens, it will use the AllCtx() signature that already exists in the firestore store implementation.

[alexedwards]

@jum @gandaldf In commit https://github.com/alexedwards/scs/commit/dba928e4fe6e1c3a7f597652ef65ec836a7fe30a I've added AllCtx() to the CtxStore interface, and changed the signature of the Iterate() method to accept a context as the first argument and call internal doStoreAll() helper to similar to the others. So iteration should now work with the firestore package. I was happy to make these changes because, so far, nothing they touch has been part of a versioned release.

[gandaldf]

Ok! Thank you @alexedwards I would like to ask you if this wouldn't be a good solution:

type CtxStore interface {
    Store

    // DeleteCtx same as Store.Delete, except it takes a context.Context.
    DeleteCtx(ctx context.Context, token string) (err error)

    // FindCtx same as Store.Find, except it takes a context.Context.
    FindCtx(ctx context.Context, token string) (b []byte, found bool, err error)

    // CommitCtx same as Store.Commit, except it takes a context.Context.
    CommitCtx(ctx context.Context, token string, b []byte, expiry time.Time) (err error)
}

type CtxIterableStore interface {
    // AllCtx should return a map containing data for all active sessions (i.e.
    // sessions which have not expired). The map key should be the session
    // token and the map value should be the session data. If no active
    // sessions exist this should return an empty (not nil) map.
        // AllCtx same as IterableStore.All, expect it takes a context.Context.
    AllCtx(ctx context.Context) (map[string][]byte, error)
}

I'm asking it because, well... it seems more "symmetric" (I know it sounds stupid) with the non-context interface; anyway I'm not proposing it, I just want your feedback because I'm here to learn. Thank you.

[alexedwards]

@gandaldf I agree that would be more 'symmetrical' and my first iteration was pretty similar to that.

The reason that I ending up making it part of CtxStore is because going forward I'd expect all stores to support iteration and I couldn't think of a good reason to keep CtxIterableStore as a separate interface (ideally IterableStore would also be part of the Store interface but that's not possible with out breaking backward compatibility).

I don't think there's a right or wrong answer here. On one hand including it in CtxStore means that CtxStore more neatly describes all the methods that we expect a context-supporting store to implement. On the other hand, it's less symmetrical and that may cause some confusion when looking at the documentation.

I guess making them separate may also make sense in case there is ever a store where it is difficult to retrieve all records, and we don't want to support iteration for it.

If anyone else has a view on this, or can think of a good reason to favour one approach over the other please shout!

[gandaldf]

Pretty clear to me @alexedwards! I would update README file to write down a bit documentation about AllCtx(ctx context.Context)