Last Comment Bug 706659 - IndexedDB: Still have some issues with complex keyPaths
: IndexedDB: Still have some issues with complex keyPaths
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
Depends on:
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-11-30 13:57 PST by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2012-03-22 11:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't throw if an index value can't be found (12.51 KB, patch)
2011-11-30 13:57 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
bent.mozilla: review+
Details | Diff | Splinter Review
Support empty keypaths on object stores (20.79 KB, patch)
2011-11-30 13:58 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
bent.mozilla: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-30 13:57:35 PST
Created attachment 578072 [details] [diff] [review]
Don't throw if an index value can't be found

First of we don't support empty keyPaths on objectStores

Second, the following code throws:

store = db.createObjectStore("mystore");
store.createIndex("myindex", "x.y");
store.add({ x: 17 }, 1);

This shouldn't throw, but simply not insert an entry in the index.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-30 13:58:30 PST
Created attachment 578073 [details] [diff] [review]
Support empty keypaths on object stores
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-30 17:02:21 PST
Comment on attachment 578073 [details] [diff] [review]
Support empty keypaths on object stores

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

Remember to add a test that checks that empty keyPath doesn't return null.

::: dom/indexedDB/DatabaseInfo.h
@@ +136,5 @@
>  
>    nsString name;
>    PRInt64 id;
>    nsString keyPath;
> +  bool hasKeyPath;

Let's use keyPath.IsVoid() instead!

::: dom/indexedDB/IDBDatabase.cpp
@@ +803,5 @@
>  {
>    nsCOMPtr<mozIStorageStatement> stmt =
>      mTransaction->GetCachedStatement(NS_LITERAL_CSTRING(
> +    "INSERT INTO object_store (id, auto_increment, name, key_path) "
> +    "VALUES (:id,  :auto_increment, :name, :key_path)"

Extra space. boo.

@@ +827,5 @@
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +  }
> +  else {
> +    rv = stmt->BindNullByName(NS_LITERAL_CSTRING("key_path"));
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

Nit: I'd do:

  rv = mObjectStore->HasKeyPath() ?
       stmt->BindStringByName() :
       stmt->BindNullByName();
  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +800,5 @@
> +
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT INTO object_store "
> +      "SELECT id, auto_increment, name, "
> +      "CASE WHEN key_path=='' THEN NULL ELSE key_path END"

..."key_path = '' THEN"...
Comment 3 Ed Morley [:emorley] 2011-12-03 04:21:55 PST
This landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9ca56d4aab
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fddf8667d21

But was backed out again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a2106940402

To reduce confusion for people merging inbound, please can you:
- Annotate the bugs with inbound changeset URLs and also the backout
- List all changeset hashes in the backout commit message (we ended up having to diff backouts vs guesses of changesets to work out what you'd backed out) + ideally the bug numbers too. Mak's backout script (http://blog.bonardo.net/2011/08/05/easier-backout-scripts) will do this for you automatically & makes backing out multiple changesets trivial (eg in this case, running simply: backout 56aba64236d8:1b9ca56d4aab). 

Thanks :-)

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