IndexedDB: Keys placed on an object by an autoincremented objectStore ignore complex keyPaths

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted patch Patch (obsolete) — Splinter Review
if we put {} into an autoincrementing objectStore with keyPath "foo.bar", we'll get back { foo.bar: keyValue } instead of { foo: { bar: keyValue } }.
Attachment #577553 - Flags: review?(jonas)
Posted patch PatchSplinter Review
The right diff, this time.
Attachment #577553 - Attachment is obsolete: true
Attachment #577553 - Flags: review?(jonas)
Attachment #577554 - Flags: review?(jonas)
Comment on attachment 577554 [details] [diff] [review]
Patch

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +926,5 @@
>  
>      rv = GetKeyFromValue(aCx, aValue, mKeyPath, aKey);
> +    // If we're an autoincrementing objectStore, this call can fail.
> +    if (!mAutoIncrement) {
> +      NS_ENSURE_SUCCESS(rv, rv);

Nit: I'd prefer:

  if (NS_FAILED(rv) && !mAutoIncrement) {
    return rv;
  }

@@ +948,5 @@
>  
>    const jschar* keyPathChars =
>      reinterpret_cast<const jschar*>(mKeyPath.get());
>    const size_t keyPathLen = mKeyPath.Length();
> +  nsString firstToken, lastToken;

nsDependentSubstring I think

@@ +965,5 @@
> +
> +    // Recursively define empty objects on the object until we're at the
> +    // end of the keyPath.
> +    while(tokenizer.hasMoreTokens()) {
> +      nsString token(tokenizer.nextToken());

const nsDependentString&

@@ +989,5 @@
> +          break;
> +        }
> +
> +        obj = dummy;
> +      }

Hm, so, you're setting firstToken and lastToken before trying to define that property... If that fails, you'll still leave them set. I think you should probably set these after the prop set succeeds?

@@ +992,5 @@
> +        obj = dummy;
> +      }
> +    }
> +
> +    JSObject* dummy = JS_NewObject(aCx, &gDummyPropClass, nsnull, nsnull);

You probably want to put all this inside an |if (NS_SUCCEEDED(rv))| block in case something in the loop failed right?
Comment on attachment 577554 [details] [diff] [review]
Patch

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

r=me with those changes.

Though would you mind if I land this on top of my other patches? I can do the merging if you attach a updated patch.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +927,5 @@
>      rv = GetKeyFromValue(aCx, aValue, mKeyPath, aKey);
> +    // If we're an autoincrementing objectStore, this call can fail.
> +    if (!mAutoIncrement) {
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }

This change should no longer be needed

@@ +965,5 @@
> +
> +    // Recursively define empty objects on the object until we're at the
> +    // end of the keyPath.
> +    while(tokenizer.hasMoreTokens()) {
> +      nsString token(tokenizer.nextToken());

use |const nsDependentString&|

@@ +971,5 @@
> +      if (firstToken.IsEmpty()) {
> +        firstToken = token;
> +      }
> +
> +      lastToken = token;

You can wrap this in a

if (!tokenizer.hasMoreTokens())

Especially since you can then turn the below if-statement into an else

@@ +1001,5 @@
> +    jsval key = OBJECT_TO_JSVAL(dummy);
> +
> +    if (!JS_DefineUCProperty(aCx, obj, lastToken.get(), lastToken.Length(),
> +                             key, nsnull, nsnull, JSPROP_ENUMERATE)) {
> +      rv = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Actually, you can just get rid of lastToken completely and do this inside an |else| after the tokenizer.hasMoreTokens() branch.
Attachment #577554 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #3)
> Comment on attachment 577554 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> Review of attachment 577554 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r=me with those changes.
> 
> Though would you mind if I land this on top of my other patches? I can do
> the merging if you attach a updated patch.

I'm not going to update this before I'm gone for the weekend.  Just land your stuff and I'll deal with the pain next week.
Posted patch Followup fixSplinter Review
This is a followup fix.

The problem with the previous patch is that it didn't handle the case when the keypath was partially there.

So if you have a keypath like "foo.id" and attempting to store an object like:

{ foo: {} }

There's also error conditions like

{ foo: { id: /x/ } }

To avoid walking the keypath multiple times (once to try to look for an existing key value, and once to find where to start inserting objects) this patch combines looking for a pre-set value with setting up the new value if it doesn't exist.
Attachment #578776 - Flags: review?(bent.mozilla)
This is Kyle's patch with review comments from me and bent fixed, and rebased on top of other patches i'm about to land.
Attachment #578777 - Flags: review+
Comment on attachment 578776 [details] [diff] [review]
Followup fix

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

r=me!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +979,5 @@
> +        if (hasProp) {
> +          // Get if the property exists...
> +          jsval intermediate;
> +          JSBool ok = JS_GetUCProperty(aCx, obj,
> +                                       keyPathChars, keyPathLen, &intermediate);

Nit, rewrap these args.

@@ +983,5 @@
> +                                       keyPathChars, keyPathLen, &intermediate);
> +          NS_ENSURE_TRUE(ok, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +          // ...and if it's the last step in the path use it as key...
> +          if (!tokenizer.hasMoreTokens()) {

Nit: Since you're going to have a block for both cases please swap these and have the non-negated condition here.

@@ +988,5 @@
> +            aKey.SetFromJSVal(aCx, intermediate);
> +            if (aKey.IsUnset()) {
> +              return NS_ERROR_DOM_INDEXEDDB_DATA_ERR;
> +            }
> +            NS_ENSURE_SUCCESS(rv, rv);

This shouldn't be here, right? You're not changing rv anywhere...

@@ +1007,3 @@
>          }
>        }
> +      if (targetObject) {

Nit, add an empty line above here.
Attachment #578776 - Flags: review?(bent.mozilla) → review+
Re-landed via inbound:
https://hg.mozilla.org/mozilla-central/rev/82e7bc80eb49
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
You need to log in before you can comment on or make changes to this bug.