Closed
Bug 706068
Opened 13 years ago
Closed 13 years ago
IndexedDB: Keys placed on an object by an autoincremented objectStore ignore complex keyPaths
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(3 files, 1 obsolete file)
7.30 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
12.87 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
4.12 KB,
patch
|
sicking
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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.
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+
Comment 8•13 years ago
|
||
This landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/56aba64236d8 But was backed out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a2106940402
Comment 9•13 years ago
|
||
Re-landed via inbound: https://hg.mozilla.org/mozilla-central/rev/82e7bc80eb49
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Assignee | ||
Updated•12 years ago
|
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•