IndexedDB: IDBCursor.update() should raise exception if key changed

https://bugs.webkit.org/show_bug.cgi?id=76952

Source/WebCore:

Move the test from the async task to the synchronous call, per spec. Also re-ordered the tests
done during the synchronous call and the asynchronous task to follow the spec order.

Reviewed by Tony Chang.

Tests: storage/indexeddb/cursor-update.html

* storage/IDBObjectStoreBackendImpl.cpp:
(WebCore::IDBObjectStoreBackendImpl::put): Added check during update() call, order checks per spec.
(WebCore::IDBObjectStoreBackendImpl::putInternal): Move effective key calculation inline.
* storage/IDBObjectStoreBackendImpl.h: Removed selectKeyForPut method.

LayoutTests:

Reviewed by Tony Chang.

* storage/indexeddb/cursor-update-expected.txt:
* storage/indexeddb/cursor-update.html:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@106387 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index c671fab..75a8529 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2012-01-31  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: IDBCursor.update() should raise exception if key changed
+        https://bugs.webkit.org/show_bug.cgi?id=76952
+
+        Reviewed by Tony Chang.
+
+        * storage/indexeddb/cursor-update-expected.txt:
+        * storage/indexeddb/cursor-update.html:
+
 2012-01-31  Ryosuke Niwa  <rniwa@webkit.org>
 
         Crash in previousLinePosition when moving into a root inline box without leaves

diff --git a/LayoutTests/storage/indexeddb/cursor-update-expected.txt b/LayoutTests/storage/indexeddb/cursor-update-expected.txt
index 4aebd54..0e21f8c 100644 (file)
--- a/LayoutTests/storage/indexeddb/cursor-update-expected.txt
+++ b/LayoutTests/storage/indexeddb/cursor-update-expected.txt
@@ -104,28 +104,28 @@ autoIncrementCheckCursor()
 PASS counter is 5
 trans.objectStore('keyPathStore').openCursor(keyRange)
 keyPathUpdateCursor()
-event.target.result.update({id: 100 + counter, number: 100 + counter})
-PASS event.target.errorCode is webkitIDBDatabaseException.DATA_ERR
-event.preventDefault()
-event.target.source.update({id: counter, number: 100 + counter++})
+Expecting exception from event.target.result.update({id: 100 + counter, number: 100 + counter})
+PASS Exception was thrown.
+PASS code is webkitIDBDatabaseException.DATA_ERR
+event.target.result.update({id: counter, number: 100 + counter++})
 event.target.source.continue()
 keyPathUpdateCursor()
-event.target.result.update({id: 100 + counter, number: 100 + counter})
-PASS event.target.errorCode is webkitIDBDatabaseException.DATA_ERR
-event.preventDefault()
-event.target.source.update({id: counter, number: 100 + counter++})
+Expecting exception from event.target.result.update({id: 100 + counter, number: 100 + counter})
+PASS Exception was thrown.
+PASS code is webkitIDBDatabaseException.DATA_ERR
+event.target.result.update({id: counter, number: 100 + counter++})
 event.target.source.continue()
 keyPathUpdateCursor()
-event.target.result.update({id: 100 + counter, number: 100 + counter})
-PASS event.target.errorCode is webkitIDBDatabaseException.DATA_ERR
-event.preventDefault()
-event.target.source.update({id: counter, number: 100 + counter++})
+Expecting exception from event.target.result.update({id: 100 + counter, number: 100 + counter})
+PASS Exception was thrown.
+PASS code is webkitIDBDatabaseException.DATA_ERR
+event.target.result.update({id: counter, number: 100 + counter++})
 event.target.source.continue()
 keyPathUpdateCursor()
-event.target.result.update({id: 100 + counter, number: 100 + counter})
-PASS event.target.errorCode is webkitIDBDatabaseException.DATA_ERR
-event.preventDefault()
-event.target.source.update({id: counter, number: 100 + counter++})
+Expecting exception from event.target.result.update({id: 100 + counter, number: 100 + counter})
+PASS Exception was thrown.
+PASS code is webkitIDBDatabaseException.DATA_ERR
+event.target.result.update({id: counter, number: 100 + counter++})
 event.target.source.continue()
 keyPathUpdateCursor()
 PASS counter is 5

diff --git a/LayoutTests/storage/indexeddb/cursor-update.html b/LayoutTests/storage/indexeddb/cursor-update.html
index 221b4d3..948f225 100644 (file)
--- a/LayoutTests/storage/indexeddb/cursor-update.html
+++ b/LayoutTests/storage/indexeddb/cursor-update.html
@@ -163,17 +163,11 @@ function keyPathUpdateCursor()
         return;
     }
 
-    request = evalAndLog("event.target.result.update({id: 100 + counter, number: 100 + counter})");
-    request.onsuccess = unexpectedSuccessCallback;
-    request.onerror = function() {
-        shouldBe("event.target.errorCode", "webkitIDBDatabaseException.DATA_ERR");
+    evalAndExpectException("event.target.result.update({id: 100 + counter, number: 100 + counter})", "webkitIDBDatabaseException.DATA_ERR");
 
-        evalAndLog("event.preventDefault()");
-
-        request = evalAndLog("event.target.source.update({id: counter, number: 100 + counter++})");
-        request.onsuccess = function() { evalAndLog("event.target.source.continue()") };
-        request.onerror = unexpectedErrorCallback;
-    }
+    request = evalAndLog("event.target.result.update({id: counter, number: 100 + counter++})");
+    request.onsuccess = function() { evalAndLog("event.target.source.continue()") };
+    request.onerror = unexpectedErrorCallback;
 }
 
 function keyPathCheckCursor()

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index b4f2a23..8c29558 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,20 @@
+2012-01-31  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: IDBCursor.update() should raise exception if key changed
+        https://bugs.webkit.org/show_bug.cgi?id=76952
+
+        Move the test from the async task to the synchronous call, per spec. Also re-ordered the tests
+        done during the synchronous call and the asynchronous task to follow the spec order.
+
+        Reviewed by Tony Chang.
+
+        Tests: storage/indexeddb/cursor-update.html
+
+        * storage/IDBObjectStoreBackendImpl.cpp:
+        (WebCore::IDBObjectStoreBackendImpl::put): Added check during update() call, order checks per spec.
+        (WebCore::IDBObjectStoreBackendImpl::putInternal): Move effective key calculation inline.
+        * storage/IDBObjectStoreBackendImpl.h: Removed selectKeyForPut method.
+
 2012-01-31  Anders Carlsson  <andersca@apple.com>
 
         Inform the tile cache whenever the visible rect changes

diff --git a/Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp b/Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp
index 51a90d0..ce042be 100644 (file)
--- a/Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp
+++ b/Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp
@@ -132,33 +132,32 @@ void IDBObjectStoreBackendImpl::put(PassRefPtr<SerializedScriptValue> prpValue,
     RefPtr<IDBTransactionBackendInterface> transaction = transactionPtr;
 
     if (putMode != CursorUpdate) {
-        if (key && !key->valid()) {
+        const bool autoIncrement = objectStore->autoIncrement();
+        const bool hasKeyPath = !objectStore->m_keyPath.isNull();
+
+        if (hasKeyPath && key) {
             ec = IDBDatabaseException::DATA_ERR;
             return;
         }
-        const bool autoIncrement = objectStore->autoIncrement();
-        const bool hasKeyPath = !objectStore->m_keyPath.isNull();
-        if (!key && !autoIncrement && !hasKeyPath) {
+        if (!hasKeyPath && !autoIncrement && !key) {
             ec = IDBDatabaseException::DATA_ERR;
             return;
         }
         if (hasKeyPath) {
-            if (key) {
+            RefPtr<IDBKey> keyPathKey = fetchKeyFromKeyPath(value.get(), objectStore->m_keyPath);
+            if (keyPathKey && !keyPathKey->valid()) {
                 ec = IDBDatabaseException::DATA_ERR;
                 return;
             }
-
-            RefPtr<IDBKey> keyPathKey = fetchKeyFromKeyPath(value.get(), objectStore->m_keyPath);
-            if (!autoIncrement) {
-                if (!keyPathKey || !keyPathKey->valid()) {
-                    ec = IDBDatabaseException::DATA_ERR;
-                    return;
-                }
-            } else if (keyPathKey && !keyPathKey->valid()) {
+            if (!autoIncrement && !keyPathKey) {
                 ec = IDBDatabaseException::DATA_ERR;
                 return;
             }
         }
+        if (key && !key->valid()) {
+            ec = IDBDatabaseException::DATA_ERR;
+            return;
+        }
         for (IndexMap::iterator it = m_indexes.begin(); it != m_indexes.end(); ++it) {
             const RefPtr<IDBIndexBackendImpl>& index = it->second;
             RefPtr<IDBKey> indexKey = fetchKeyFromKeyPath(value.get(), index->keyPath());
@@ -167,73 +166,64 @@ void IDBObjectStoreBackendImpl::put(PassRefPtr<SerializedScriptValue> prpValue,
                 return;
             }
         }
+    } else {
+        ASSERT(key);
+        const bool hasKeyPath = !objectStore->m_keyPath.isNull();
+        if (hasKeyPath) {
+            RefPtr<IDBKey> keyPathKey = fetchKeyFromKeyPath(value.get(), objectStore->m_keyPath);
+            if (!keyPathKey || !keyPathKey->isEqual(key.get())) {
+                ec = IDBDatabaseException::DATA_ERR;
+                return;
+            }
+        }
     }
 
     if (!transaction->scheduleTask(createCallbackTask(&IDBObjectStoreBackendImpl::putInternal, objectStore, value, key, putMode, callbacks, transaction)))
         ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;
 }
 
-PassRefPtr<IDBKey> IDBObjectStoreBackendImpl::selectKeyForPut(IDBObjectStoreBackendImpl* objectStore, IDBKey* key, PutMode putMode, IDBCallbacks* callbacks, RefPtr<SerializedScriptValue>& value)
+void IDBObjectStoreBackendImpl::putInternal(ScriptExecutionContext*, PassRefPtr<IDBObjectStoreBackendImpl> objectStore, PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBKey> prpKey, PutMode putMode, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<IDBTransactionBackendInterface> transaction)
 {
-    if (putMode == CursorUpdate)
-        ASSERT(key);
-
-    const bool autoIncrement = objectStore->autoIncrement();
-    const bool hasKeyPath = !objectStore->m_keyPath.isNull();
-
-    if (autoIncrement && key) {
-        objectStore->resetAutoIncrementKeyCache();
-        return key;
-    }
-
-    if (autoIncrement) {
-        ASSERT(!key);
-        if (!hasKeyPath)
-            return objectStore->genAutoIncrementKey();
-
-        RefPtr<IDBKey> keyPathKey = fetchKeyFromKeyPath(value.get(), objectStore->m_keyPath);
-        if (keyPathKey) {
-            objectStore->resetAutoIncrementKeyCache();
-            return keyPathKey;
-        }
+    RefPtr<SerializedScriptValue> value = prpValue;
+    RefPtr<IDBKey> key = prpKey;
 
-        RefPtr<IDBKey> autoIncKey = objectStore->genAutoIncrementKey();
-        RefPtr<SerializedScriptValue> valueAfterInjection = injectKeyIntoKeyPath(autoIncKey, value, objectStore->m_keyPath);
-        if (!valueAfterInjection) {
-            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::DATA_ERR, "The generated key could not be inserted into the object using the keyPath."));
-            return 0;
+    if (putMode != CursorUpdate) {
+        const bool autoIncrement = objectStore->autoIncrement();
+        const bool hasKeyPath = !objectStore->m_keyPath.isNull();
+        if (hasKeyPath) {
+            ASSERT(!key);
+            RefPtr<IDBKey> keyPathKey = fetchKeyFromKeyPath(value.get(), objectStore->m_keyPath);
+            if (keyPathKey)
+                key = keyPathKey;
         }
-        value = valueAfterInjection;
-        return autoIncKey.release();
-    }
-
-    if (hasKeyPath) {
-        RefPtr<IDBKey> keyPathKey = fetchKeyFromKeyPath(value.get(), objectStore->m_keyPath);
-
-        // FIXME: This check should be moved to put() and raise an exception. WK76952
-        if (putMode == CursorUpdate && !keyPathKey->isEqual(key)) {
-            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::DATA_ERR, "The key fetched from the keyPath does not match the key of the cursor."));
-            return 0;
+        if (autoIncrement) {
+            if (!key) {
+                RefPtr<IDBKey> autoIncKey = objectStore->genAutoIncrementKey();
+                if (hasKeyPath) {
+                    // FIXME: Add checks in put() to ensure this will always succeed (apart from I/O errors).
+                    // https://bugs.webkit.org/show_bug.cgi?id=77374
+                    RefPtr<SerializedScriptValue> valueAfterInjection = injectKeyIntoKeyPath(autoIncKey, value, objectStore->m_keyPath);
+                    if (!valueAfterInjection) {
+                        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::DATA_ERR, "The generated key could not be inserted into the object using the keyPath."));
+                        return;
+                    }
+                    value = valueAfterInjection;
+                }
+                key = autoIncKey;
+            } else {
+                // FIXME: Logic to update generator state should go here. Currently it does a scan.
+                objectStore->resetAutoIncrementKeyCache();
+            }
         }
-
-        return keyPathKey.release();
-    }
-
-    if (!key) {
-        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::DATA_ERR, "No key supplied"));
-        return 0;
     }
 
-    return key;
-}
+    ASSERT(key && key->valid());
 
-void IDBObjectStoreBackendImpl::putInternal(ScriptExecutionContext*, PassRefPtr<IDBObjectStoreBackendImpl> objectStore, PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBKey> prpKey, PutMode putMode, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<IDBTransactionBackendInterface> transaction)
-{
-    RefPtr<SerializedScriptValue> value = prpValue;
-    RefPtr<IDBKey> key = selectKeyForPut(objectStore.get(), prpKey.get(), putMode, callbacks.get(), value);
-    if (!key)
+    RefPtr<IDBBackingStore::ObjectStoreRecordIdentifier> recordIdentifier = objectStore->m_backingStore->createInvalidRecordIdentifier();
+    if (putMode == AddOnly && objectStore->m_backingStore->keyExistsInObjectStore(objectStore->m_databaseId, objectStore->id(), *key, recordIdentifier.get())) {
+        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::CONSTRAINT_ERR, "Key already exists in the object store."));
         return;
-    ASSERT(key->valid());
+    }
 
     Vector<RefPtr<IDBKey> > indexKeys;
     for (IndexMap::iterator it = objectStore->m_indexes.begin(); it != objectStore->m_indexes.end(); ++it) {
@@ -263,14 +253,6 @@ void IDBObjectStoreBackendImpl::putInternal(ScriptExecutionContext*, PassRefPtr<
         indexKeys.append(indexKey.release());
     }
 
-    RefPtr<IDBBackingStore::ObjectStoreRecordIdentifier> recordIdentifier = objectStore->m_backingStore->createInvalidRecordIdentifier();
-    bool isExistingValue = objectStore->m_backingStore->keyExistsInObjectStore(objectStore->m_databaseId, objectStore->id(), *key, recordIdentifier.get());
-
-    if (putMode == AddOnly && isExistingValue) {
-        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::CONSTRAINT_ERR, "Key already exists in the object store."));
-        return;
-    }
-
     // Before this point, don't do any mutation.  After this point, rollback the transaction in case of error.
 
     if (!objectStore->m_backingStore->putObjectStoreRecord(objectStore->m_databaseId, objectStore->id(), *key, value->toWireString(), recordIdentifier.get())) {

diff --git a/Source/WebCore/storage/IDBObjectStoreBackendImpl.h b/Source/WebCore/storage/IDBObjectStoreBackendImpl.h
index 2ab5627..7c12d84 100644 (file)
--- a/Source/WebCore/storage/IDBObjectStoreBackendImpl.h
+++ b/Source/WebCore/storage/IDBObjectStoreBackendImpl.h
@@ -87,7 +87,6 @@ private:
     void loadIndexes();
     PassRefPtr<IDBKey> genAutoIncrementKey();
     void resetAutoIncrementKeyCache() { m_autoIncrementNumber = -1; }
-    static PassRefPtr<IDBKey> selectKeyForPut(IDBObjectStoreBackendImpl*, IDBKey*, PutMode, IDBCallbacks*, RefPtr<SerializedScriptValue>&);
 
     static void getInternal(ScriptExecutionContext*, PassRefPtr<IDBObjectStoreBackendImpl>, PassRefPtr<IDBKey> key, PassRefPtr<IDBCallbacks>);
     static void putInternal(ScriptExecutionContext*, PassRefPtr<IDBObjectStoreBackendImpl>, PassRefPtr<SerializedScriptValue>, PassRefPtr<IDBKey>, PutMode, PassRefPtr<IDBCallbacks>, PassRefPtr<IDBTransactionBackendInterface>);