From d96db35e43f604d22bb93efa0e7f0f2e804383ff Mon Sep 17 00:00:00 2001 From: "sdwilsh@shawnwilsher.com" Date: Mon, 18 Jun 2007 16:42:23 -0700 Subject: [PATCH] Bug 384454 - Remove sqlite3_bind_parameter_indexes. r=sspitzer --- db/sqlite3/README.MOZILLA | 2 - db/sqlite3/sqlite3-param-indexes.patch | 80 ------------- db/sqlite3/src/sqlite3.h | 16 +-- db/sqlite3/src/vdbeapi.c | 44 -------- storage/src/mozStorageStatement.cpp | 41 +++++-- storage/src/mozStorageStatementWrapper.cpp | 68 +++++------- storage/test/unit/head_storage.js | 3 +- storage/test/unit/test_storage_statement.js | 24 ++-- .../unit/test_storage_statement_wrapper.js | 105 ++++++++++++++++++ 9 files changed, 176 insertions(+), 207 deletions(-) delete mode 100644 db/sqlite3/sqlite3-param-indexes.patch create mode 100644 storage/test/unit/test_storage_statement_wrapper.js diff --git a/db/sqlite3/README.MOZILLA b/db/sqlite3/README.MOZILLA index ba30a8482d7d..a66df2ea869e 100644 --- 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. diff --git a/db/sqlite3/sqlite3-param-indexes.patch b/db/sqlite3/sqlite3-param-indexes.patch deleted file mode 100644 index 4ea5fa7a6201..000000000000 --- a/db/sqlite3/sqlite3-param-indexes.patch +++ /dev/null @@ -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; inVar; i++){ -+ const char *z = p->azVar[i]; -+ if( z && strcmp(z,zName)==0 ){ -+ nVars++; -+ } -+ } -+ indexes = sqliteMalloc( sizeof(int) * nVars ); -+ j = 0; -+ for(i=0; inVar; 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~ diff --git a/db/sqlite3/src/sqlite3.h b/db/sqlite3/src/sqlite3.h index 383a1ffe7e9b..178ed0d49a76 100644 --- 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. diff --git a/db/sqlite3/src/vdbeapi.c b/db/sqlite3/src/vdbeapi.c index fb1f9dbc63c5..94f18aa607c9 100644 --- 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; inVar; i++){ - const char *z = p->azVar[i]; - if( z && strcmp(z,zName)==0 ){ - nVars++; - } - } - indexes = sqliteMalloc( sizeof(int) * nVars ); - j = 0; - for(i=0; inVar; 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 diff --git a/storage/src/mozStorageStatement.cpp b/storage/src/mozStorageStatement.cpp index 96faf7130bb2..7283a76cbeea 100644 --- 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; } diff --git a/storage/src/mozStorageStatementWrapper.cpp b/storage/src/mozStorageStatementWrapper.cpp index 69b7cdeca65a..97a81113141a 100644 --- 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 + * Shawn Wilsher * * 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); */ diff --git a/storage/test/unit/head_storage.js b/storage/test/unit/head_storage.js index 4885de20a5e1..f79b6ba3cd18 100644 --- 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) diff --git a/storage/test/unit/test_storage_statement.js b/storage/test/unit/test_storage_statement.js index 1cfa58708b87..d09227c1e356 100644 --- 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() diff --git a/storage/test/unit/test_storage_statement_wrapper.js b/storage/test/unit/test_storage_statement_wrapper.js new file mode 100644 index 000000000000..20c9b19c492d --- /dev/null +++ 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 (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(); +} +