Skip to content

spanner-jdbc: Allow empty or invalid env credentials - #6092

Closed
olavloite wants to merge 2 commits into
googleapis:masterfrom
olavloite:spanner-jdbc-fix-tests-for-envs-with-invalid-or-no-app-credentials
Closed

spanner-jdbc: Allow empty or invalid env credentials#6092
olavloite wants to merge 2 commits into
googleapis:masterfrom
olavloite:spanner-jdbc-fix-tests-for-envs-with-invalid-or-no-app-credentials

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

On environments with no or invalid application credentials, the JDBC driver would throw an IllegalArgumentException when a connection was being created without an explicit credentials URL included in the connection string.

Updates #6075

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 16, 2019
@olavloite
olavloite requested a review from kolea2August 16, 2019 07:56
@olavloite

olavloite commented Aug 16, 2019

Copy link
Copy Markdown
ContributorAuthor

The build failures on both Java 7 and 8 are related to the second issue described in #6075, that is solved by #6094.

} catch (Exception e) {
// Ignore the error if the user did not set a credentials URL.
// The error is then caused by an invalid environment configuration
// and should not be thrown here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we add a test that asserts the correct exception is thrown?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've added a couple of test cases to cover this.

On environments with no or invalid application credentials, the JDBC
driver would throw an IllegalArgumentException when a connection was
being created without an explicit credentials URL included in the
connection string.
@olavloite
olavloiteforce-pushed the spanner-jdbc-fix-tests-for-envs-with-invalid-or-no-app-credentials branch from 5415654 to 3f019ebCompareAugust 17, 2019 07:05
@codecov

codecovBot commented Aug 17, 2019

Copy link
Copy Markdown

Codecov Report

Merging #6092 into master will increase coverage by <.01%.
The diff coverage is 93.75%.

Impacted file tree graph

@@ Coverage Diff @@## master #6092 +/- ##
============================================
+ Coverage 47.38% 47.38% +<.01% 
Complexity 27180 27180 ============================================
Files 2523 2523 Lines 274580 274589 +9 Branches 31380 31381 +1 ============================================
+ Hits 130118 130126 +8 
Misses 134852 134852 - Partials 9610 9611 +1
Impacted FilesCoverage ΔComplexity Δ
...m/google/cloud/spanner/jdbc/ConnectionOptions.java83.62% <100%> (+0.8%)44 <0> (ø)⬇️
...ava/com/google/cloud/spanner/jdbc/SpannerPool.java86.36% <66.66%> (-0.57%)30 <0> (ø)

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 d820f3d...3f019eb. Read the comment docs.

@olavloite
olavloite requested a review from kolea2August 17, 2019 09:28
@skuruppu
skuruppu self-requested a review August 23, 2019 01:34
@olavloite

Copy link
Copy Markdown
ContributorAuthor

Abandoned. This should be fixed in the test cases rather than deferring the exception to a later moment.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@olavloite@kolea2@googlebot