IndexedDB: Still have some issues with complex keyPaths

RESOLVED FIXED

Status

()

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

People

(Reporter: sicking, Assigned: sicking)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Attachment #578072 - Flags: review?(khuey)
Created attachment 578073 [details] [diff] [review]
Support empty keypaths on object stores
Assignee: nobody → jonas
Attachment #578073 - Flags: review?(bent.mozilla)
Attachment #578072 - Flags: review?(khuey) → review+
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"...
Attachment #578073 - Flags: review?(bent.mozilla) → review+

Comment 3

6 years ago
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 :-)
https://hg.mozilla.org/mozilla-central/rev/62131fc525d4
https://hg.mozilla.org/mozilla-central/rev/b6fb93ef1aee
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.