403068 - Need a wrapper function for SQLite function sqlite3_column_decltype

Status: RESOLVED FIXED

(Toolkit :: Storage, enhancement)

Description

[john Zhang]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30)
Build Identifier: Firefox 3 Alpha 8

We have a product that is based on the Mozilla/Firefox engine.  JavaScript developers who write code for our product will want access to the database embedded in Mozilla/Firefox. The Storage subsystem wraps some of the SQLite3 C API in XPCOM but not all. In particular, it does not wrap the sqlite3_column_decltype  function. Why is that important? When I get an array of JS parameters to pass into a SQLite prepared statement, I have enough information about the types of those parameters from JavaScript to know how to bind them. But when the statement returns results, in a given row I don’t know enough to unambiguously turn them back into JavaScript values. In particular, if a user has datetimes or Booleans saved in a table, I can certainly save them, but when U get them back as integers I don’t know that I should be converting them back to JavaScript Dates and Booleans. But I could guess the right thing to do if I had the declared type of the column: I could look for the characters “bool” and “date” or “time” in the declared type and convert when I see them.

Reproducible: Always

Steps to Reproduce:
N/A



We will produce the fix; we have talked to Shawn Wilsher ([e-mail]), the original developer who has kindly accepted as the reviwer.

Comment 1

[john Zhang]

Attachment: Added a new wrapper function for a SQLite function and a test case

The cvs diff is done under the storage folder.

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 287909 [details] [diff] [review]
Added a new wrapper function for a SQLite function and a test case

>-[scriptable, uuid(1a95a6ec-e4b0-4d0b-947b-d2f40b04d057)]
>+[scriptable, uuid(42FAD13E-C67D-4b2c-BD61-2C5B17186772)]
can we keep them as lower case letters please?

>+
>+  /**
>+   * wrapper for const char *sqlite3_column_decltype(sqlite3_stmt *, int i)
While this is true, I'm more inclined to tell consumers of this what this does.  How about something more along the lines of "Obtains the declared column type of a prepared statement."

>+   * @param    aParamIndex index of the column in the statement.  0 is the 1st column.
nit: "The zero-based index of the column who's declared type we are interested in."
>+   * @returns  an AString type of the column
nit: lose the extra space please

>+function test_getColumnDecltype()
>+{
>+  var stmt = createStatement("SELECT name, id FROM test");
>+  do_check_eq("TEXT", stmt.getColumnDecltype(0));
>+  do_check_eq("INTEGER", stmt.getColumnDecltype(1));
>+  try {
>+    do_check_eq("INTEGER", stmt.getColumnDecltype(2));
could you change INTERGER to GARBAGE here since we shouldn't actually be returning anything useful?

Comment 3

[john Zhang]

Attachment: Update based on comments

Shawn,
I have updated the change based on your comments.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 287917 [details] [diff] [review]
Update based on comments

>-[scriptable, uuid(1a95a6ec-e4b0-4d0b-947b-d2f40b04d057)]
>+[scriptable, uuid(42fad13e-c67d-4b2c-bd61-2c5b17186772)]
>+
>+
nit: drop the whitespace

>+  /**
>+   * Obtains the declared column type of a prepared statement.
>+   *
>+   * @param    aParamIndex The zero-based index of the column who's declared type we are interested in.
>+   * @returns  an AString type of the column
>+   */
nit: line wrapping at 80 chars, and wording of @returns needs work.

>+  AUTF8String getColumnDecltype(in unsigned long aParamIndex);
nit: let's move this up above close()
 
> };
>Index: src/mozStorageStatement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/storage/src/mozStorageStatement.cpp,v
>retrieving revion 1.28
>diff -u -8 -p -r1.28 mozStorageStatement.cpp
>--- src/mozStorageStatement.cpp	21 Sep 2007 18:13:47 -0000	1.28
>+++ src/mozStorageStatement.cpp	8 Nov 2007 23:48:00 -0000
>@@ -18,16 +18,17 @@
>  * The Initial Developer of the Original Code is
>  *  Oracle Corporation
>  * Portions created by the Initial Developer are Copyright (C) 2004
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Vladimir Vukicevic <[e-mail]>
>  *   Shawn Wilsher <[e-mail]>
>+ *   John Zhang <[e-mail]>
>  *
>  * 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
>@@ -800,8 +801,25 @@ mozStorageStatement::EscapeStringForLIKE
>     for (PRInt32 i = 0; i < aValue.Length(); i++) {
>         if (aValue[i] == aEscapeChar || aValue[i] == MATCH_ALL || 
>             aValue[i] == MATCH_ONE)
>             aEscapedString += aEscapeChar;
>         aEscapedString += aValue[i];
>     }
>     return NS_OK;
> }
>+
>+/* AString getColumnDecltype(in unsigned long aParamIndex); */
>+NS_IMETHODIMP
>+mozStorageStatement::GetColumnDecltype(PRUint32 aParamIndex, nsACString& aDeclType)
nit: line wrapping at 80 chars

>+    return NS_OK;
>+}
>+
nit: extra newline at end of file

>Index: test/unit/test_storage_statement.js
>+function test_getColumnDecltype()
>+{
>+  var stmt = createStatement("SELECT name, id FROM test");
>+  do_check_eq("TEXT", stmt.getColumnDecltype(0));
>+  do_check_eq("INTEGER", stmt.getColumnDecltype(1));
>+  try {
>+    do_check_eq("GARBAGE", stmt.getColumnDecltype(2));
>+    do_throw("should not get here");
>+  } catch (e) {
>+    do_check_eq(Cr.NS_ERROR_ILLEGAL_VALUE, e.result);
>+  }
whoops - this totally needs a stmt.finalize() too.

I'll attach the patch with my review nits fixed.  r=sdwilsh

Comment 5

[Shawn Wilsher :sdwilsh]

s/close/reset/

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: for checkin

Fixes review nits.  Passes unit tests.

Drivers: this is a low risk patch that fixes a whole in our API.

Comment 7

[Damon Sicore (:damons)]

Comment on attachment 287953 [details] [diff] [review]
for checkin

a=release drivers

Comment 8

[Reed Loden [:reed]]

Checking in storage/public/mozIStorageStatement.idl;
/cvsroot/mozilla/storage/public/mozIStorageStatement.idl,v  <--  mozIStorageStatement.idl
new revision: 1.12; previous revision: 1.11
done
Checking in storage/src/mozStorageStatement.cpp;
/cvsroot/mozilla/storage/src/mozStorageStatement.cpp,v  <--  mozStorageStatement.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in storage/test/unit/test_storage_statement.js;
/cvsroot/mozilla/storage/test/unit/test_storage_statement.js,v  <--  test_storage_statement.js
new revision: 1.6; previous revision: 1.5
done