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

fix: quotaProjectId should be applied for cached getRequestMetadata(URI, Executor, RequestMetadataCallback) #509

Merged
merged 5 commits into from Dec 3, 2020

Conversation

chingor13
Copy link
Contributor

@chingor13 chingor13 commented Nov 30, 2020

Refactors the injection of the quota project id (and possibly other future headers) into the entrypoint that caches the request metadata (in OAuth2Credentials).

Fixes #507

@google-cla google-cla bot added the cla: yes label Nov 30, 2020
@codecov
Copy link

@codecov codecov bot commented Nov 30, 2020

Codecov Report

Merging #509 (1b4174f) into master (3b50627) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #509      +/-   ##
============================================
+ Coverage     79.80%   79.91%   +0.11%     
- Complexity      409      413       +4     
============================================
  Files            28       28              
  Lines          1936     1947      +11     
  Branches        201      203       +2     
============================================
+ Hits           1545     1556      +11     
  Misses          283      283              
  Partials        108      108              
Impacted Files Coverage Δ Complexity Δ
...java/com/google/auth/oauth2/OAuth2Credentials.java 88.00% <100.00%> (+0.90%) 38.00 <2.00> (+2.00)
.../google/auth/oauth2/ServiceAccountCredentials.java 82.44% <100.00%> (+0.14%) 49.00 <2.00> (+1.00)
...p/java/com/google/auth/oauth2/UserCredentials.java 80.28% <100.00%> (+0.28%) 32.00 <2.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b50627...1b4174f. Read the comment docs.

@chingor13 chingor13 changed the title test: add failing test for request metadata with callback fix: quotaProjectId should be applied for cached getRequestMetadata(URI, Executor, RequestMetadataCallback) Nov 30, 2020
@chingor13 chingor13 marked this pull request as ready for review Nov 30, 2020
@chingor13 chingor13 requested a review from as a code owner Nov 30, 2020
@@ -154,7 +156,9 @@ public void refresh() throws IOException {
synchronized (lock) {
requestMetadata = null;
temporaryAccess = null;
useAccessToken(Preconditions.checkNotNull(refreshAccessToken(), "new access token"));
useAccessToken(
Preconditions.checkNotNull(refreshAccessToken(), "new access token"),
Copy link
Contributor

@elharo elharo Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very strange and possibly inappropriate use of Preconditions.

Copy link
Contributor Author

@chingor13 chingor13 Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it's been there for years.

Filed #510 to track.

/**
* Provide additional headers to return as request metadata.
*
* @return Map of additional headers.
Copy link
Contributor

@elharo elharo Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "map of additional headers" or just "additional headers" since the map is provided by the type

Copy link
Contributor Author

@chingor13 chingor13 Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to "additional headers"

@@ -56,6 +57,7 @@

private static final long serialVersionUID = 4556936364828217687L;
private static final long MINIMUM_TOKEN_MILLISECONDS = 60000L * 5L;
private static final Map<String, List<String>> EMPTY_EXTRA_HEADERS = Collections.emptyMap();
Copy link
Contributor

@elharo elharo Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this be added to by subclasses? I'm not quite following but it looks like it might be; but this is immutable.

Copy link
Contributor Author

@chingor13 chingor13 Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addQuotaProjectIdToRequestMetadata returns a new ImmutableMap so nothing should be added to this (nor do we want anyone to).

*
* @return Map of additional headers.
*/
protected Map<String, List<String>> getAdditionalHeaders() {
Copy link
Contributor

@elharo elharo Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nested types (Map of List) suggest there's a missed opportunity for a more semantic class.

Copy link
Contributor Author

@chingor13 chingor13 Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP headers behave as maps of Strings to a list of Strings. We could possible wrap it, but we're not adding any extra behavior. For a general purpose HTTP client, we might do so, but it seem unnecessary here.

Note: getRequestMetadata() does return this same type (but we really can't feasibly change that as everything that depends on google-auth-library expects that return type).

@chingor13 chingor13 requested review from and elharo Dec 2, 2020
@chingor13 chingor13 merged commit 0a8412f into master Dec 3, 2020
14 checks passed
@chingor13 chingor13 deleted the bug-507 branch Dec 3, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Dec 11, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
### [0.22.2](https://www.github.com/googleapis/google-auth-library-java/compare/v0.22.1...v0.22.2) (2020-12-11)


### Bug Fixes

* quotaProjectId should be applied for cached `getRequestMetadata(URI, Executor, RequestMetadataCallback)` ([#509](https://www.github.com/googleapis/google-auth-library-java/issues/509)) ([0a8412f](https://www.github.com/googleapis/google-auth-library-java/commit/0a8412fcf9de4ac568b9f88618e44087dd31b144))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants