This PR (HEAD: 387e226) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/oauth2/+/192798 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

This PR (HEAD: 6d45fe3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/oauth2/+/192798 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Message from Frank Harding:

Patch Set 8:

Adding Brad Fitzpatrick as a reviewer since it looks like he deals with a lot of the golang/oauth2 CLs, let me know if I should have added someone else. Thank you!

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Brad Fitzpatrick:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Brad Fitzpatrick:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Brad Fitzpatrick:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

And to be clear I totally agree that I would love if this was possible without changing this library, I just don't think it is without re-implementing the behavior of the unexported tokenRefresher. Would love to be wrong about that.

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Brad Fitzpatrick:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

If you want to get a fresh un-cached token, call:

func (c *Config) TokenSource(ctx context.Context, t *Token) TokenSource {

... with a nil Token.

Then call Token() on the returned TokenSource?

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

If you want to get a fresh un-cached token, call:

func (c *Config) TokenSource(ctx context.Context, t *Token) TokenSource {

... with a nil Token.

Then call Token() on the returned TokenSource?

That's what I meant in the commit message by: "You basically have to use the config to create a whole new TokenSource (or HTTP client) which means you have to keep the config around for the entire life of your code rather than just generating a TokenSource that can refresh whenever you want it at the start." My point was just that you can't actually wrap a TokenSource and refresh it whenever you want since all the TokenSources are using ReuseTokenSource.

If you're fine with having consumers do that then I guess that's fine (and sorry to have wasted your time), I just thought it made sense to allow users to refresh TokenSources at their leisure if they need to (especially considering the use cases I mentioned).

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Brad Fitzpatrick:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

If you want to get a fresh un-cached token, call:

func (c *Config) TokenSource(ctx context.Context, t *Token) TokenSource {

... with a nil Token.

Then call Token() on the returned TokenSource?

That's what I meant in the commit message by: "You basically have to use the config to create a whole new TokenSource (or HTTP client) which means you have to keep the config around for the entire life of your code rather than just generating a TokenSource that can refresh whenever you want it at the start." My point was just that you can't actually wrap a TokenSource and refresh it whenever you want since all the TokenSources are using ReuseTokenSource.

If you're fine with having consumers do that then I guess that's fine (and sorry to have wasted your time), I just thought it made sense to allow users to refresh TokenSources at their leisure if they need to (especially considering the use cases I mentioned).

Gotcha. Sorry, I didn't exactly follow earlier.

But I don't think it's as bad as you made it sound... you said that you "have to keep the config around", but that's not any different from today. The returned TokenSource today already retains a pointer to the *Config. Any TokenSource you implement would also do so. But downstream users of either API don't need to care about that detail... they'd just use a TokenSource (or Transport/Client using said TokenSource).

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

If you want to get a fresh un-cached token, call:

func (c *Config) TokenSource(ctx context.Context, t *Token) TokenSource {

... with a nil Token.

Then call Token() on the returned TokenSource?

That's what I meant in the commit message by: "You basically have to use the config to create a whole new TokenSource (or HTTP client) which means you have to keep the config around for the entire life of your code rather than just generating a TokenSource that can refresh whenever you want it at the start." My point was just that you can't actually wrap a TokenSource and refresh it whenever you want since all the TokenSources are using ReuseTokenSource.

If you're fine with having consumers do that then I guess that's fine (and sorry to have wasted your time), I just thought it made sense to allow users to refresh TokenSources at their leisure if they need to (especially considering the use cases I mentioned).

Gotcha. Sorry, I didn't exactly follow earlier.

But I don't think it's as bad as you made it sound... you said that you "have to keep the config around", but that's not any different from today. The returned TokenSource today already retains a pointer to the *Config. Any TokenSource you implement would also do so. But downstream users of either API don't need to care about that detail... they'd just use a TokenSource (or Transport/Client using said TokenSource).

That's true. You would just have to fetch a new TokenSource from the config whenever you want to refresh the token rather than using the underlying unexported tokenRefresher and telling that to refresh. I just think that's a bit weird, but you're right that that's doable with the current API. I think I was just a little hung up on the fact that the way the API works right now already implements token refreshing, and it seems a bit weird to have to create new token sources every time you want to refresh the token rather than just being able to tell the underlying tokenRefresher to return a new token. For example, if there was a way to get the tokenSource created by oauth2.Config.TokenSource before it's wrapped with ReuseTokenSource, this would be a lot easier. Or, if oauth2.Config had a Token method that always returned a new token (so oauth2.Config itself was a TokenSource without refreshiing).

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

If you want to get a fresh un-cached token, call:

func (c *Config) TokenSource(ctx context.Context, t *Token) TokenSource {

... with a nil Token.

Then call Token() on the returned TokenSource?

That's what I meant in the commit message by: "You basically have to use the config to create a whole new TokenSource (or HTTP client) which means you have to keep the config around for the entire life of your code rather than just generating a TokenSource that can refresh whenever you want it at the start." My point was just that you can't actually wrap a TokenSource and refresh it whenever you want since all the TokenSources are using ReuseTokenSource.

If you're fine with having consumers do that then I guess that's fine (and sorry to have wasted your time), I just thought it made sense to allow users to refresh TokenSources at their leisure if they need to (especially considering the use cases I mentioned).

Gotcha. Sorry, I didn't exactly follow earlier.

But I don't think it's as bad as you made it sound... you said that you "have to keep the config around", but that's not any different from today. The returned TokenSource today already retains a pointer to the *Config. Any TokenSource you implement would also do so. But downstream users of either API don't need to care about that detail... they'd just use a TokenSource (or Transport/Client using said TokenSource).

That's true. You would just have to fetch a new TokenSource from the config whenever you want to refresh the token rather than using the underlying unexported tokenRefresher and telling that to refresh. I just think that's a bit weird, but you're right that that's doable with the current API. I think I was just a little hung up on the fact that the way the API works right now already implements token refreshing, and it seems a bit weird to have to create new token sources every time you want to refresh the token rather than just being able to tell the underlying tokenRefresher to return a new token. For example, if there was a way to get the tokenSource created by oauth2.Config.TokenSource before it's wrapped with ReuseTokenSource, this would be a lot easier. Or, if oauth2.Config had a Token method that always returned a new token (so oauth2.Config itself was a TokenSource without refreshiing).

And in that case you would still have to keep the oauth2.Config around, but you wouldn't be creating new TokenSources every time you want to refresh the token. That's not too much better, so I think you're right that it's probably not worth changing the API to enable doing that. I'll close this PR/CL.

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!

Message from Frank Harding:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Patch Set 8:

Sorry, this is more API than I want to add to this package.

Alright. I think it's a fairly small change to enable a lot of realistic use cases, but that's fair enough. I kind of expected a non-insignificant (even non-breaking) API change to golang/oauth2 wouldn't be super welcomed. Is there any way you could see implementing this functionality with a smaller API, or do you think it's just not worth making any changes to the API to enable these use cases? While hopefully checking whether the token is valid should be good enough to know when we need to refresh the token, that's not realistic for a lot of APIs, and it makes any code that needs to implement that functionality really awkward since like I mentioned the user will need to keep around their config and create new clients or token sources everytime they want to refresh the token (rather than just being able to use a token source that allows you to refresh whenever you want).

Sorry, I don't understand the problem enough probably.

But the package is designed by take a very simple interface: TokenSource (https://godoc.org/golang.org/x/oauth2#TokenSource).

So seems like you can just put all of this newly proposed code into some new package that provides a different TokenSource.

It's okay to compose a TokenSource out of other TokenSources.

This is my understanding, it could be wrong:

If you use oauth2.Config to generate a TokenSource or an HTTP Client it will use ReuseTokenSource under the hood (which wraps the unexported tokenRefresher). This means it will only refresh the token when the token is invalid. Otherwise, it will keep returning the same token. If you need to invalidate the token for any reason (I gave some reasons in the commit message) despite it being "valid" you need a way to refresh the token. And because ReuseTokenSource is using the unexported tokenRefresher under the hood, there's effectively no way for a user to create their own token source that they can refresh whenever they want. If they want to do that, they have to keep the oauth2.Config around and have it return a new TokenSource or Client.

That's why I said compose what you want. You can write your own TokenSource wrapper that does the validation on top of whatever lower one (even if that lower one is a ReuseTokenSource of something else). You could even then wrap your validating one with another ReuseTokenSource, or your own implementation of that sort of behavior. It's only a few lines.

The point is: I see no fundamental reason you can't put this in a new package. I don't want to add complexity in the base package if not required.

Also, I need to make some changes to this CL to actually make it accommodate this fully, because I just realized my CL suffers from the same issues. There's still no way to wrap what's returned from oauth2.Config.TokenSource and refresh the token whenever you want since the underlying ReuseTokenSource is going to keep returning the stale token even if you ask for a new one. I guess oauth2.Config would have to be updated with a new TokenSource method that accepts a ValidFunc. On the plus side if we added that we could unexport CustomTokenSource, so all we would be adding to the API is a new oauth2.Config method and an exported type signature for ValidFunc.

I'm confused, how are you going to wrap ReuseTokenSource to refresh the token if it keeps returning a stale token as long as the token it's holding onto returns that it's valid? Maybe I'm missing something?

If you want to get a fresh un-cached token, call:

func (c *Config) TokenSource(ctx context.Context, t *Token) TokenSource {

... with a nil Token.

Then call Token() on the returned TokenSource?

That's what I meant in the commit message by: "You basically have to use the config to create a whole new TokenSource (or HTTP client) which means you have to keep the config around for the entire life of your code rather than just generating a TokenSource that can refresh whenever you want it at the start." My point was just that you can't actually wrap a TokenSource and refresh it whenever you want since all the TokenSources are using ReuseTokenSource.

If you're fine with having consumers do that then I guess that's fine (and sorry to have wasted your time), I just thought it made sense to allow users to refresh TokenSources at their leisure if they need to (especially considering the use cases I mentioned).

Gotcha. Sorry, I didn't exactly follow earlier.

But I don't think it's as bad as you made it sound... you said that you "have to keep the config around", but that's not any different from today. The returned TokenSource today already retains a pointer to the *Config. Any TokenSource you implement would also do so. But downstream users of either API don't need to care about that detail... they'd just use a TokenSource (or Transport/Client using said TokenSource).

That's true. You would just have to fetch a new TokenSource from the config whenever you want to refresh the token rather than using the underlying unexported tokenRefresher and telling that to refresh. I just think that's a bit weird, but you're right that that's doable with the current API. I think I was just a little hung up on the fact that the way the API works right now already implements token refreshing, and it seems a bit weird to have to create new token sources every time you want to refresh the token rather than just being able to tell the underlying tokenRefresher to return a new token. For example, if there was a way to get the tokenSource created by oauth2.Config.TokenSource before it's wrapped with ReuseTokenSource, this would be a lot easier. Or, if oauth2.Config had a Token method that always returned a new token (so oauth2.Config itself was a TokenSource without refreshiing).

And in that case you would still have to keep the oauth2.Config around, but you wouldn't be creating new TokenSources every time you want to refresh the token. That's not too much better, so I think you're right that it's probably not worth changing the API to enable doing that. I'll close this PR/CL.

... and thank you for having the patience to talk through this :)

Please don’t reply on this GitHub thread. Visit golang.org/cl/192798.
After addressing review feedback, remember to publish your drafts!