Bug 384454 - Remove sqlite3_bind_parameter_indexes. r=sspitzer

2007-06-18 16:42:23 -07:00 · 2007-06-18 16:42:23 -07:00 · d96db35e43
--- a/db/sqlite3/README.MOZILLA
+++ b/db/sqlite3/README.MOZILLA
@ -22,8 +22,6 @@ the parser and all that goop as part of our build.
 If you do a configure, you may want to --disable-tcl because the tcl
 part may not work on all systems.

-You will also need to apply sqlite3-param-indexes.patch.
-
 They you need to update sqlite3file.h, which pulls out random bits of the
 internal files that we need to export. If any of these internal structures
 change, they need to be changed in sqlite3file.h as well.
--- a/db/sqlite3/sqlite3-param-indexes.patch
+++ b/db/sqlite3/sqlite3-param-indexes.patch
@ -1,80 +0,0 @@
-diff -ru orig/sqlite333/sqlite3.h sqlite333/sqlite3.h
--- orig/sqlite333/sqlite3.h	2006-01-31 08:23:57.000000000 -0800
-+++ sqlite333/sqlite3.h	2006-02-08 12:12:42.156250000 -0800
-@@ -1377,6 +1377,20 @@
- # undef double
- #endif
- 
-+/*
-+** Given a wildcard parameter name, return the set of indexes of the
-+** variables with that name.  If there are no variables with the given
-+** name, return 0.  Otherwise, return the number of indexes returned
-+** in *pIndexes.  The array should be freed with
-+** sqlite3_free_parameter_indexes.
-+*/
-+int sqlite3_bind_parameter_indexes(
-+    sqlite3_stmt *pStmt,
-+    const char *zName,
-+    int **pIndexes
-+);
-+void sqlite3_free_parameter_indexes(int *pIndexes);
-+
- #ifdef __cplusplus
- }  /* End of the 'extern "C"' block */
- #endif
-Only in sqlite333: sqlite3.h~
-diff -ru orig/sqlite333/vdbeapi.c sqlite333/vdbeapi.c
--- orig/sqlite333/vdbeapi.c	2006-01-31 08:23:57.000000000 -0800
-+++ sqlite333/vdbeapi.c	2006-02-08 12:13:46.562500000 -0800
-@@ -764,6 +764,50 @@
- }
- 
- /*
-+** Given a wildcard parameter name, return the set of indexes of the
-+** variables with that name.  If there are no variables with the given
-+** name, return 0.  Otherwise, return the number of indexes returned
-+** in *pIndexes.  The array should be freed with
-+** sqlite3_free_parameter_indexes.
-+*/
-+int sqlite3_bind_parameter_indexes(
-+    sqlite3_stmt *pStmt,
-+    const char *zName,
-+    int **pIndexes
-+){
-+  Vdbe *p = (Vdbe*)pStmt;
-+  int i, j, nVars, *indexes;
-+  if( p==0 ){
-+    return 0;
-+  }
-+  createVarMap(p);
-+  if( !zName )
-+    return 0;
-+  /* first count */
-+  nVars = 0;
-+  for(i=0; i<p->nVar; i++){
-+    const char *z = p->azVar[i];
-+    if( z && strcmp(z,zName)==0 ){
-+      nVars++;
-+    }
-+  }
-+  indexes = sqliteMalloc( sizeof(int) * nVars );
-+  j = 0;
-+  for(i=0; i<p->nVar; i++){
-+    const char *z = p->azVar[i];
-+    if( z && strcmp(z,zName)==0 )
-+      indexes[j++] = i+1;
-+  }
-+  *pIndexes = indexes;
-+  return nVars;
-+} 
-+  
-+void sqlite3_free_parameter_indexes(int *pIndexes)
-+{   
-+  sqliteFree( pIndexes );
-+} 
-+
-+/*
- ** Transfer all bindings from the first statement over to the second.
- ** If the two statements contain a different number of bindings, then
- ** an SQLITE_ERROR is returned.
-Only in sqlite333: vdbeapi.c~
--- a/db/sqlite3/src/sqlite3.h
+++ b/db/sqlite3/src/sqlite3.h
@ -12,7 +12,7 @@
 ** This header file defines the interface that the SQLite library
 ** presents to client programs.
 **
-** @(#) $Id: sqlite3.h,v 1.6 2006/05/22 17:48:14 brettw%gmail.com Exp $
+** @(#) $Id: sqlite3.h,v 1.7 2007/06/18 23:42:24 sdwilsh%shawnwilsher.com Exp $
 */
 #ifndef _SQLITE3_H_
 #define _SQLITE3_H_
@ -1476,20 +1476,6 @@ int sqlite3_table_column_metadata(
 # undef double
 #endif

-/*
-** Given a wildcard parameter name, return the set of indexes of the
-** variables with that name.  If there are no variables with the given
-** name, return 0.  Otherwise, return the number of indexes returned
-** in *pIndexes.  The array should be freed with
-** sqlite3_free_parameter_indexes.
-*/
-int sqlite3_bind_parameter_indexes(
-    sqlite3_stmt *pStmt,
-    const char *zName,
-    int **pIndexes
-);
-void sqlite3_free_parameter_indexes(int *pIndexes);
-
 /*
 ** Preload the databases into the pager cache, up to the maximum size of the
 ** pager cache.
--- a/db/sqlite3/src/vdbeapi.c
+++ b/db/sqlite3/src/vdbeapi.c
@ -780,50 +780,6 @@ int sqlite3_bind_parameter_index(sqlite3_stmt *pStmt, const char *zName){
  return 0;
 }

-/*
-** Given a wildcard parameter name, return the set of indexes of the
-** variables with that name.  If there are no variables with the given
-** name, return 0.  Otherwise, return the number of indexes returned
-** in *pIndexes.  The array should be freed with
-** sqlite3_free_parameter_indexes.
-*/
-int sqlite3_bind_parameter_indexes(
-    sqlite3_stmt *pStmt,
-    const char *zName,
-    int **pIndexes
-){
-  Vdbe *p = (Vdbe*)pStmt;
-  int i, j, nVars, *indexes;
-  if( p==0 ){
-    return 0;
-  }
-  createVarMap(p);
-  if( !zName )
-    return 0;
-  /* first count */
-  nVars = 0;
-  for(i=0; i<p->nVar; i++){
-    const char *z = p->azVar[i];
-    if( z && strcmp(z,zName)==0 ){
-      nVars++;
-    }
-  }
-  indexes = sqliteMalloc( sizeof(int) * nVars );
-  j = 0;
-  for(i=0; i<p->nVar; i++){
-    const char *z = p->azVar[i];
-    if( z && strcmp(z,zName)==0 )
-      indexes[j++] = i+1;
-  }
-  *pIndexes = indexes;
-  return nVars;
-}
-
-void sqlite3_free_parameter_indexes(int *pIndexes)
-{
-  sqliteFree( pIndexes );
-}
-
 /*
 ** Transfer all bindings from the first statement over to the second.
 ** If the two statements contain a different number of bindings, then
--- a/storage/src/mozStorageStatement.cpp
+++ b/storage/src/mozStorageStatement.cpp
@ -222,18 +222,41 @@ mozStorageStatement::GetParameterIndexes(const nsACString &aParameterName, PRUin
    NS_ENSURE_ARG_POINTER(aCount);
    NS_ENSURE_ARG_POINTER(aIndexes);

-    int *indexes, count;
-    count = sqlite3_bind_parameter_indexes(mDBStatement, nsPromiseFlatCString(aParameterName).get(), &indexes);
-    if (count) {
-        *aIndexes = (PRUint32*) nsMemory::Alloc(sizeof(PRUint32) * count);
-        for (int i = 0; i < count; i++)
-            (*aIndexes)[i] = indexes[i];
-        sqlite3_free_parameter_indexes(indexes);
-        *aCount = count;
-    } else {
+    nsCAutoString name(":");
+    name.Append(aParameterName);
+
+    if (sqlite3_bind_parameter_index(mDBStatement, name.get()) == 0) {
+        // Named parameter not found
        *aCount = 0;
        *aIndexes = nsnull;
+        return NS_OK;
    }
+    
+    int count = sqlite3_bind_parameter_count(mDBStatement);
+    int *idxs = new int[count];
+    if (!idxs)
+        return NS_ERROR_OUT_OF_MEMORY;
+
+    int size = 0;
+    for (int i = 0; i < count; i++) {
+        // sqlite indices start at 1
+        const char *pName = sqlite3_bind_parameter_name(mDBStatement, i + 1);
+        if (name.Equals(pName))
+            idxs[size++] = i;
+    }
+
+    *aCount = size;
+    *aIndexes = (PRUint32*) NS_Alloc(sizeof(PRUint32) * size);
+    if (!aIndexes) {
+        delete[] idxs;
+        return NS_ERROR_OUT_OF_MEMORY;
+    }
+
+    for (int i = 0; i < size; i++)
+        (*aIndexes)[i] = idxs[i];
+
+    delete[] idxs;
+
    return NS_OK;
 }

--- a/storage/src/mozStorageStatementWrapper.cpp
+++ b/storage/src/mozStorageStatementWrapper.cpp
@ -1,5 +1,6 @@
-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
-/* ***** BEGIN LICENSE BLOCK *****
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+ * vim: sw=4 ts=4 sts=4
+ * ***** BEGIN LICENSE BLOCK *****
 * Version: MPL 1.1/GPL 2.0/LGPL 2.1
 *
 * The contents of this file are subject to the Mozilla Public License Version
@ -21,6 +22,7 @@
 *
 * Contributor(s):
 *   Vladimir Vukicevic <vladimir.vukicevic@oracle.com>
+ *   Shawn Wilsher <me@shawnwilsher.com>
 *
 * Alternatively, the contents of this file may be used under the terms of
 * either the GNU General Public License Version 2 or later (the "GPL"), or
@ -97,34 +99,22 @@ protected:
 static PRBool
 JSValStorageStatementBinder (JSContext *cx,
                             mozIStorageStatement *aStatement,
-                             int *aParamIndexes,
-                             int aNumIndexes,
+                             int aIdx,
                             jsval val)
 {
-    int i;
    if (JSVAL_IS_INT(val)) {
        int v = JSVAL_TO_INT(val);
-        for (i = 0; i < aNumIndexes; i++)
-            aStatement->BindInt32Parameter(aParamIndexes[i], v);
+        (void)aStatement->BindInt32Parameter(aIdx, v);
    } else if (JSVAL_IS_DOUBLE(val)) {
        double d = *JSVAL_TO_DOUBLE(val);
-        for (i = 0; i < aNumIndexes; i++)
-            aStatement->BindDoubleParameter(aParamIndexes[i], d);
+        (void)aStatement->BindDoubleParameter(aIdx, d);
    } else if (JSVAL_IS_STRING(val)) {
        JSString *str = JSVAL_TO_STRING(val);
-        for (i = 0; i < aNumIndexes; i++)
-            aStatement->BindStringParameter(aParamIndexes[i], nsDependentString(NS_REINTERPRET_CAST(PRUnichar*, JS_GetStringChars(str)), JS_GetStringLength(str)));
+        (void)aStatement->BindStringParameter(aIdx, nsDependentString(NS_REINTERPRET_CAST(PRUnichar*, JS_GetStringChars(str)), JS_GetStringLength(str)));
    } else if (JSVAL_IS_BOOLEAN(val)) {
-        if (val == JSVAL_TRUE) {
-            for (i = 0; i < aNumIndexes; i++)
-                aStatement->BindInt32Parameter(aParamIndexes[i], 1);
-        } else {
-            for (i = 0; i < aNumIndexes; i++)
-                aStatement->BindInt32Parameter(aParamIndexes[i], 0);
-        }
+        (void)aStatement->BindInt32Parameter(aIdx, (val == JSVAL_TRUE) ? 1 : 0);
    } else if (JSVAL_IS_NULL(val)) {
-        for (i = 0; i < aNumIndexes; i++)
-            aStatement->BindNullParameter(aParamIndexes[i]);
+        (void)aStatement->BindNullParameter(aIdx);
    } else if (JSVAL_IS_OBJECT(val)) {
        JSObject *obj = JSVAL_TO_OBJECT(val);
        // some special things
@ -134,8 +124,7 @@ JSValStorageStatementBinder (JSContext *cx,
            PRInt64 msec;
            LL_D2L(msec, msecd);

-            for (i = 0; i < aNumIndexes; i++)
-                aStatement->BindInt64Parameter(aParamIndexes[i], msec);
+            (void)aStatement->BindInt64Parameter(aIdx, msec);
        } else {
            return PR_FALSE;
        }
@ -313,7 +302,7 @@ mozStorageStatementWrapper::Call(nsIXPConnectWrappedNative *wrapper, JSContext *

    // bind parameters
    for (int i = 0; i < (int) argc; i++) {
-        if (!JSValStorageStatementBinder(cx, mStatement, &i, 1, argv[i])) {
+        if (!JSValStorageStatementBinder(cx, mStatement, i, argv[i])) {
            *_retval = PR_FALSE;
            return NS_ERROR_INVALID_ARG;
        }
@ -811,9 +800,9 @@ mozStorageStatementParams::SetProperty(nsIXPConnectWrappedNative *wrapper, JSCon
    if (JSVAL_IS_INT(id)) {
        int idx = JSVAL_TO_INT(id);

-        *_retval = JSValStorageStatementBinder (cx, mStatement, &idx, 1, *vp);
+        *_retval = JSValStorageStatementBinder (cx, mStatement, idx, *vp);
    } else if (JSVAL_IS_STRING(id)) {
-        int indexCount = 0, *indexes;
+        sqlite3_stmt *stmt = mStatement->GetNativeStatementPointer();

        JSString *str = JSVAL_TO_STRING(id);
        nsCAutoString name(":");
@ -821,28 +810,25 @@ mozStorageStatementParams::SetProperty(nsIXPConnectWrappedNative *wrapper, JSCon
                                                            ::JS_GetStringLength(str))));

        // check to see if there's a parameter with this name
-        indexCount = sqlite3_bind_parameter_indexes(mStatement->GetNativeStatementPointer(), name.get(), &indexes);
-        if (indexCount == 0) {
-            // er, not found? How'd we get past NewResolve?
-            fprintf (stderr, "********** mozStorageStatementWrapper: Couldn't find parameter %s\n", name.get());
+        if (sqlite3_bind_parameter_index(stmt, name.get()) == 0) {
            *_retval = PR_FALSE;
-            return NS_ERROR_FAILURE;
-        } else {
-            // drop this by 1, to account for sqlite's indexes being 1-based
-            for (int i = 0; i < indexCount; i++)
-                indexes[i]--;
-
-            *_retval = JSValStorageStatementBinder (cx, mStatement, indexes, indexCount, *vp);
-            sqlite3_free_parameter_indexes(indexes);
+            return NS_ERROR_INVALID_ARG;
+        }
+        
+        *_retval = PR_TRUE;
+        // You can use a named parameter more than once in a statement...
+        int count = sqlite3_bind_parameter_count(stmt);
+        for (int i = 0; (i < count) && (*_retval); i++) {
+            // sqlite indices start at 1
+            const char *pName = sqlite3_bind_parameter_name(stmt, i + 1);
+            if (name.Equals(pName))
+                *_retval = JSValStorageStatementBinder(cx, mStatement, i, *vp);
        }
    } else {
        *_retval = PR_FALSE;
    }

-    if (*_retval)
-        return NS_OK;
-    else
-        return NS_ERROR_INVALID_ARG;
+    return (*_retval) ? NS_OK : NS_ERROR_INVALID_ARG;
 }

 /* void preCreate (in nsISupports nativeObj, in JSContextPtr cx, in JSObjectPtr globalObj, out JSObjectPtr parentObj); */
--- a/storage/test/unit/head_storage.js
+++ b/storage/test/unit/head_storage.js
@ -62,9 +62,10 @@ function getService()
  return Cc["@mozilla.org/storage/service;1"].getService(Ci.mozIStorageService);
 }

+var gDBConn = null;
 function getOpenedDatabase()
 {
-  return getService().openDatabase(getTestDB());
+  return gDBConn ? gDBConn : getService().openDatabase(getTestDB());
 }

 function createStatement(aSQL)
--- a/storage/test/unit/test_storage_statement.js
+++ b/storage/test/unit/test_storage_statement.js
@ -64,33 +64,27 @@ function test_getParameterIndexes_different()
 {
  var stmt = createStatement("SELECT * FROM test WHERE id = :id OR name = :name");
  var count = { value: 0 };
-  var result = stmt.getParameterIndexes(":id", count);
+  var result = stmt.getParameterIndexes("id", count);
  do_check_eq(1, count.value);
  do_check_eq(1, result.length);
-  do_check_eq(1, result[0]); // XXX is this a bug?
-  // SQLite index is one, but how we treat them, it should be zero
+  do_check_eq(0, result[0]);
  
-  result = stmt.getParameterIndexes(":name", count);
+  result = stmt.getParameterIndexes("name", count);
  do_check_eq(1, count.value);
  do_check_eq(1, result.length);
-  do_check_eq(2, result[0]); // XXX is this a bug?
-  // SQLite index is two, but how we treat them, it should be one
+  do_check_eq(1, result[0]);
 }

 function test_getParameterIndexes_same()
 {
-  // XXX SQLite is buggy with this case :(
-  return;
+  // XXX this looks like a bug in sqlite
  var stmt = createStatement("SELECT * FROM test WHERE id = :test OR name = :test");

-  print("param count = " + stmt.parameterCount);
  var count = { value: 0 };
-  var result = stmt.getParameterIndexes(":test", count);
-  print("count.value = " + count.value);
-  do_check_eq(2, count.value);
-  print("result.length = " + result.length);
-  print("result[0] = " + result[0]);
-  do_check_eq(2, result.length);
+  var result = stmt.getParameterIndexes("test", count);
+  do_check_eq(1, count.value);
+  do_check_eq(1, result.length);
+  do_check_eq(0, result[0]);
 }

 function test_columnCount()
--- a/storage/test/unit/test_storage_statement_wrapper.js
+++ b/storage/test/unit/test_storage_statement_wrapper.js
@ -0,0 +1,105 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Storage Test Code.
+ *
+ * The Initial Developer of the Original Code is
+ *   Mozilla Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2007
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+// This file tests the functions of mozIStorageStatement
+
+function setup()
+{
+  getOpenedDatabase().createTable("test", "id INTEGER PRIMARY KEY, name TEXT," +
+                                          "alt_name TEXT");
+}
+
+var wrapper = new Components.Constructor("@mozilla.org/storage/statement-wrapper;1",
+                                         Ci.mozIStorageStatementWrapper,
+                                         "initialize");
+
+// we want to override the default function for this file
+createStatement = function(aSQL) {
+  return new wrapper(getOpenedDatabase().createStatement(aSQL));
+}
+
+function test_binding_params()
+{
+  var stmt = createStatement("INSERT INTO test (name) VALUES (:name)");
+
+  const name = "foo";
+  stmt.params.name = name;
+  stmt.execute();
+
+  stmt = createStatement("SELECT COUNT(*) AS number FROM test");
+  do_check_true(stmt.step());
+  do_check_eq(1, stmt.row.number);
+  stmt.reset();
+
+  stmt = createStatement("SELECT name FROM test WHERE id = 1");
+  do_check_true(stmt.step());
+  do_check_eq(name, stmt.row.name);
+  stmt.reset();
+}
+
+function test_binding_multiple_params()
+{
+  var stmt = createStatement("INSERT INTO test (name, alt_name)" +
+                             "VALUES (:name, :name)");
+  const name = "me";
+  stmt.params.name = name;
+  stmt.execute();
+
+  stmt = createStatement("SELECT COUNT(*) AS number FROM test");
+  do_check_true(stmt.step());
+  do_check_eq(2, stmt.row.number);
+  stmt.reset();
+
+  stmt = createStatement("SELECT name, alt_name FROM test WHERE id = 2");
+  do_check_true(stmt.step());
+  do_check_eq(name, stmt.row.name);
+  do_check_eq(name, stmt.row.alt_name);
+  stmt.reset();
+}
+
+var tests = [test_binding_params, test_binding_multiple_params];
+
+function run_test()
+{
+  setup();
+
+  for (var i = 0; i < tests.length; i++)
+    tests[i]();
+    
+  cleanup();
+}
+