[summer-ji-eng]

expand #1162

[codecov]

Codecov Report

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

@@             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.

[chingor13]

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.

[summer-ji-eng]

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.

[chingor13]

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.

[chingor13]

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

[summer-ji-eng]

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

[chingor13]

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

[summer-ji-eng]

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

[chingor13]

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.