-
Notifications
You must be signed in to change notification settings - Fork 119
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
fix: support Google OAuth Client credentials without expiration time #1117
fix: support Google OAuth Client credentials without expiration time #1117
Conversation
8a4888b
to
515cbfc
Compare
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.
Looks like these are null pointer exceptions
515cbfc
to
7d1b712
Compare
@shubha-rajan for some reason, I cannot run the unit tests locally (had issues before, but I also remember having them running fine). I also tried Anyway, I fixed the issue from the build. |
credential.getExpirationTimeMilliseconds() != null | ||
? new Date(credential.getExpirationTimeMilliseconds()) | ||
: null |
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.
nit: can we use an optional here instead of this ternary?
something like:
Optional<Long> expTime = Optional.ofNullable(credential.getExpirationTimeMilliseconds());
AccessToken accessToken = new AccessToken(
credential.getAccessToken(),
expTime.isPresent()
? new Date(expTime.get())
: null
);
Looks like there are still errors. The fix should just be to add a check before L398 to check whether |
7d1b712
to
df7934a
Compare
All right, I pushed a fix, let's see. |
Looks like tests are passing this time. Did you see my note about using an Optional? It might be a nit, but I think it makes the code cleaner. |
df7934a
to
38b6300
Compare
Ah, I already dragged in jsr305 for nullability, but Optional makes more sense indeed (writing Kotlin for some time already). Will have a look at it. |
38b6300
to
3c3666c
Compare
Applied your feedback! 🤞 |
Change Description
Small fix for #1097 (cf024eb) to also allow credentials without an expiration time.
I only tested the previous fix with my own application default credentials, but it seems some tokens don't have the expiration time as it can be
null
.Checklist
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea.
Relevant issues:
Additional fix for #1093.