328884 - mozStorageStatementWrapper.reset semantics very confusing

Status: RESOLVED WONTFIX

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

I recently managed to introduce bug 328773 into the calendar code because I figured that there would be no need to reset a throwaway mozStorageStatementWrapper after I was done with it.  As it turns out, the current semantics not only reset the statement in question but also do special magic that is required for subsequent statements to work.

Commenting the IDL to this effect would be a fine start, but even better might be to clarify the API itself.  Perhaps this would just be renaming "reset" to "finish", or maybe splitting the finish semantics of reset out into their own method.

Comment 1

[Shawn Wilsher :sdwilsh]

dmose - can you clarify this a bit please?

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Being required to call a method named "reset" to ensure that a method called "execute" invoked earlier actually completes seems exceedingly non-intuitive to me.  In particular, it seems to me that people new to coding with these APIs are likely to overlook the need to call reset and have an unpleasant experience trying to figure out why their code doesn't work.  I think this is especially likely because people often copy-and-paste code when they're new to something, and they may see the "reset" and think (because of its name) "I'm not gonna use this statement again; no need to copy that chunk". It seems like the things I suggested in comment 0 would mitigate that problem.

Comment 3

[Shawn Wilsher :sdwilsh]

If you are in JS, you can use .step() so that you never have to worry about calling reset (it does it for you automatically once you've processed all the results).

If you are in native code, you should be using mozStorageStatementScoper which will reset the statement for you when it falls out of scope.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

I agree, it's completely possible to use the APIs in a reasonable way.  The problem that I perceive is that the current API framing makes it unnecessarily easy for platform/extension newbies to stub their toes in a data-loss-y kind of way.  I don't think it would take much effort to mitigate that problem.

Comment 5

[Shawn Wilsher :sdwilsh]

Yeah, you are probably right.  executeStep should probably just do exactly what step() does.  It's not clear to me why it doesn't.

Comment 6

[Shawn Wilsher :sdwilsh]

So, while this API isn't the greatest, I'm going to WONTFIX this for two reasons:
1) The proper (and documented now on mdc) way to use storage in JS should be to wrap execution in a try block, and to reset in finally.  This is because if an error occurs, you need to reset.
2) We really really really want people to use the asynchronous API instead of the synchronous one, and that doesn't have this problem.  I'm more interested in supporting and making API's that are sane and smart work well, instead of changing old ones to work better that people shouldn't be using anyway.