Disallow non-parameterized queries for Bulk Copy API for batch insert #756

peterbae · 2018-07-23T18:18:37Z

As for the error messages, I am throwing an illegalArgumentException but it gets caught and gets turned into a logging message, so this part doesn't need translation into other languages.

rene-ye · 2018-07-23T18:53:17Z

...test/java/com/microsoft/sqlserver/jdbc/preparedStatement/BatchExecutionWithBulkCopyTest.java

+            pstmt.addBatch();
+
+            pstmt.executeBatch();
+            throw new Exception("shouldn't come here.");


Can we throw "Test did not throw an exception when it was Expected" instead? I think there's existing tests that do this and we should be consistent.

codecov-io · 2018-07-24T19:55:51Z

Codecov Report

Merging #756 into dev will increase coverage by 0.08%.
The diff coverage is 59.22%.

@@             Coverage Diff              @@
##                dev     #756      +/-   ##
============================================
+ Coverage     48.35%   48.43%   +0.08%     
- Complexity     2771     2806      +35     
============================================
  Files           116      116              
  Lines         27871    27853      -18     
  Branches       4631     4636       +5     
============================================
+ Hits          13476    13490      +14     
- Misses        12212    12225      +13     
+ Partials       2183     2138      -45

Flag	Coverage Δ	Complexity Δ
#JDBC42	`47.7% <59.22%> (-0.13%)`	`2738 <22> (+11)`
#JDBC43	`48.31% <59.22%> (+0.09%)`	`2796 <22> (+30)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java	`52.23% <59.22%> (-1.88%)`	`217 <22> (+6)`
...in/java/com/microsoft/sqlserver/jdbc/Geometry.java	`52.38% <0%> (-20.06%)`	`15% <0%> (-18%)`
...n/java/com/microsoft/sqlserver/jdbc/Geography.java	`52.38% <0%> (-17.7%)`	`15% <0%> (-18%)`
...sqlserver/jdbc/SQLServerBulkBatchInsertRecord.java	`40.39% <0%> (-2.65%)`	`25% <0%> (-2%)`
...osoft/sqlserver/jdbc/SQLServerSpatialDatatype.java	`85.38% <0%> (-0.88%)`	`290% <0%> (+33%)`
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	`38.36% <0%> (-0.44%)`	`42% <0%> (-1%)`
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	`62.43% <0%> (-0.12%)`	`0% <0%> (ø)`
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`52.6% <0%> (ø)`	`262% <0%> (ø)`	⬇️
...lserver/jdbc/SQLServerDataSourceObjectFactory.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
... and 34 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 0fef350...b62d82a. Read the comment docs.

cheenamalhotra · 2018-07-25T20:46:37Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerPreparedStatement.java

+                }
+
+                checkAdditionalQuery();
+
                String destinationTableName = tableName;
                SQLServerStatement stmt = (SQLServerStatement) connection.createStatement(ResultSet.TYPE_FORWARD_ONLY,


Statement is created here, and closed in next try's finally block? Better to put it in the next try-with-resources block along with ResultSet and remove finally block as it will have no purpose.

that try block is closing the resultset. this statement is closed during finally.

@peterbae , Why don't you just move statement creation to try-with-resources?

…/datetime/spatial datatype columns. added tests.

This reverts commit fd68988.

ulvii · 2018-07-27T19:36:03Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerPreparedStatement.java

@@ -2514,9 +2605,17 @@ private boolean checkAndRemoveComments() {
            localUserSQL = localUserSQL.substring(temp);


int temp = localUserSQL.indexOf("\n") + 2;

It should be +1 here, not +2.

* Update Snapshot for upcoming RTW release v7.0.0 * Change order of logic for checking the condition for using Bulk Copy API (#736) Fix | Change order of logic for checking the condition for using Bulk Copy API (#736) * Update CHANGELOG.md * Merge pull request #732 from cheenamalhotra/module (Export driver in automatic module) Introduce Automatic Module Name in POM. * Update CHANGELOG.md * Change Sha1HashKey to CityHash128Key for generating PreparedStatement handle and metadata cache keys (#717) * Change Sha1HashKey to CityHash128Key * Formatted code * Prepared statement performance fixes 1) Further speedups to prepared statement hashing 2) Caching of '?' chararacter positiobs in prepared statements to speed parameter substitution * String compare for hash keys added missing line for bulkcopy tests. * comment change * Move CityHash class to a separate file * spacings fixes cleaner code & logic * Add | Adding useBulkCopyForBatchInsert property to Request Boundary methods (#739) * Apply the collation name change to UTF8SupportTest * Package changes for CityHash with license information (#740) * Reformatted Code + Updated formatter (#742) * Reformatted Code + Updated formatter * Fix policheck issue with 'Country' keyword (#745) * Adding a new test for beginRequest()/endRequest() (#746) * Add | Adding a new test to notify the developers to consider beginRequest()/endRequest() when adding a new Connection API * Fix | Fixes for issues reported by static analysis tools (SonarQube + Fortify) (#747) * handle buffer reading for invalid buffer input by user * Revert "handle buffer reading" This reverts commit 11e2bf4. * updated javadocs (#754) * fixed some typos in javadocs (#760) * API and JavaDoc changes for Spatial Datatypes (#752) Add | API and JavaDoc changes for Spatial Datatypes (#752) * Disallow non-parameterized queries for Bulk Copy API for batch insert (#756) fix | Disallow non-parameterized queries for Bulk Copy API for batch insert (#756) * Formatting | Change scope of unwanted Public APIs + Code Format (#757) * Fix unwanted Public APIs. * Updated formatter to add new line to EOF + formatted project * Release | Release 7.0 changelog and version update (#748) * Updated Changelog + release version changes * Changelog entry updated as per review. * Updated changelog for new changes * Terminology update: request boundary declaration APIs * Trigger Appveyor * Update Samples and add new samples for new features (#761) * Update Samples and add new Samples for new features * Update samples from Peter * Updated JavaDocs * Switch to block comment * Update License copyright (#767)

Not going to allow non-parameterized query + test

e82db3f

peterbae changed the title ~~Not going to allow non-parameterized query + test~~ Disallow non-parameterized queries Bulk Copy API for batch insert + test Jul 23, 2018

rene-ye reviewed Jul 23, 2018

View reviewed changes

peterbae added 2 commits July 23, 2018 13:00

change how exceptions are worded

c4eef3c

fix issue with multiple queries going through

bb74613

cheenamalhotra added this to the 7.0.0 milestone Jul 24, 2018

peterbae added 2 commits July 24, 2018 09:50

part of fuzz testing

e9d35e9

check string length

e40f6cd

peterbae added 3 commits July 24, 2018 15:05

more checkings

088f7b1

remove recursion

5baa313

remove all recursions

85e0186

cheenamalhotra reviewed Jul 25, 2018

View reviewed changes

peterbae added 7 commits July 25, 2018 13:54

move statement to try block

d3ea922

remove double try statement

87f56b6

immediately reject the query if it's a non-parameterized query

f644d1d

problems go away

e15c6ff

check before insert to make sure we're not dealing with smalldatetime…

a286b23

…/datetime/spatial datatype columns. added tests.

remove out the commented test part

db89338

update test

4ffb9cb

peterbae changed the title ~~Disallow non-parameterized queries Bulk Copy API for batch insert + test~~ Disallow non-parameterized queries for Bulk Copy API for batch insert Jul 26, 2018

cheenamalhotra added this to Under Peer Review in MSSQL JDBC Jul 27, 2018

cheenamalhotra previously approved these changes Jul 27, 2018

View reviewed changes

rene-ye previously approved these changes Jul 27, 2018

View reviewed changes

ulvi is never wrong it seems

2e80254

peterbae dismissed stale reviews from rene-ye and cheenamalhotra via 2e80254 July 27, 2018 20:54

peterbae added 3 commits July 27, 2018 15:54

provide some extra security against semicolons

b4aed05

thank you cheena

74872cb

has to be <= 0

88e2da2

peterbae added 3 commits July 27, 2018 16:27

extra safety

3fe78e1

escape things

fd68988

Revert "escape things"

b62d82a

This reverts commit fd68988.

rene-ye approved these changes Jul 28, 2018

View reviewed changes

ulvii approved these changes Jul 28, 2018

View reviewed changes

peterbae merged commit ca4558e into microsoft:dev Jul 28, 2018

MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Jul 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow non-parameterized queries for Bulk Copy API for batch insert #756

Disallow non-parameterized queries for Bulk Copy API for batch insert #756

peterbae commented Jul 23, 2018 •

edited by cheenamalhotra

Loading

rene-ye Jul 23, 2018

codecov-io commented Jul 24, 2018 •

edited

Loading

cheenamalhotra Jul 25, 2018 •

edited

Loading

peterbae Jul 25, 2018

ulvii Jul 25, 2018

peterbae Jul 25, 2018

ulvii Jul 27, 2018

		@@ -2514,9 +2605,17 @@ private boolean checkAndRemoveComments() {
		localUserSQL = localUserSQL.substring(temp);

Disallow non-parameterized queries for Bulk Copy API for batch insert #756

Disallow non-parameterized queries for Bulk Copy API for batch insert #756

Conversation

peterbae commented Jul 23, 2018 • edited by cheenamalhotra Loading

rene-ye Jul 23, 2018

Choose a reason for hiding this comment

codecov-io commented Jul 24, 2018 • edited Loading

Codecov Report

cheenamalhotra Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

peterbae Jul 25, 2018

Choose a reason for hiding this comment

ulvii Jul 25, 2018

Choose a reason for hiding this comment

peterbae Jul 25, 2018

Choose a reason for hiding this comment

ulvii Jul 27, 2018

Choose a reason for hiding this comment

peterbae commented Jul 23, 2018 •

edited by cheenamalhotra

Loading

codecov-io commented Jul 24, 2018 •

edited

Loading

cheenamalhotra Jul 25, 2018 •

edited

Loading