Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix: configure quota project id from context #1166

Closed
wants to merge 6 commits into from

Conversation

summer-ji-eng
Copy link
Contributor

expand #1162

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 7, 2020
@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #1166 into master will increase coverage by 0.01%.
The diff coverage is 92.30%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1166      +/-   ##
============================================
+ Coverage     78.97%   78.98%   +0.01%     
- Complexity     1194     1199       +5     
============================================
  Files           205      205              
  Lines          5265     5263       -2     
  Branches        436      436              
============================================
- Hits           4158     4157       -1     
+ Misses          935      933       -2     
- Partials        172      173       +1     
Impacted Files Coverage Δ Complexity Δ
...main/java/com/google/api/gax/rpc/StubSettings.java 74.64% <ø> (-0.68%) 12.00 <0.00> (ø)
...ain/java/com/google/api/gax/rpc/ClientContext.java 92.55% <92.30%> (-0.31%) 24.00 <15.00> (+5.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 08c96a5...cf0d14d. Read the comment docs.

Comment on lines 222 to 224
settings.getQuotaProjectId() == null && quotaProjectID != null
? quotaProjectID
: settings.getQuotaProjectId())
Copy link
Contributor

@chingor13 chingor13 Aug 7, 2020

Choose a reason for hiding this comment

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

I'm not convinced that we need this -- the quotaProjectId instance variable in ClientContext is only used in the constructor when converting back to StubSettings. Since the headers, internalHeaders, and credentials will still be set, if the resulting StubSettings is converted back to ClientContext, the same quotaProjectId resolution will happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand from our chat, Settings would only care about the explicit value set from setQuotaProjectId ClientContext would care about all 4 sources of that value.

  1. Settings would only care about the explicit value set from setQuotaProjectId, so I removed get QuotaProjectId from setHeaderProvider, setInternalHeaderProvider, and SetCredentialProvider.

  2. ClientContext would care about all 4 sources of that value: (summary follow, if quotaProjectId not being set from setting, we update the value from clientcontext if it has been set from header, internalheader or credentials)
    Details:
    when create ClientContext,
    first, we check if quota_project_id has been set from setting, if yes, we just use this value, update header with this value and wrap credentials hide quotaProjectId
    if not, then, we look at if header and internalHeader contain quota-project-id key, if yes, we update local quota project id, and wrap credentials hide quotaProjectId.
    final, we look at if credential has quotaProjectId key, if yes we update local quota project id
    At the end, clientcontext.setQuotaProjectId with correct value.

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment is that the instance value set by clientcontext.setQuotaProjectId is not how the header is being set on the request and is only used when recreating a StubSettings instance from the ClientContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't hurt to do this (except doing an extra conditional check above), but it doesn't seem necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I know why it is necessary, because https://github.com/googleapis/gax-java/blob/master/gax/src/main/java/com/google/api/gax/rpc/StubSettings.java#L199-L213

Maybe I over think this problem. Looks like we only need to remove credential from stubsetting. Your PR is already covered everything.

I am agreeing on #1162

Comment on lines 156 to 158
} else if (credentials != null
&& credentials.getRequestMetadata().containsKey(QUOTA_PROJECT_ID_HEADER_KEY)) {
quotaProjectID = credentials.getRequestMetadata().get(QUOTA_PROJECT_ID_HEADER_KEY).get(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this else clause is needed for the reason described in https://github.com/googleapis/gax-java/pull/1166/files#diff-ac347ed639ae3cbdd394c7769616ff1dR224

Copy link
Contributor Author

Choose a reason for hiding this comment

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

credentials could null when NoCredentialProvider is used in the unit test.

Copy link
Contributor

@chingor13 chingor13 Aug 7, 2020

Choose a reason for hiding this comment

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

For this one I mean the entire block that update the quotaProjectId value to be set on the ClientContext.Builder at the bottom of ClientContext.create.

@chingor13 chingor13 closed this Aug 7, 2020
@elharo elharo deleted the hide_quota_from_context branch October 29, 2020 14:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants