fix: use credentials key in pool

[olavloite]

The SpannerPool uses among other fields the credentials of a ConnectionOptions to determine whether an existing Spanner instance can be reused or not. Certain credentials instances, most notably UserCredentials, are mutable and the result of equals(Object) and hashCode() of an instance may change during its lifetime. This could cause the SpannerPool to inadvertently close a Spanner instance that is still in use.

This change replaces the usage of the actual credentials as part of the pool key with a key that is based on the method that was used to determine the credentials. This ensures that if two connections request a Spanner instance using the same credentialsUrl, that they will be considered equal. The same also applies to two connections that both do not set any specific credentials options and use the application default credentials.

Fixes googleapis/java-spanner-jdbc#206 and GoogleCloudPlatform/google-cloud-spanner-hibernate#202

[codecov]

Codecov Report

Merging #430 into master will increase coverage by 0.06%.
The diff coverage is 91.30%.

@@             Coverage Diff              @@
##             master     #430      +/-   ##
============================================
+ Coverage     82.16%   82.23%   +0.06%     
- Complexity     2455     2465      +10     
============================================
  Files           136      138       +2     
  Lines         13589    13620      +31     
  Branches       1307     1309       +2     
============================================
+ Hits          11166    11200      +34     
+ Misses         1895     1892       -3     
  Partials        528      528              

Impacted Files	Coverage Δ	Complexity Δ
...m/google/cloud/spanner/connection/SpannerPool.java	86.98% <88.23%> (+0.44%)	30.00 <0.00> (ø)
...le/cloud/spanner/connection/ConnectionOptions.java	86.38% <100.00%> (+0.67%)	60.00 <2.00> (+3.00)
...m/google/cloud/spanner/spi/v1/GapicSpannerRpc.java	82.03% <0.00%> (ø)	81.00% <0.00%> (ø%)
...m/google/cloud/spanner/LazySpannerInitializer.java	50.00% <0.00%> (ø)	1.00% <0.00%> (?%)
.../google/cloud/spanner/AbstractLazyInitializer.java	100.00% <0.00%> (ø)	5.00% <0.00%> (?%)
...gle/cloud/spanner/AsyncTransactionManagerImpl.java	76.27% <0.00%> (+6.77%)	12.00% <0.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 b312091...10928e7. Read the comment docs.

[olavloite]

@skuruppu @thiagotnunes Friendly ping for this PR. It fixes a bug in the JDBC driver that can cause a Pooled has been closed exception to occur.

[thiagotnunes]

hey @olavloite, I think I am missing how does assigning the credentials to a new field fixedCredentials will prevent mutability (since equals will compare the new CredentialsKey, which might have a credential, which might be mutable).

[olavloite]

hey @olavloite, I think I am missing how does assigning the credentials to a new field fixedCredentials will prevent mutability (since equals will compare the new CredentialsKey, which might have a credential, which might be mutable).

Thanks for the quick response @thiagotnunes

Assigning the credentials to the new field fixedCredentials will not fix the problem of mutability of the fixedCredentials. The problem however was that the methods of assigning a credentials instance and a credentialsUrl to the ConnectionOptions were not separated properly.

You have a number of options for setting the credentials when creating a ConnectionOptions instance:

1. You can assign an OAuthToken that you have previously obtained.
2. You can assign a Credentials instance.
3. You set set a URL that points to a file that contains the credentials.
4. You can let the client library pick the default credentials of the environment by not setting any of the above options.

Option 2 and 3 internally both set the ConnectionOptions.credentials field and when determining whether a Spanner instance was present in the pool, this field was used to determine equality. This change separates the two options so that:

1. When a credentials instance has been assigned to the ConnectionOptions the field fixedCredentials will be used for the equality check.
2. When a credentialsUrl has been set on the ConnectionOptions, the URL will be used for the equality check.

Setting a credentials instance directly when creating a ConnectionOptions will still suffer from any possible mutability of the credentials instance (or use of mutable fields in its equals method). It is however not currently used by any part of the client library or JDBC driver.

Setting a credentialsUrl is used by the JDBC driver and will no longer suffer from the possible mutability of the credentials that are chosen.

What also might be worth noting:

• This problem occurs when using UserCredentials, as this class calls super.equals(Object) as part of its equals(Object) method. The super implementation (OAuth2Credentials) will compare the current OAuth tokens of this and other.
• This problem does not occur when using ServiceAccountCredentials, as this class does not include any mutable fields in the equals(Object) implementation, and does not call the super implementation.

[thiagotnunes]

@olavloite thanks for the detailed explanation. LGTM.

[olavloite]

@thiagotnunes Apparently you have not (yet?) been added as a member of https://github.com/orgs/googleapis/teams/yoshi-java/members, which means that merging this PR is still blocked until a member of that team approves the PR. Maybe one of the maintainers could add you as a member to the team.

[thiagotnunes]

@olavloite sorry about that, I have requested membership