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

All storage interfaces have a Context except for GetClient #161

Closed
michaelboke opened this issue May 16, 2017 · 3 comments
Closed

All storage interfaces have a Context except for GetClient #161

michaelboke opened this issue May 16, 2017 · 3 comments

Comments

@michaelboke
Copy link
Contributor

All the storage interfaces have a Context except for the Storage.GetClient (fosite.Storage / fosite.ClientManager).

function signature GetClient(id string) (fosite.Client, error)

And what is the deal with the separate ClientManager interface?
Seems like a strange duck while there is always a Storage interface defined.

Is this an candidate for a refactor?
GetClient(ctx context.Context, id string) (fosite.Client, error)

Let me know i could do the refactor for you

@aeneasr
Copy link
Member

aeneasr commented May 16, 2017

The client manager is it's own interface so it's easier to compose stores later on. However, adding the context here could be benefitial for some cases.

On the other hand, the context is never used anywhere in the library. I'm not sure if we should keep it at all. What do you think?

@michaelboke
Copy link
Contributor Author

I came across this while implementing the storages with a sql backend. Since go 1.8 the DB has
context support for cancelable queries. I could now implement that for all except the client storage.

I would say atleast add it to interface to keep it in line with all the other storage interfaces.

And the ClientManager interface does not really describe a manager functions, it still looks like a storage interface. There is just one getter inside.

@aeneasr
Copy link
Member

aeneasr commented May 16, 2017

Yeah right, I forgot that go 1.8 added that - so let's keep the contexts and I'd also appreciate an update to the GetClient method with context :)

budougumi0617 added a commit to budougumi0617/fosite that referenced this issue May 10, 2019
Closes ory#161

Signed-off-by: Michael Boke <michael.boke@gmail.com>
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