fix: configure quota project id from context #1166
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
settings.getQuotaProjectId() == null && quotaProjectID != null | ||
? quotaProjectID | ||
: settings.getQuotaProjectId()) |
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.
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.
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.
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
.
-
Settings would only care about the explicit value set from setQuotaProjectId
, so I removed getQuotaProjectId
from setHeaderProvider, setInternalHeaderProvider, and SetCredentialProvider. -
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.
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.
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.
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.
It doesn't hurt to do this (except doing an extra conditional check above), but it doesn't seem necessary.
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.
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
} else if (credentials != null | ||
&& credentials.getRequestMetadata().containsKey(QUOTA_PROJECT_ID_HEADER_KEY)) { | ||
quotaProjectID = credentials.getRequestMetadata().get(QUOTA_PROJECT_ID_HEADER_KEY).get(0); |
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.
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
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.
credentials could null when NoCredentialProvider is used in the unit test.
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.
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.
Co-authored-by: Jeff Ching <chingor@google.com>
expand #1162