Last Comment Bug 692627 - IndexedDB: Support complex key paths
: IndexedDB: Support complex key paths
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
:
Mentors:
http://dvcs.w3.org/hg/IndexedDB/raw-f...
Depends on:
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-10-06 15:41 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2015-12-01 05:01 PST (History)
8 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (13.81 KB, patch)
2011-11-15 09:06 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (13.81 KB, patch)
2011-11-15 15:12 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (17.50 KB, patch)
2011-11-16 04:44 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch (17.45 KB, patch)
2011-11-18 03:56 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Final patch (17.61 KB, patch)
2011-11-21 06:50 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-06 15:41:12 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-14 13:22:02 PST
Jonas says that just foo.bar is enough here, no need to do array indexes or anything.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-15 09:06:51 PST
Created attachment 574603 [details] [diff] [review]
Patch
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-15 09:08:32 PST
Comment on attachment 574603 [details] [diff] [review]
Patch

And njn for the jsapi bits.
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-15 09:52:45 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-15 10:02:48 PST
Comment on attachment 574603 [details] [diff] [review]
Patch

Fine, I'll write some tests :-P
Comment 6 Nicholas Nethercote [:njn] 2011-11-15 12:08:35 PST
BTW, dmandelin's a better person to ask for review of JSAPI stuff, I'm not very familiar with it.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-15 15:12:35 PST
Created attachment 574707 [details] [diff] [review]
Patch
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-16 04:44:15 PST
Created attachment 574873 [details] [diff] [review]
Patch

And I managed to reattach the original diff.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-16 22:26:03 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-18 03:56:45 PST
Created attachment 575420 [details] [diff] [review]
Patch

bent's comments addressed.
Comment 11 Jason Orendorff [:jorendorff] 2011-11-18 11:56:25 PST
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] 2011-11-18 17:39:27 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-21 06:50:17 PST
Created attachment 575848 [details] [diff] [review]
Final patch

Not sure if the js guys wanted to look at this again.
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-21 06:57:36 PST
https://hg.mozilla.org/mozilla-central/rev/763d63759721
Comment 15 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-12-01 05:01:39 PST
I've added a note to the relevant release notes:

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

Note You need to log in before you can comment on or make changes to this bug.