490085 - Add ability to bind multiple sets of parameters and execute asynchronously

Status: RESOLVED FIXED

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

Description

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

As discussed in the newsgroups, we need a way to bind multiple parameters to one statement, and execute that statement asynchronously.  The attached patch does this, but I need to write a bunch more tests before I'm confident with this implementation.

Comment 1

[Shawn Wilsher :sdwilsh]

Trying to debug test_statement_executeAsync.js has led to a major refactoring of it.  It's for the better, but it's a bit of code churn in the test...

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

This one actually passes the tests that I mentioned in the pass.  I added one more test since the last iteration as well.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

I think the only thing I'm not testing here is the noscript methods.  I'm not sure I care, given that the code is very straightforward, and calls into the generic methods anyway.

My public patch queue has this patch updated as well.

Comment 4

[Shawn Wilsher :sdwilsh]

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

This is a new API, so lets get sr on this as well.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

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

The code is beautiful and the extensive unit tests are awesome.  Although the choice of "itr" as an iterator name is quite the juxtaposition to the beauty :)

(Pulled the current patch from your patch repo along with new-async-api and the needs-revision patch on bug 490833.  Tests ran fine.)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

>    // Bind the data to our statement.
>    nsCOMPtr<mozIStorageError> error((*itr)->bind(stmt));
>    if (error) {

Just as a point of style, I really dislike when important things are done as function arguments; the important action here is the binding, not the creating of the error object.  So I'd split these up (probably with the extra blank lines so that the Important Bit doesn't get lost, especially given that it doesn't stand out:

    nsCOMPtr<mozIStorageError> error;

    error = (*itr)->bind(stmt);

    if (error) {


In BindByIndex/BindByName:
>   NS_ENSURE_FALSE(mLocked, NS_ERROR_UNEXPECTED);

Given that lock() is an explicit method here, this probably shouldn't be a NS_ENSURE_FALSE -- that'll actually spit out an error in debug builds, and I don't think that should happen since it's not really an assertion type thing.  It should be an error, but probably something explicit like NS_ERROR_STORAGE_BINDING_PARAMS_LOCKED instead of just unexpected (though NS_ERROR_UNEXPECTED is probably ok).

>   * Locks the array and prevents further modification to it (such as adding
>   * more elements to it).
>   */
>
>   void lock();
>
>   BindingParamsArray(Statement *aOwningStatement);

Another style nit (and I'm only being this nitpicky about style because Storage overall has pretty good coding style already) -- this constructor decl is in a really odd place in the header file -- at the very least put it above lock() (and lock's docs), or maybe right after the NS_DECL_* blocks at the top.  Maybe even move the iterator class so that it's defined just before begin(), and move getOwner() to be after lock(), to keep the iterator "block" all together.

Other than that, everything looks great.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Given that lock() is an explicit method here, this probably shouldn't be a
> NS_ENSURE_FALSE -- that'll actually spit out an error in debug builds, and I
> don't think that should happen since it's not really an assertion type thing. 
> It should be an error, but probably something explicit like
> NS_ERROR_STORAGE_BINDING_PARAMS_LOCKED instead of just unexpected (though
> NS_ERROR_UNEXPECTED is probably ok).
I'm not sure I agree with you actually.  They are misusing the API by calling this at this point, and I'd want to know it.  In general, we are pretty bad about giving specifics of the mistake in storage, so throwing NS_ERROR_UNEXPECTED is nothing new.  I think that if someone is using this API wrong, we should make it obvious in a debug build (there is no use case when it'd be legal to call this, so they'd have a real bug in their code).

Everything else has been fixed.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

For checkin.

Comment 9

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/d546bd2c46a4

Comment 10

[Shawn Wilsher :sdwilsh]

backed this out because we only return one row in the old case even if you have more than one row.  Oops.

I'll attach a patch that has a test for this, and the fix.

Comment 11

[Shawn Wilsher :sdwilsh]

For my own reference, we a do-while on hasResults in executeAndProcessStatement.

Comment 12

[Shawn Wilsher :sdwilsh]

bug 488148 was actually the cause of the failure.

Comment 13

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/e6b53112a17c

Comment 14

[Eric Shepherd [:sheppy]]

I've added initial documentation to MDC, and would appreciate a review. Also, do we have a good, readable code sample somewhere? The tests are a little convoluted and contrived (which is usually, by necessity, the case).

The documentation is here:

https://developer.mozilla.org/en/Storage#Binding_multiple_parameters
https://developer.mozilla.org/en/mozIStorageBindingParams
https://developer.mozilla.org/en/mozIStorageBindingParamsArray

And the first two methods here:

https://developer.mozilla.org/en/mozIStorageStatement

Comment 15

[Shawn Wilsher :sdwilsh]

I've modified https://developer.mozilla.org/en/Storage#Binding_Parameters making it a bit more structured and beefed up the example code.  Also clarified some points that were wrong.

Updated https://developer.mozilla.org/en/mozIStorageBindingParams to use the storage templates so all the wording is consistent throughout all the documents (feel free to tweak those templates).

Look good?

Comment 16

[Eric Shepherd [:sheppy]]

Looks snazzy - thank you!