706659 - IndexedDB: Still have some issues with complex keyPaths

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: 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]

Attachment: Support empty keypaths on object stores

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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]]

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 :-)

Comment 4

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/62131fc525d4
https://hg.mozilla.org/mozilla-central/rev/b6fb93ef1aee