384454 - Remove sqlite3_bind_parameter_indexes

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

We add this function ourselves, and it's only use is for binding named parameters, which we don't even support anyway.

This also makes it harder for us to upgrade sqlite because we have to add this function to their files.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This will make it a wee bit easier to upgrade sqlite too.

Eventually, I'd like to make real support for binding named parameters, but we'll see if we have time down the line.

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 268395 [details] [diff] [review]
v1.0

I just talked to vlad - this is not so cool.  I'll fix it and make it all better, I promise!

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

Ok - no removing API functions, and I've added a unit test.  Arguably, that file needs more work, but it at least makes sure we don't break stuff with that functionality.

Comment 4

[(not reading, please use [e-mail] instead)]

Comment on attachment 268458 [details] [diff] [review]
v2.0

1)

+
+    if (0 == sqlite3_bind_parameter_index(mDBStatement, name.get())) {
+        // Named parameter not found
         *aCount = 0;
         *aIndexes = nsnull;
+        return NS_OK;
     }

minor nit, is "if (0 == sqlite3_bind_parameter_index.." the coding style of mozilla/storage?

2)
   
+    *aIndexes = (PRUint32*) NS_Alloc(sizeof(PRUint32) * size);

shouldn't we check if this alloc failed?

3)

+        if (0 == sqlite3_bind_parameter_index(stmt, name.get())) {

again, the coding style nit.

4)

+        *_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; i++) {
+            // sqlite indices start at 1
+            const char *pName = sqlite3_bind_parameter_name(stmt, i + 1);
+            if (name.Equals(pName))
+                *_retval = (*_retval) && JSValStorageStatementBinder(cx, mStatement, i, *vp);
         }

Perhaps instead of and'ing with *_retval, we could do something like this to bail out early when JSValStorageStatementBinder() returns PR_FALSE.

for (int i = 0; (i < count) && (*_retval); i++) {

5)

do we have a test case that uses a named parameter more than once in a statement?

Comment 5

[(not reading, please use [e-mail] instead)]

6)

I see the stmt.params.name test, but it seems that you are only testing that the insert succeeded (and we have a row in the test table) not that the value is "foo".

perhaps you could elaborate this test, and test all the other types (int, double, string, bool, null, object, ...)

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
re : 1 / 3
I didn't see any if statements that did anything similar to be honest.

re: 2
yes

re: 4
I can do that

re: 5
working on it

Comment 7

[(not reading, please use [e-mail] instead)]

> I didn't see any if statements that did anything similar to be honest.

I think the prevailing style of mozStorage points to either:

if (sqlite3_bind_parameter_index() == 0) ...

or

if (!sqlite3_bind_parameter_index()) ....

Thanks for addressing my comments and nits.  The test cases are what I care about the most.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

Fixes issues.  Filing a follow-up bug to get better tests for the other types.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 268858 [details] [diff] [review]
v2.1

r=sspitzer, thanks for addressing those nits / comments, and please don't forget to log the spin of bug (to expand the tests).

Comment 10

[Shawn Wilsher :sdwilsh]

Filed Bug 384954

I'm gonna do a clean build just to double check that these tests pass.

Comment 11

[Shawn Wilsher :sdwilsh]

Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.17; previous revision: 1.16
Checking in storage/src/mozStorageStatementWrapper.cpp;
new revision: 1.14; previous revision: 1.13
Checking in storage/test/unit/head_storage.js;
new revision: 1.3; previous revision: 1.2
Checking in storage/test/unit/test_storage_statement.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_storage_statement_wrapper.js;
initial revision: 1.1
Checking in db/sqlite3/README.MOZILLA;
new revision: 1.9; previous revision: 1.8
Removing db/sqlite3/sqlite3-param-indexes.patch;
new revision: delete; previous revision: 1.1
Checking in db/sqlite3/src/sqlite3.h;
new revision: 1.7; previous revision: 1.6
Checking in db/sqlite3/src/vdbeapi.c;
new revision: 1.11; previous revision: 1.10