-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[Core] Update AccessToken
#36406
[Core] Update AccessToken
#36406
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
0a9912e
to
f7b332f
Compare
This change should avoid breaking people using The following usage scenarios are tested and still work as it did previously: access_token = AccessToken("token", 42)
assert access_token.token == "token"
assert access_token.expires_on == 42 Unpacking (not a common occurrence, at least in our SDKs) still works as before: token, expires_on = AccessToken("token, 42")
assert token == "token"
assert expires_on == 42
# Unpacking all values still works, but this assumes refresh_on is set:
token, expires_on, refresh_on = AccessToken("token", 42, 21)
assert token == "token"
assert expires_on == 42
assert refresh_on == 21 For the rare occurrence of people using numeric-indices to access values, this still works token = AccessToken("token", 42)
assert token[0] == "token"
assert token[1] == 42
assert token[:2] == ("token", 42) The Azure CLI does not import Azure Core's |
205f289
to
a1881e0
Compare
This allows adding a new field to AccessToken without breaking unpacking or index-based access of the previous credential. Signed-off-by: Paul Van Eck <[email protected]>
a1881e0
to
7cba5d5
Compare
@catalinaperalta , we should make sure that our breaking change tool catches additions of values to named tuples are flagged as breaking.... |
We will implement it in a non-breaking way which is not supposed to be caught. |
assert token == "token" | ||
assert expires_on == 42 | ||
assert refresh_on == 21 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also want to test the case:
token, expires_on = AccessToken(token="token", expires_on=42, refresh_on=21)
assert token == "token"
assert expires_on == 42
Earlier,
refresh_on
was added toAccessToken
as an optional property. This had unintended breaking side effects. Theazure-servicebus
package recently ran into issues when they were unpacking theget_token()
call:This yielded:
ValueError: too many values to unpack (expected 2)
.To avoid breaking users who might be unpacking like this, this change converts
AccessToken
into a DataClass. A NamedTuple and frozen DataClass are functionally the same for our purposes, but a DataClass provides more flexibility. Here, I override the__iter__
,__getitem__
, and__len__
methods to provide the same functionality as previously.Why we need
refresh_on
?In some cases, our current strategy of requesting a new token if it is within five minutes of expiration is not sufficient. Some customers have long-running jobs that span longer than five minutes and would like earlier refresh times to avoid authentication issues causing interruptions. In some cases, tokens can contain info on when it can/should be refreshed. We want to be able to use that info in our
BearerTokenCredentialPolicies
.Issue: #35473