The default bug view has changed. See this FAQ.

IndexedDB: Support complex key paths

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: sicking, Assigned: khuey)

Tracking

({dev-doc-complete})

unspecified
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

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.
Jonas says that just foo.bar is enough here, no need to do array indexes or anything.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 574603 [details] [diff] [review]
Patch
Attachment #574603 - Flags: review?(bent.mozilla)
Comment on attachment 574603 [details] [diff] [review]
Patch

And njn for the jsapi bits.
Attachment #574603 - Flags: review?(nnethercote)
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 on attachment 574603 [details] [diff] [review]
Patch

Fine, I'll write some tests :-P
Attachment #574603 - Attachment is obsolete: true
Attachment #574603 - Flags: review?(nnethercote)
Attachment #574603 - Flags: review?(bent.mozilla)
BTW, dmandelin's a better person to ask for review of JSAPI stuff, I'm not very familiar with it.
Created attachment 574707 [details] [diff] [review]
Patch
Attachment #574707 - Flags: review?(bent.mozilla)
Created attachment 574873 [details] [diff] [review]
Patch

And I managed to reattach the original diff.
Attachment #574707 - Attachment is obsolete: true
Attachment #574707 - Flags: review?(bent.mozilla)
Attachment #574873 - Flags: review?(bent.mozilla)
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.
Attachment #574873 - Flags: review?(bent.mozilla) → review+
Created attachment 575420 [details] [diff] [review]
Patch

bent's comments addressed.
Attachment #574873 - Attachment is obsolete: true
Attachment #575420 - Flags: review?
Attachment #575420 - Flags: review? → review?(dmandelin)
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 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.
Attachment #575420 - Flags: review?(dmandelin)
Created attachment 575848 [details] [diff] [review]
Final patch

Not sure if the js guys wanted to look at this again.
Attachment #575420 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/763d63759721
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Keywords: dev-doc-needed
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
Version: Trunk → unspecified
I've added a note to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/11#IndexedDB
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.