retrieve paramater encryption metadata if they are not retrieved yet

[xiangyushawn]

for issue #195

[xiangyushawn]

We do not have GitHub test for AE yet, so I didn't add a test here. Even if we have AE test, how can we test it explicitly? I tested it manually.

[codecov-io]

Codecov Report

Merging #220 into dev will increase coverage by 0.15%.
The diff coverage is 36.36%.

@@             Coverage Diff              @@
##                dev     #220      +/-   ##
============================================
+ Coverage     32.87%   33.03%   +0.15%     
- Complexity     1437     1451      +14     
============================================
  Files            97       97              
  Lines         23385    23448      +63     
  Branches       3889     3896       +7     
============================================
+ Hits           7688     7745      +57     
+ Misses        14147    14145       -2     
- Partials       1550     1558       +8

Flag	Coverage Δ	Complexity Δ
#JDBC41	32.92% <36.36%> (+0.18%)	1444 <3> (+11)	⬆️
#JDBC42	32.97% <36.36%> (+0.18%)	1447 <3> (+15)	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java	27.62% <36.36%> (+0.09%)	76 <3> (ø)	⬇️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	39.66% <0%> (-0.44%)	202% <0%> (-2%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	23.72% <0%> (-0.21%)	170% <0%> (-1%)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	36.75% <0%> (ø)	0% <0%> (ø)	⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	44.87% <0%> (+0.07%)	183% <0%> (+2%)	⬆️
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java	29.61% <0%> (+0.64%)	36% <0%> (ø)	⬇️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java	39.9% <0%> (+0.71%)	52% <0%> (+1%)	⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	25.66% <0%> (+1.11%)	42% <0%> (+3%)	⬆️
...va/com/microsoft/sqlserver/jdbc/SQLServerBlob.java	26.59% <0%> (+1.29%)	13% <0%> (+2%)	⬆️
...va/com/microsoft/sqlserver/jdbc/SQLServerClob.java	25.75% <0%> (+2.33%)	4% <0%> (ø)	⬇️
... and 5 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 6e66442...277b50e. Read the comment docs.

[Suraiya-Hameed]

this fix will work only if the first parameter is encrypted. Consider the case where first parameter is not encrypted, then it will fall back to the old behavior as inOutParam[0].getCryptoMetadata() is null.

[xiangyushawn]

thank you. fixed with flag

[Suraiya-Hameed]

update the variable name to camel case, encryptionMetadataIsRetrieved

[xiangyushawn]

ah.. typo

[Suraiya-Hameed]

double check if this needs to go inside the if block.

[xiangyushawn]

@v-suhame we use this if block to increase performance. The line of code you mentioned does not break anything or degrade performance. It just set 2 values. There are a lot of places setting maxRows, I feel it's safe to have it outside of the if black

[Suraiya-Hameed]

apply the changes for batch statement too.

[Suraiya-Hameed]

It does not break anything, but I don't see any reason why either this line or the one next to it should be called. We are resting the necessary fields after calling the sp_describe_parameter_encryption, so there is no obvious need to reset them when it is not called.

[xiangyushawn]

@v-suhame I noticed batch statement too but we do not need to apply changes for batch statement: since it's called only when numBatchesExecuted is equal to 0

[xiangyushawn]

It does break things if we do not call this line. This new logic was introduced to resolve inconsistent parameter definition problem between (AE on, no encryption) and (AE on, encryption used).
Here is an example:
when AE is on and the column is smallmoney, when the column is not encrypted, we should send it as money ; when the column is encrypted, we have to send it as SmallMoney. If we remove that line, instead of money, driver will send non-encrypted smallmoney as smallmoney for the second preparedStatement, which is not correct.

[Suraiya-Hameed]

thanks @v-xiangs, that clarifies it.

If buildPreparedStrings has to be called with renewDefinition as true every time AE is on, then it would be better to make a single call to buildPreparedStrings rather than calling it twice, once with renew as false and later override it with renew as true.

Also, hasNewTypeDefinitions should be updated with the result of buildPreparedStrings, else driver will prepare the query every single time preparedStatement is executed. In current AE implementation, sp_prepexec is called for each pstmt execution, unlike non-AE case where sp_prepexec is called once followed by sp_execute

[xiangyushawn]

@v-suhame Thank you for the very thoughtful code review. It does help me to remember the AE logic.

For the first question:
We have to call buildPreparedStrings twice.
The first call updates inOutParam and then inOutParam will be used in getParameterEncryptionMetadata, otherwise it will throw exception because parameter information is unknown. Also, for the first call, renewDefinition has to be false in order to give proper parameter definition.
For the second call, as I have already explained, is necessary to resolve inconsistent parameter definition problem when AE is on and column is not encrypted. The parameter renewDefinition as to be true.

For the second question:
The purpose of buildPreparedStrings is to update the information of inOutParam, there is no need to update hasNewTypeDefinitions since it should not change the value of hasNewTypeDefinitions.

[Suraiya-Hameed]

Let me detail what I meant in the early comment.

If there is a non encrypted smallmoney column, just having AE on in connection, and calling pstmt.execute twice makes these calls

exec sp_prepexec @p1 output,N'@P0 decimal(38,2)',N'INSERT INTO [table] ( [smallMoney_col] ) VALUES(@P0)        ',12.34
exec sp_prepexec @p1 output,N'@P0 decimal(38,2)',N'INSERT INTO [table] ( [smallMoney_col] ) VALUES(@P0)        ',12.34

with AE off, pstmt is not prepared every time.

exec sp_prepexec @p1 output,N'@P0 decimal(38,2)',N'INSERT INTO [table] ( [smallMoney_col] ) VALUES(@P0)        ',12.34
exec sp_execute 1,12.34

As you are aware, sp_execute is faster than sp_prepexec. It would be better to stick to it. I was suggesting those changes for this to happen.

[xiangyushawn]

@v-suhame I understand what you meant now. It does improve performance a lot. I am going to make the change now. Thank you.