692627 - IndexedDB: Support complex key paths

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Writing something like "foo.bar.baz" should get the baz property of the bar property of the foo property.

Parsing of it defers to the ecmascript spec.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Jonas says that just foo.bar is enough here, no need to do array indexes or anything.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 574603 [details] [diff] [review]
Patch

And njn for the jsapi bits.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. can you also test that we actually correctly evaluate the key path :) Especially edge cases like "foo.bar.baz" when the foo or bar objects don't exist, as well as the "" keypath.

And please add a test that checks that "foo.2.bar" throws.

We also need tests that checks that key paths work correctly on indexes.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 574603 [details] [diff] [review]
Patch

Fine, I'll write some tests :-P

Comment 6

[Nicholas Nethercote [inactive]]

BTW, dmandelin's a better person to ask for review of JSAPI stuff, I'm not very familiar with it.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

And I managed to reattach the original diff.

Comment 9

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

Comment on attachment 574873 [details] [diff] [review]
Patch

Review of attachment 574873 [details] [diff] [review]:
-----------------------------------------------------------------

You'll need a js peer to sign off on the js/src changes.

r=me on the dom bits!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +427,2 @@
>  {
> +  NS_PRECONDITION(aCx, "Null pointer!");

Ugh, since you're changing it, NS_ASSERTION.

@@ +427,3 @@
>  {
> +  NS_PRECONDITION(aCx, "Null pointer!");
> +  NS_ASSERTION(JSVAL_IS_OBJECT(aVal), "Why are we here!?");

You actually want !JSVAL_IS_PRIMITIVE. We can't tolerate JSVAL_NULL either.

@@ +427,5 @@
>  {
> +  NS_PRECONDITION(aCx, "Null pointer!");
> +  NS_ASSERTION(JSVAL_IS_OBJECT(aVal), "Why are we here!?");
> +  NS_ASSERTION(IDBObjectStore::IsValidKeyPath(aCx, aKeyPath),
> +               "This will explode!");

Hm, how do you ensure this? I think you should check and return a suitable error code here.

@@ +429,5 @@
> +  NS_ASSERTION(JSVAL_IS_OBJECT(aVal), "Why are we here!?");
> +  NS_ASSERTION(IDBObjectStore::IsValidKeyPath(aCx, aKeyPath),
> +               "This will explode!");
> +
> +  nsCharSeparatedTokenizerTemplate<IgnoreWhitespace> tokenizer(aKeyPath, '.');

This is a bit tedious, can you add a typedef for this just below the IgnoreWhitespace def? Maybe call it Tokenizer?

@@ +431,5 @@
> +               "This will explode!");
> +
> +  nsCharSeparatedTokenizerTemplate<IgnoreWhitespace> tokenizer(aKeyPath, '.');
> +
> +  JSObject* obj;

Nit: You don't need this. Just use JSVAL_TO_OBJECT as the arg in the call to JS_GetUCProperty().

@@ +437,5 @@
> +  while (tokenizer.hasMoreTokens()) {
> +    nsString token(tokenizer.nextToken());
> +
> +    if (!token.Length()) {
> +      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Spec says any key path validation error should throw SyntaxError (NS_ERROR_DOM_SYNTAX_ERR).

@@ +440,5 @@
> +    if (!token.Length()) {
> +      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +    }
> +
> +    const jschar* keyPathChars = reinterpret_cast<const jschar*>(token.get());

I don't think this cast is necessary any more (Luke fixed something a little while ago, now they should be interchangeable).

@@ +443,5 @@
> +
> +    const jschar* keyPathChars = reinterpret_cast<const jschar*>(token.get());
> +    const size_t keyPathLen = token.Length();
> +
> +    if (!JSVAL_IS_OBJECT(intermediate)) {

JSVAL_IS_PRIMITIVE again

@@ +455,5 @@
> +    NS_ENSURE_TRUE(ok, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +  }
> +
> +  nsresult rv = aKey.SetFromJSVal(aCx, intermediate);
> +  if (NS_FAILED(rv)) {

Nit: Just do

  if (NS_FAILED(aKey.SetFromJSVal(aCx, intermediate))) {
    aKey.Unset();
  }

@@ +533,5 @@
>  }
>  
>  // static
> +bool
> +IDBObjectStore::IsValidKeyPath(JSContext* aCx, const nsAString& aKeyPath)

Nit, each arg on its own line.

@@ +535,5 @@
>  // static
> +bool
> +IDBObjectStore::IsValidKeyPath(JSContext* aCx, const nsAString& aKeyPath)
> +{
> +  if (aKeyPath.IsVoid()) {

This should be asserted. No one should be passing around void strings.

@@ +553,5 @@
> +    if (!xpc_qsStringToJsval(aCx, token, &stringVal)) {
> +      return false;
> +    }
> +
> +    MOZ_ASSERT(JSVAL_IS_STRING(stringVal));

Just use NS_ASSERTION. At some point we'll need to fix all this.

@@ +563,5 @@
> +  }
> +
> +  // If the very last character was a '.', the tokenizer won't give us an empty
> +  // token, but the keyPath is still invalid.
> +  if (aKeyPath.Length() &&

It's weird, but I prefer !IsEmpty().

@@ +564,5 @@
> +
> +  // If the very last character was a '.', the tokenizer won't give us an empty
> +  // token, but the keyPath is still invalid.
> +  if (aKeyPath.Length() &&
> +      aKeyPath.CharAt(NS_MAX<PRUint32>(0, aKeyPath.Length() - 1)) == '.') {

Wait, you know already that it's > 0. Lose the NS_MAX.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

bent's comments addressed.

Comment 11

[Jason Orendorff [:jorendorff]]

We try to make it very clear to jsapi.h users whether or not an error occurred. There are three possible outcomes for JS_IsIdentifier: yes, no, and error. So change the function to use a JSBool out parameter. It should return true on success with the yes or no answer stored in the out param. 

House style in js/src is not to assert that a pointer isn't null if it's about to be used anyway. So delete these lines:
    JS_ASSERT(cx);
    JS_ASSERT(str);

But add this for compartment safety:
    assertSameCompartment(cx, str);

Thanks!

Comment 12

[David Mandelin [:dmandelin]]

Comment on attachment 575420 [details] [diff] [review]
Patch

Jason already gave a complete review of the JS part (thanks!), so you can just go by that.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Final patch

Not sure if the js guys wanted to look at this again.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/763d63759721

Comment 15

[Chris Mills [:cmills]]

I've added a note to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/11#IndexedDB