Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removing connection property - fipsProvider #460

Merged
merged 5 commits into from
Aug 31, 2017
Merged

Removing connection property - fipsProvider #460

merged 5 commits into from
Aug 31, 2017

Conversation

ulvii
Copy link
Contributor

@ulvii ulvii commented Aug 25, 2017 •
edited
Loading

Based on the discussions on #433 and #438.

@msftclas
Copy link

@ulvii,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@ulvii ulvii mentioned this pull request Aug 25, 2017
Closed
@codecov-io
Copy link

codecov-io commented Aug 25, 2017 •
edited
Loading

Codecov Report

Merging #460 into dev will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##               dev     #460      +/-   ##
===========================================
- Coverage     46.3%   46.23%   -0.07%     
+ Complexity    2198     2196       -2     
===========================================
  Files          108      108              
  Lines        25260    25244      -16     
  Branches      4176     4172       -4     
===========================================
- Hits         11696    11672      -24     
- Misses       11542    11551       +9     
+ Partials      2022     2021       -1
Flag Coverage Δ Complexity Δ
#JDBC41 46.03% <0%> (-0.09%) 2185 <0> (-2)
#JDBC42 46.1% <0%> (-0.04%) 2192 <0> (-2)
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerResource.java 100% <ø> (ø) 4 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java 46.99% <ø> (-0.16%) 66 <0> (-1)
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 76.96% <ø> (-0.13%) 25 <0> (ø)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 53.61% <0%> (-0.47%) 0 <0> (ø)
...java/com/microsoft/sqlserver/jdbc/StringUtils.java 25% <0%> (-12.5%) 3% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 41.57% <0%> (-5.62%) 14% <0%> (-2%)
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 44.26% <0%> (-0.45%) 104% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.06% <0%> (-0.31%) 129% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.48% <0%> (ø) 238% <0%> (-1%) ⬇️
... and 4 more

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 7e7ada5...6b26648. Read the comment docs.

sehrope
sehrope reviewed Aug 30, 2017
View reviewed changes
src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java Outdated
if (logger.isLoggable(Level.FINER))
logger.finer(toString() + " FIPS provider & TrustStoreType should pass with TrustStore.");
logger.finer(toString() + "TrustStoreType is requiered alongside with TrustStore.");
Copy link
Contributor

Choose a reason for hiding this comment

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

s/requiered/required/

@cheenamalhotra cheenamalhotra added this to the 6.3.2 milestone Aug 30, 2017
cheenamalhotra
cheenamalhotra reviewed Aug 31, 2017
View reviewed changes
src/test/java/com/microsoft/sqlserver/jdbc/fips/FipsTest.java
@@ -72,32 +71,12 @@ public void fipsEncryptTest() throws Exception {
}
catch (SQLServerException e) {
Assertions.assertTrue(
e.getMessage().contains("Could not enable FIPS due to either encrypt is not true or using trusted certificate settings."),
e.getMessage().contains("Unable to verify FIPS mode settings."),
Copy link
Member

@cheenamalhotra cheenamalhotra Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I would suggest getting these same String error messages from SQLServerResource.java in test cases, such that your code becomes:
e.getMessage().contains(SQLServerResource.getResource("R_invalidFipsConfig"), "Some..").
It would remove all possibilities of errors and add maintainability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getResource() is not a public API

Copy link
Member

Choose a reason for hiding this comment

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

Yeah.. I missed that, in that case we need a custom resource bundle for our test framework. I have created issue #466 to track this change.

cheenamalhotra
cheenamalhotra reviewed Aug 31, 2017
View reviewed changes
src/test/java/com/microsoft/sqlserver/jdbc/fips/FipsTest.java Outdated
@@ -52,8 +52,7 @@ public void fipsTrustServerCertificateTest() throws Exception {
}
catch (SQLServerException e) {
Assertions.assertTrue(
e.getMessage().contains("Could not enable FIPS due to either encrypt is not true or using trusted certificate settings."),
"Should create exception for invalid TrustServerCertificate value");
e.getMessage().contains("Unable to verify FIPS mode settings."));
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason for removing "Should create exception for invalid TrustServerCertificate value" from line 56?