Adding Old Constructor back to AKV Provider by cheenamalhotra · Pull Request #675 · microsoft/mssql-jdbc

cheenamalhotra · 2018-04-12T19:50:16Z

This PR adds back the old Constructor for SQLServerColumnEncryptionAzureKeyVaultProvider as below:

SQLServerColumnEncryptionAzureKeyVaultProvider akvProvider = new SQLServerColumnEncryptionAzureKeyVaultProvider(authenticationCallback, service);

This constructor was removed in PR #397 due to changed HttpClient implementation in AKV library version 1.0.0. With this PR we add backwards compatibility to applications using old AKV libraries with 6.0 and 6.2.1 driver versions and will now be able to upgrade without making any changes to code.

codecov-io · 2018-04-12T20:09:49Z

Codecov Report

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

@@             Coverage Diff              @@
##                dev     #675      +/-   ##
============================================
- Coverage     48.08%   48.02%   -0.07%     
+ Complexity     2586     2582       -4     
============================================
  Files           114      114              
  Lines         26578    26601      +23     
  Branches       4453     4454       +1     
============================================
- Hits          12781    12775       -6     
- Misses        11664    11689      +25     
- Partials       2133     2137       +4

Flag	Coverage Δ	Complexity Δ
#JDBC42	`47.94% <0%> (-0.03%)`	`2581 <0> (ø)`
#JDBC43	`47.89% <0%> (-0.11%)`	`2576 <0> (-6)`

Impacted Files	Coverage Δ	Complexity Δ
...m/microsoft/sqlserver/jdbc/KeyVaultCredential.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...QLServerColumnEncryptionAzureKeyVaultProvider.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	`33.21% <0%> (-0.4%)`	`245% <0%> (-3%)`
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	`54.53% <0%> (-0.2%)`	`0% <0%> (ø)`
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java	`59.44% <0%> (-0.1%)`	`134% <0%> (-1%)`
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	`45.41% <0%> (ø)`	`107% <0%> (-1%)`	⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`52.61% <0%> (+0.06%)`	`238% <0%> (-1%)`	⬇️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java	`50.68% <0%> (+0.24%)`	`157% <0%> (ø)`	⬇️
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java	`62.76% <0%> (+0.83%)`	`63% <0%> (ø)`	⬇️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	`46.06% <0%> (+1.12%)`	`17% <0%> (+1%)`	⬆️
... and 1 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 8cf4afb...e3bdaec. Read the comment docs.

peterbae

I tested the code and read through the logic, LGTM. Thanks for the implementation.

ulvii · 2018-05-03T22:44:43Z

 */
 class KeyVaultCredential extends KeyVaultCredentials {

+	SQLServerKeyVaultAuthenticationCallback authenticationCallback = null;


Indentation.

ulvii · 2018-05-04T00:30:51Z

+     *             when an error occurs
+     */
+    public SQLServerColumnEncryptionAzureKeyVaultProvider(SQLServerKeyVaultAuthenticationCallback authenticationCallback,
+            ExecutorService executorService) throws SQLServerException {


Where is executorService used?

executorService is not consumed here because the KeyVaultClientImpl now manages its own KeyVaultClientService as here. These services are used by Retrofit to make Rest Calls.

The only reason of adding this here was to retain the old constructor params used by current applications in industry to allow no change upgrade of JDBC Driver version, but can be removed as well.

Could lead to confusions. Let's make sure we discuss this with Jakub.

ulvii · 2018-05-04T00:40:07Z

            String scope) {
-        AuthenticationResult token = getAccessTokenFromClientCredentials(authorization, resource, clientId, clientKey);
-        return token.getAccessToken();
+    	if(authenticationCallback==null) {


Maybe we could try to avoid multiple returns?

https://github.com/Microsoft/mssql-jdbc/blob/dev/Coding_Guidelines.md

… old Constructor and added a new constructor with 1 param

peterbae

Other than these two comments, I approve.

peterbae · 2018-05-23T22:32:58Z

+    SQLServerKeyVaultAuthenticationCallback authenticationCallback = null;
+    String                                  clientId               = null;
+    String                                  clientKey              = null;
+    String                                  accessToken            = null;


formatting here?

That's how mssql-jdbc formatter works here.

This is what happens when you apply the formatter.

SQLServerKeyVaultAuthenticationCallback authenticationCallback = null; String clientId = null; String clientKey = null; String accessToken = null;

peterbae · 2018-05-23T22:41:25Z

+        RestClient restClient = new RestClient.Builder(new OkHttpClient.Builder(), new Retrofit.Builder()).withBaseUrl(baseUrl)
+                .withCredentials(credentials).withSerializerAdapter(new AzureJacksonAdapter())
+                .withResponseBuilderFactory(new AzureResponseBuilder.Factory()).build();
+        keyVaultClient = new KeyVaultClient(restClient);


If both SQLServerColumnEncryptionAzureKeyVaultProvider constructors do exactly the same thing, maybe refactor the inner part so we don't duplicate code?

...actually, we would have to revert that in our next release when we remove the deprecated API. in that case, just have the SQLServerColumnEncryptionAzureKeyVaultProvider with ExecutorService in its parameter call the other SQLServerColumnEncryptionAzureKeyVaultProvider constructor.

rene-ye · 2018-05-23T22:50:09Z

+     */
+    @Deprecated
+    public SQLServerColumnEncryptionAzureKeyVaultProvider(SQLServerKeyVaultAuthenticationCallback authenticationCallback,
+            ExecutorService executorService) throws SQLServerException {


Since we aren't doing anything with the executor, and the constructor is just here for backwards compatibility, shouldn't we make a call to SQLServerColumnEncryptionAzureKeyVaultProvider(SQLServerKeyVaultAuthenticationCallback authenticationCallback)? This might confuse people who are stepping through the code, thinking there is a very small change somewhere and that's why there are separate functions.

lilgreenbird · 2018-05-23T22:52:51Z

-        AuthenticationResult token = getAccessTokenFromClientCredentials(authorization, resource, clientId, clientKey);
-        return token.getAccessToken();
+        String accessToken;
+        if (authenticationCallback == null) {


if (null == authenticationCallback)

ulvii · 2018-05-24T16:16:30Z

+     * @param authenticationCallback
+     *            - Callback function used for authenticating to AAD.
+     * @param executorService
+     *            - The ExecutorService, previously used to create the keyVaultClient, but not in use anymore. - This param can be passed as 'null'


"This parameter"

ulvii · 2018-05-24T16:16:38Z

+     * Constructor that takes a callback function to authenticate to AAD. This is used by KeyVaultClient at runtime to authenticate to Azure Key
     * Vault.
     * 
+     * This constructor is present to maintain backwards compatibility with 6.0 version of the driver. Deprecated for removal in next Stable release.


"stable release"

Adding Old Constrcutor back to AKV Implementation

0b1d915

cheenamalhotra added 2 commits April 12, 2018 13:22

Making baseURL final

43f6e91

Remove unnecessary code.

433abda

cheenamalhotra requested review from peterbae and ulvii April 12, 2018 20:37

cheenamalhotra added this to the 6.5.2 milestone Apr 12, 2018

peterbae previously approved these changes Apr 12, 2018

View reviewed changes

cheenamalhotra removed this from the 6.5.2 milestone Apr 25, 2018

cheenamalhotra mentioned this pull request Apr 30, 2018

Updating multiple Maven Dependency and Plugin versions. #688

Merged

ulvii reviewed May 3, 2018

View reviewed changes

ulvii reviewed May 4, 2018

View reviewed changes

cheenamalhotra changed the title ~~Adding Old Constrcutor back to AKV Provider~~ Adding Old Constructor back to AKV Provider May 10, 2018

cheenamalhotra added 2 commits May 11, 2018 16:55

Merge branch 'dev' into akv-old-constructor

ab6ad0c

Feature | AKV Old Constructor changes - Reformatted code + Deprecated…

e05abee

… old Constructor and added a new constructor with 1 param

cheenamalhotra dismissed peterbae’s stale review via e05abee May 12, 2018 00:13

cheenamalhotra added this to the 6.5.3 milestone May 12, 2018

cheenamalhotra requested review from lilgreenbird and rene-ye May 14, 2018 18:35

cheenamalhotra added 2 commits May 15, 2018 10:12

Merge branch 'dev' into akv-old-constructor

633fe83

Trigger Appveyor test

c48cef5

peterbae reviewed May 23, 2018

View reviewed changes

rene-ye reviewed May 23, 2018

View reviewed changes

lilgreenbird reviewed May 23, 2018

View reviewed changes

cheenamalhotra added 2 commits May 23, 2018 15:54

Fix | AKV Old Constructor - Calling the other constructor instead.

fc1303c

Fix | Reversed null checks

252cacd

rene-ye previously approved these changes May 23, 2018

View reviewed changes

ulvii reviewed May 24, 2018

View reviewed changes

Resolving alignment problems and comments

e3bdaec

cheenamalhotra dismissed rene-ye’s stale review via e3bdaec May 24, 2018 17:57

rene-ye approved these changes May 24, 2018

View reviewed changes

ulvii approved these changes May 24, 2018

View reviewed changes

cheenamalhotra merged commit feaa8c3 into microsoft:dev May 25, 2018

Conversation

cheenamalhotra commented Apr 12, 2018

Uh oh!

codecov-io commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

peterbae left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulvii May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbae left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Apr 12, 2018 •

edited

Loading

cheenamalhotra May 4, 2018 •

edited

Loading

ulvii May 4, 2018 •

edited

Loading