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 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

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