485071 - implement mozStorageStatementParams::NewEnumerate

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[David Dahl :ddahl]

This would help in being able to dynamically get metadata from any statement.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

If someone feels crazy, they can try to run with this, but I'm tired of banging my head on this trying to get it to work for now.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Talked it over with mrbkap last night, and this fixes things.  Once bent looks this over, I'm going to update NewEnumerate on https://developer.mozilla.org/En/NsIXPCScriptable

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

>+            // Start our internal index at zero.
>+            *statep = INT_TO_JSVAL(0);

JSVAL_ZERO

>+            JSString *jsname = JS_NewStringCopyN(cx, &(name.get()[1]),
>+                                                 name.Length() - 1);

This will be all sorts of bad if GetParameterName fails or somehow returns an empty string. Need to guard against that. Also I'm not a huge fan of manual buffer calculations though I don't care a whole lot, maybe use nsDependentSubstring? Up to you.

>+            NS_ENSURE_TRUE(jsname, NS_ERROR_OUT_OF_MEMORY);
>+
>+            // Set our name.
>+            JS_ValueToId(cx, STRING_TO_JSVAL(jsname), idp);

This can fail, you should check and act accordingly.

>+    // We do not throw any point after this because we want to allow the

"do not throw _at_ any point"

>+    *_retval = PR_TRUE;

jst and mrbkap assure me that this is not necessary, remove it.

>+        *_retval = JS_DefineUCProperty(cx, obj, nameChars, nameLength,
>+                                       JSVAL_VOID, nsnull, nsnull, 0);
>+        NS_ENSURE_TRUE(_retval, NS_OK);

This is incorrect, you want *_retval.

>+    // Ensure that our index is within range.  Here we do not check the rest of
>+    // the prototype chain.

This directly contradicts your earlier comment and is confusing.

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> This will be all sorts of bad if GetParameterName fails or somehow returns an
> empty string. Need to guard against that. Also I'm not a huge fan of manual
> buffer calculations though I don't care a whole lot, maybe use
> nsDependentSubstring? Up to you.
I'll add an assertion so ensure that the Length is greater than one, but this should absolutely be safe (and this is why I'm confident about not checking the return variable when calling it).  We check the one case where this could fail (index out of bounds).

> >+            // Set our name.
> >+            JS_ValueToId(cx, STRING_TO_JSVAL(jsname), idp);
> 
> This can fail, you should check and act accordingly.
Neat - there's a bug here then:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#9155

> This directly contradicts your earlier comment and is confusing.
Right - I updated the comment at the top, and moved this check into the JSVAL_IS_INT check, because that's the only place it could be bad anyway.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> This is incorrect, you want *_retval.
Apparently I didn't save before qrefreshing this change.  It is fixed locally.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

uploading a new version for good measure

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

>  We check the one case where this could fail (index out of bounds).

Looking at the function I see two other cases where it could fail:

> mozStorageStatement::GetParameterName(PRUint32 aParamIndex, ...
> {
>   if (!mDBConnection || !mDBStatement)
>     return NS_ERROR_NOT_INITIALIZED;

I think it would be much smarter in the long run to actually guard against this - either pre-set the stack string or check the return value.

> Neat - there's a bug here then:

Please file a followup.

> Right - I updated the comment at the top, and moved this check into the
> JSVAL_IS_INT check, because that's the only place it could be bad anyway

So this will make it possible for GetParameterIndex to return something that may be out of bounds for mParamCount, is that safe?

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> > mozStorageStatement::GetParameterName(PRUint32 aParamIndex, ...
> > {
> >   if (!mDBConnection || !mDBStatement)
> >     return NS_ERROR_NOT_INITIALIZED;
Hmm, we need to stop checking against mDBConnection since we got rid of Initialize in the idl (filed bug 485223).  It could be an assertion I guess though.  As for mDBStatement, it'd only be null if they called finalize.  I guess we can protect against that.

> Please file a followup.
Bug 485220

> So this will make it possible for GetParameterIndex to return something that
> may be out of bounds for mParamCount, is that safe?
If GetParameterIndex returns an index that is out of bounds, it's horribly broken.  It returns a failure code, or a valid index, and we check for failure codes here.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Comment 11

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

Comment on attachment 369323 [details] [diff] [review]
v1.3

This really doesn't work with ?nnn syntax.  I'm not sure it has to, but I think a more intuitive failure model is required at minimum.

Enumeration nearly works on consecutively numbered ?nnn syntax.  Overlooking the off-by-one index problem, the main glitch is that the off-by-one causes the enumeration to choke when the enumeration hits 'n' because of the numeric coercion and the enumeration's tendency to use OBJ_LOOKUP_PROPERTY which invokes NewResolve, sees the value == mParamCount, and explodes with a NS_ERROR_INVALID_ARG.

Enumeration fails bizarrely with ?nnn syntax with gaps in the sequence before it runs afoul of the mParamCount issue.  This is because GetParameterName makes up its own zero-based name when sqlite3_bind_parameter_name returns null.  But when using ?nnn syntax, null is what we expect for gaps.

Regrettably, I think that any solution, whether it makes enumeration truly work, or just explodes immediately before returning confusing results, needs to understand the binding idiom in use (anonymous '?' versus '?nnn' versus ':aaa'/'@aaa'/'$aaa').  This could be inferred by retrieving the name for the index at the last parameter (per sqlite_bind_parameter_count).  If the name is null, we are using anonymous parameters.  If the name's first character is "?" it's ?nnn syntax, otherwise :aaa/@aaa/$aaa.  (We have to check the last parameter because only that is guaranteed to not be a gap.)

This would make it feasible, if ugly, to do the right thing for ?nnn.  Because of the gaps you'd need a while loop in there, but at least you could avoid having to atoi the strings (since you already know the number, it would just be redundant work).

Also, since you explicitly cite correctness of prototype lookup, you probably want a unit test like so:
  // make sure prototype lookup is okay...
  let arbitraryObj = {};
  stmt.__proto__ = {d: arbitraryObj};
  do_check_eq(stmt.d, arbitraryObj);

(or maybe that's wrong, in which case I don't understand how you'd get something in your prototype chain...)

Comment 12

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

Also, the unit test probably wants to actually check that it can actually get values out of 'params' or I'm not sure why it can enumerate in the first place.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> (From update of attachment 369323 [details] [diff] [review])
> This really doesn't work with ?nnn syntax.  I'm not sure it has to, but I think
> a more intuitive failure model is required at minimum.
The params and row object have always been pushed along with using named parameters.  In fact, I'd even be willing to drop the index support on mozilla-central to make this more sensible.

> Regrettably, I think that any solution, whether it makes enumeration truly
> work, or just explodes immediately before returning confusing results, needs to
> understand the binding idiom in use (anonymous '?' versus '?nnn' versus
> ':aaa'/'@aaa'/'$aaa').  This could be inferred by retrieving the name for the
> index at the last parameter (per sqlite_bind_parameter_count).  If the name is
> null, we are using anonymous parameters.  If the name's first character is "?"
> it's ?nnn syntax, otherwise :aaa/@aaa/$aaa.  (We have to check the last
> parameter because only that is guaranteed to not be a gap.)
I think we should make it clear that storage only supports named parameters of type ":aaa", and we've been sorta making that assumption all along in mozStorageStatement::GetParameterName.  I'm happy with making this more explicit.


> Also, since you explicitly cite correctness of prototype lookup, you probably
> want a unit test like so:
Well, we could check for toString on the prototype, but I can assure you that the current unit test does not pass if we do not lookup the prototype chain.  When we try to enumerate, NewResolve gets called for the property __iterator__, which we end up failing to get an index for.  We also get QueryInterface called during this time (I'm not sure why though).

(In reply to comment #12)
> Also, the unit test probably wants to actually check that it can actually get
> values out of 'params' or I'm not sure why it can enumerate in the first place.
I think the point of enumerating would be something like what the password manager does.  It iterates over properties in an object to bind them to a statement.  However, it has to be careful to only have the right properties in the object, otherwise binding fails (it has some semi-complicated code to deal with this every time it creates an object to pass in as a result).  However, it could change it's code to iterate over the available bindable properties and no longer have to have complicated code elsewhere.  This was the use case I was thinking of, but I imagine ddahl has another use case in mind too.

Comment 14

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

(In reply to comment #13)
> The params and row object have always been pushed along with using named
> parameters.  In fact, I'd even be willing to drop the index support on
> mozilla-central to make this more sensible.

> I think we should make it clear that storage only supports named parameters of
> type ":aaa", and we've been sorta making that assumption all along in
> mozStorageStatement::GetParameterName.  I'm happy with making this more
> explicit.

https://developer.mozilla.org/en/Storage uses ?nnn all over the place.  I feel like this creates an implied contract for all of 1.9.x.
 
> Well, we could check for toString on the prototype, but I can assure you that
> the current unit test does not pass if we do not lookup the prototype chain. 

Works for me.

> I think the point of enumerating would be something like what the password
> manager does.  It iterates over properties in an object to bind them to a
> statement.  However, it has to be careful to only have the right properties in
> the object, otherwise binding fails (it has some semi-complicated code to deal
> with this every time it creates an object to pass in as a result).  However, it
> could change it's code to iterate over the available bindable properties and no
> longer have to have complicated code elsewhere.  This was the use case I was
> thinking of, but I imagine ddahl has another use case in mind too.

Ah.  See, I had assumed the point was for debugging support.  In which case, it would make sense that we would attempt to support statements and their parameters in their many forms.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> https://developer.mozilla.org/en/Storage uses ?nnn all over the place.  I feel
> like this creates an implied contract for all of 1.9.x.
I have an open bug to go over and update all that documentation.  Nobody every used the statement wrapper really because you had to explicitly create it, but not it happens automagically.  I also think that encouraging the use of named parameters when using the js language helpers is a good idea.  It makes for more readable code while ends up introducing less bugs.

C++ is still going to use numbered parameters, but we really don't have a good API for it to use.

Also, I don't think it's common (or a good idea) to skip a step with numbered parameters.  But really, with JS, we should just encourage the use of named parameters and continue to support the old way of binding by index (I'll note that the logic there about not being able to skip an index has not changed since 1.8.1 AFAIK).

Comment 16

[Shawn Wilsher :sdwilsh]

> Nobody every
> used the statement wrapper really because you had to explicitly create it, but
> not it happens automagically.
er, this should read "Nobody ever used the statement wrapper because they had to explicitly create it, but now it happens automagically.

Comment 17

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

Comment on attachment 369323 [details] [diff] [review]
v1.3

With changes to the documentation to discourage use of ?nnn (and avoiding using it in examples), this is cool.

Comment 18

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

Which is to say this can land now and the documentation can happen later, unless this patch for some odd reason wants to get on 1.9.1.  In that case, the documentation should happen before the backport lands.

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> Which is to say this can land now and the documentation can happen later,
> unless this patch for some odd reason wants to get on 1.9.1.  In that case, the
> documentation should happen before the backport lands.
not looking to land this for 1.9.1, but I'll be doing the documentation soon anyway.

Comment 20

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/136b80e1082b

Comment 21

[Shawn Wilsher :sdwilsh]

Documentation about named parameters covered a bit here:
https://developer.mozilla.org/en/Storage#Binding_Parameters

Comment 22

[Shawn Wilsher :sdwilsh]

Completed docs here.  I just rewrote the whole page since it was useless before.
https://developer.mozilla.org/En/MozIStorageStatementParams