Repository: weecology/retriever

Issue:
SQL INSERT statements use custom quoting rather than parameter binding 
Any reason you can't use parameter binding for binding values to SQL statements such as  `INSERT (...) VALUES (...)`? For most SQL engines, it's both faster and safer than using custom quoting and direct interpolation of values. It's certainly a lot less code (because the SQL engine does most of the type conversion and quoting).    


Discussion:
ethanwhite: Tests are all passing (locally at least). It would be worth testing out a few other datasets using both flat file and database formats to confirm that this didn't break any edge cases.
henrykironde: I think mysql parameter should be ?
henrykironde: Not sure but will do some tests later
ethanwhite: > I think mysql parameter should be ?

Yeah, I thought so originally too, but couldn't get to it work and then looked at the docs again.

> Not sure but will do some tests later

Looks like the tests are failing on CI so no need to worry about this one until I get that sorted out.
ethanwhite: Looks like I broke something on Python 2 (hooray for CI!). I'll take a look when I get the chance and update later.
ethanwhite: Looks like it's just MySQL on Python 2, so some sort of lovely edge case. It fails when trying to insert the first line of `mammal-masses` (to provide an example). First line is the insert statement, second line is the first of the sets of values to be inserted by `executemany`, and then the error message:

```
Creating table mammal_masses.MammalMasses...
INSERT INTO mammal_masses.MammalMasses (continent, status, sporder, family, genus, species, log_mass_g, comb_mass_g, reference) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s)
[u'AF', u'extant', u'Artiodactyla', u'Bovidae', u'Addax', u'nasomaculatus', '4.85', '70000.3', u'60']
INSERT INTO mammal_masses.MammalMasses (continent, status, sporder, family, genus, species, log_mass_g, comb_mass_g, reference) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s)
(1064, u"You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '4.85'', b''70000.3'', '60'),('AF', 'extant', 'Artiodactyla', 'Bovidae', 'Aepycer' at line 1")
```

The error message has `b''70000.3''` instead of `b"70000.3"`. Not sure if this is the source of the issue or just a display quick.

Patch:
diff --git a/retriever/engines/csvengine.py b/retriever/engines/csvengine.py
index 303017e..d4a1961 100644
--- a/retriever/engines/csvengine.py
+++ b/retriever/engines/csvengine.py
@@ -53,9 +53,13 @@ class engine(Engine):
         """Write a line to the output file"""
         self.output_file.writerows(statement)
 
+    def executemany(self, statement, values, commit=True):
+        """Write a line to the output file"""
+        self.output_file.writerows(statement)
+
     def format_insert_value(self, value, datatype):
         """Formats a value for an insert statement"""
-        v = Engine.format_insert_value(self, value, datatype, escape=False, processed=True)
+        v = Engine.format_insert_value(self, value, datatype)
         if v == 'null':
             return ""
         try:
diff --git a/retriever/engines/jsonengine.py b/retriever/engines/jsonengine.py
index 360d172..873c927 100644
--- a/retriever/engines/jsonengine.py
+++ b/retriever/engines/jsonengine.py
@@ -67,9 +67,13 @@ class engine(Engine):
         """Write a line to the output file"""
         self.output_file.writelines(statement)
 
+    def executemany(self, statement, values, commit=True):
+        """Write a line to the output file"""
+        self.output_file.writelines(statement)
+
     def format_insert_value(self, value, datatype):
         """Formats a value for an insert statement"""
-        v = Engine.format_insert_value(self, value, datatype, escape=False, processed=True)
+        v = Engine.format_insert_value(self, value, datatype)
         if v == 'null':
             return ""
         try:
diff --git a/retriever/engines/msaccess.py b/retriever/engines/msaccess.py
index c133f18..860deb7 100644
--- a/retriever/engines/msaccess.py
+++ b/retriever/engines/msaccess.py
@@ -51,10 +51,6 @@ class engine(Engine):
         dropstatement = "DROP %s %s" % (objecttype, objectname)
         return dropstatement
 
-    def escape_single_quotes(self, value):
-        """Escapes the single quotes in the value"""
-        return value.replace("'", "''")
-
     def insert_data_from_file(self, filename):
         """Perform a bulk insert."""
         self.get_cursor()
diff --git a/retriever/engines/mysql.py b/retriever/engines/mysql.py
index 05c669c..a6c0db7 100644
--- a/retriever/engines/mysql.py
+++ b/retriever/engines/mysql.py
@@ -19,6 +19,7 @@ class engine(Engine):
         "bool": "BOOL",
     }
     max_int = 4294967295
+    placeholder = "%s"
     required_opts = [("user",
                       "Enter your MySQL username",
                       "root"),
diff --git a/retriever/engines/postgres.py b/retriever/engines/postgres.py
index 1df9bc9..e30e3dd 100644
--- a/retriever/engines/postgres.py
+++ b/retriever/engines/postgres.py
@@ -17,6 +17,7 @@ class engine(Engine):
         "bool": "boolean",
     }
     max_int = 2147483647
+    placeholder = "%s"
     required_opts = [("user",
                       "Enter your PostgreSQL username",
                       "postgres"),
@@ -63,10 +64,6 @@ class engine(Engine):
         statement += " CASCADE;"
         return statement.replace(" DATABASE ", " SCHEMA ")
 
-    def escape_single_quotes(self, value):
-        """Escapes single quotes in the value"""
-        return value.replace("'", "''")
-
     def insert_data_from_file(self, filename):
         """Use PostgreSQL's "COPY FROM" statement to perform a bulk insert."""
         self.get_cursor()
diff --git a/retriever/engines/sqlite.py b/retriever/engines/sqlite.py
index ca6b4c1..9b8630a 100644
--- a/retriever/engines/sqlite.py
+++ b/retriever/engines/sqlite.py
@@ -17,6 +17,7 @@ class engine(Engine):
         "char": "TEXT",
         "bool": "INTEGER",
     }
+    placeholder = "?"
     required_opts = [("file",
                       "Enter the filename of your SQLite database",
                       os.path.join(DATA_DIR, "sqlite.db"),
@@ -31,10 +32,6 @@ class engine(Engine):
         a separate connection."""
         return None
 
-    def escape_single_quotes(self, line):
-        """Escapes single quotes in the line"""
-        return line.replace("'", "''")
-
     def get_bulk_insert_statement(self):
         """Get insert statement for bulk inserts
 
diff --git a/retriever/engines/xmlengine.py b/retriever/engines/xmlengine.py
index ed90c19..d0f970a 100644
--- a/retriever/engines/xmlengine.py
+++ b/retriever/engines/xmlengine.py
@@ -64,10 +64,14 @@ class engine(Engine):
         """Write a line to the output file"""
         self.output_file.writelines(statement)
 
+    def executemany(self, statement, values, commit=True):
+        """Write a line to the output file"""
+        self.output_file.writelines(statement)
+
     def format_insert_value(self, value, datatype):
         """Formats a value for an insert statement"""
-        v = Engine.format_insert_value(self, value, datatype, escape=False, processed=True)
-        if v == 'null':
+        v = Engine.format_insert_value(self, value, datatype)
+        if v == None:
             return ""
         try:
             if len(v) > 1 and v[0] == v[-1] == "'":
diff --git a/retriever/lib/datapackage.py b/retriever/lib/datapackage.py
index de40c43..dfff62d 100644
--- a/retriever/lib/datapackage.py
+++ b/retriever/lib/datapackage.py
@@ -97,26 +97,6 @@ def get_contains_pk(dialect):
     dialect['contains_pk'] = val
 
 
-def get_escape_single_quotes(dialect):
-    """Set escape_single_quotes property"""
-    val = clean_input("escape_single_quotes (bool = True/False) (press return to skip): ",
-                      ignore_empty=True, dtype=bool)
-    if val == "" or val == []:
-        # return and dont add key to dialect dict if empty val
-        return
-    dialect['escape_single_quotes'] = val
-
-
-def get_escape_double_quotes(dialect):
-    """Set escape_double_quotes property"""
-    val = clean_input("escape_double_quotes (bool = True/False) (press return to skip): ",
-                      ignore_empty=True, dtype=bool)
-    if val == "" or val == []:
-        # return and dont add key to dialect dict if empty val
-        return
-    dialect['escape_double_quotes'] = val
-
-
 def get_fixed_width(dialect):
     """Set fixed_width property"""
     val = clean_input("fixed_width (bool = True/False) (press return to skip): ",
@@ -195,8 +175,6 @@ def create_json():
             get_delimiter(table['dialect'])
             get_do_not_bulk_insert(table['dialect'])
             get_contains_pk(table['dialect'])
-            get_escape_single_quotes(table['dialect'])
-            get_escape_double_quotes(table['dialect'])
             get_fixed_width(table['dialect'])
             get_header_rows(table['dialect'])
 
diff --git a/retriever/lib/engine.py b/retriever/lib/engine.py
index 117fc9f..5cded86 100644
--- a/retriever/lib/engine.py
+++ b/retriever/lib/engine.py
@@ -112,15 +112,15 @@ class Engine(object):
                         if self.debug:
                             print(cleanvalues)
                         raise
-                    multiple_values = []
                     try:
-                        self.execute(insert_stmt, commit=False)
+                        self.executemany(insert_stmt, multiple_values, commit=False)
                         prompt = "Progress: " + str(count_iter) + " / " + str(real_line_length) + " rows inserted into " + self.table_name() + " totaling " + str(total) + ":"
                         sys.stdout.write(prompt + "\b" * len(prompt))
                         sys.stdout.flush()
                     except:
                         print(insert_stmt)
                         raise
+                    multiple_values = []
                 else:
                     multiple_values.append(cleanvalues)
             count_iter += 1
@@ -470,20 +470,18 @@ class Engine(object):
         dropstatement = "DROP %s IF EXISTS %s" % (objecttype, objectname)
         return dropstatement
 
-    def escape_single_quotes(self, value):
-        """Escapes single quotes in the value"""
-        return value.replace("'", "\\'")
-
-    def escape_double_quotes(self, value):
-        """Escapes double quotes in the value"""
-        return value.replace('"', '\\"')
-
     def execute(self, statement, commit=True):
         """Executes the given statement"""
         self.cursor.execute(statement)
         if commit:
             self.connection.commit()
 
+    def executemany(self, statement, values, commit=True):
+        """Executes the given statement with multiple values"""
+        self.cursor.executemany(statement, values)
+        if commit:
+            self.connection.commit()
+
     def exists(self, script):
         """Checks to see if the given table exists"""
         return all([self.table_exists(
@@ -516,7 +514,7 @@ class Engine(object):
         """Returns the full path of a file in the archive directory."""
         return os.path.join(self.format_data_dir(), filename)
 
-    def format_insert_value(self, value, datatype, escape=True, processed=False):
+    def format_insert_value(self, value, datatype):
         """Format a value for an insert statement based on data type
 
         Different data types need to be formated differently to be properly
@@ -527,14 +525,7 @@ class Engine(object):
         2. Harmonizing null indicators
         3. Cleaning up badly formatted integers
         4. Obtaining consistent float representations of decimals
-
-        The optional `escape` argument controls whether additional quotes in
-        strings are escaped, as needed for SQL database management systems
-        (escape=True), or not escaped, as needed for flat file based engines
-        (escape=False).
-
-        The optional processed argument indicates that the engine has it's own
-        escaping mechanism. i.e the csv engine which uses its own dialect"""
+        """
         datatype = datatype.split('-')[-1]
         strvalue = str(value).strip()
 
@@ -544,41 +535,32 @@ class Engine(object):
             strvalue = strvalue[1:-1]
         missing_values = ("null", "none")
         if strvalue.lower() in missing_values:
-            return "null"
+            return None
         elif datatype in ("int", "bigint", "bool"):
             if strvalue:
                 intvalue = strvalue.split('.')[0]
                 if intvalue:
                     return int(intvalue)
                 else:
-                    return "null"
+                    return None
             else:
-                return "null"
+                return None
         elif datatype in ("double", "decimal"):
             if strvalue.strip():
                 try:
                     decimals = float(str(strvalue))
-                    return str(decimals)
+                    return decimals
                 except:
-                    return "null"
+                    return None
             else:
-                return "null"
+                return None
         elif datatype == "char":
             if strvalue.lower() in missing_values:
-                return "null"
-            if escape:
-                # automatically escape quotes in string fields
-                if hasattr(self.table, "escape_double_quotes") and self.table.escape_double_quotes:
-                    strvalue = self.escape_double_quotes(strvalue)
-                if hasattr(self.table, "escape_single_quotes") and self.table.escape_single_quotes:
-                    strvalue = self.escape_single_quotes(strvalue)
-                return "'" + strvalue + "'"
-            if processed:
-                return strvalue
+                return None
             else:
-                return "'" + strvalue + "'"
+                return strvalue
         else:
-            return "null"
+            return None
 
     def get_cursor(self):
         """Gets the db cursor."""
@@ -645,15 +627,19 @@ class Engine(object):
         columns = self.table.get_insert_columns()
         types = self.table.get_column_datatypes()
         columncount = len(self.table.get_insert_columns(join=False, create=False))
-        insert_stmt = "INSERT INTO {} ({}) VALUES ".format(self.table_name(), columns)
         for row in values:
             row_length = len(row)
             # Add None with appropriate value type for empty cells
             for i in range(columncount - row_length):
                 row.append(self.format_insert_value(None, types[row_length + i]))
 
-            insert_stmt += " (" + ", ".join([str(val) for val in row]) + "), "
-        insert_stmt = insert_stmt.rstrip(", ") + ";"
+        insert_stmt = "INSERT INTO " + self.table_name()
+        insert_stmt += " (" + columns + ")"
+        insert_stmt += " VALUES ("
+        for i in range(0, columncount):
+            insert_stmt += "{}, ".format(self.placeholder)
+        insert_stmt = insert_stmt.rstrip(", ") + ")"
+
         if self.debug:
             print(insert_stmt)
         return insert_stmt
diff --git a/retriever/lib/table.py b/retriever/lib/table.py
index 3b28911..25ff7c6 100644
--- a/retriever/lib/table.py
+++ b/retriever/lib/table.py
@@ -26,8 +26,6 @@ class Table(object):
         self.record_id = 0
         self.columns = []
         self.replace_columns = []
-        self.escape_single_quotes = True
-        self.escape_double_quotes = True
         self.cleaned_columns = False
         for key, item in list(kwargs.items()):
             setattr(self, key, item[0] if isinstance(item, tuple) else item)
diff --git a/scripts/mt_st_helens_veg.json b/scripts/mt_st_helens_veg.json
index 8fbe5f5..33e03ba 100644
--- a/scripts/mt_st_helens_veg.json
+++ b/scripts/mt_st_helens_veg.json
@@ -143,8 +143,7 @@
         },
         {
             "dialect": {
-                "do_not_bulk_insert": "True",
-                "escape_single_quotes": "True"
+                "do_not_bulk_insert": "True"
             },
             "name": "species",
             "schema": {},
@@ -166,5 +165,5 @@
         "species_plot_year": "https://ndownloader.figshare.com/files/5613783",
         "structure_plot_year": "https://ndownloader.figshare.com/files/5613786"
     },
-    "version": "1.2.0"
+    "version": "1.2.1"
 }
diff --git a/version.txt b/version.txt
index b3e3451..492f2bf 100644
--- a/version.txt
+++ b/version.txt
@@ -46,7 +46,7 @@ mapped_plant_quads_ks.json,1.2.0
 mapped_plant_quads_mt.json,1.2.0
 marine_recruitment_data.json,1.0.0
 mediter_basin_plant_traits.json,1.0.0
-mt_st_helens_veg.json,1.2.0
+mt_st_helens_veg.json,1.2.1
 nematode_traits.json,1.0.0
 ngreatplains-flowering-dates.json,1.0.0
 npn.py,2.1.0


Test patch:
diff --git a/test/test_retriever.py b/test/test_retriever.py
index 0af9764..f155d9a 100644
--- a/test/test_retriever.py
+++ b/test/test_retriever.py
@@ -127,16 +127,6 @@ def test_drop_statement():
     assert test_engine.drop_statement('TABLE', 'tablename') == "DROP TABLE IF EXISTS tablename"
 
 
-def test_escape_single_quotes():
-    """Test escaping of single quotes"""
-    assert test_engine.escape_single_quotes("1,2,3,'a'") == "1,2,3,\\'a\\'"
-
-
-def test_escape_double_quotes():
-    """Test escaping of double quotes"""
-    assert test_engine.escape_double_quotes('"a",1,2,3') == '\\"a\\",1,2,3'
-
-
 def test_extract_values_fixed_width():
     """Test extraction of values from line of fixed width data"""
     test_engine.table.fixed_width = [5, 2, 2, 3, 4]
@@ -181,18 +171,18 @@ def test_format_insert_value_int():
 
 def test_format_insert_value_double():
     """Test formatting of values for insert statements"""
-    assert test_engine.format_insert_value(26.22, 'double') == '26.22'
+    assert test_engine.format_insert_value(26.22, 'double') == 26.22
 
 
 def test_format_insert_value_string_simple():
     """Test formatting of values for insert statements"""
-    assert test_engine.format_insert_value('simple text', 'char') == "'simple text'"
+    assert test_engine.format_insert_value('simple text', 'char') == "simple text"
 
 
 def test_format_insert_value_string_complex():
     """Test formatting of values for insert statements"""
     assert test_engine.format_insert_value('my notes: "have extra, stuff"',
-                                           'char') == '\'my notes: \\"have extra, stuff\\"\''
+                                           'char') == 'my notes: "have extra, stuff"'
 
 
 def test_getmd5_lines():
@@ -407,4 +397,3 @@ def test_add_schema():
 
     add_schema(table_dict, table)
     assert(table_dict == result)
-    
\ No newline at end of file