Bug 50674 – IndexedDB: Numeric keys are floats.

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: Numeric keys are floats.

[Hans Wennborg]

Created attachment 75883 [details]
Patch

[Jeremy Orlow]

Comment on attachment 75883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75883&action=review

Looks great!

> LayoutTests/storage/indexeddb/objectstore-cursor.html:19
>      1,

Each one increases the size of the test exponentially.  Let's just get rid of 1 and 2 maybe?

Also add something to index-cursor.  Replace 2 of the identical integers with identical doubles maybe?

> WebCore/storage/IDBFactoryBackendImpl.cpp:127
> +    if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER WHERE type = 'table' AND tbl_name = 'ObjectStoreData' AND sql = 'CREATE TABLE ObjectStoreData (id INTEGER PRIMARY KEY, objectStoreId INTEGER NOT NULL REFERENCES ObjectStore(id), keyString TEXT, keyDate INTEGER, keyNumber INTEGER, value TEXT NOT NULL)'"))

I wonder if this is too fragile.  If some SQLite somewhere will not use this exact string or something like that.  Maybe it is worth looking for a version table and having the migration create one and add a version number of 1?

> WebCore/storage/IDBFactoryBackendImpl.cpp:134
> +        "DROP TABLE ObjectStoreData",

Add a comment that this depends on SQLite not enforcing the referential consistency.

> WebCore/storage/IDBFactoryBackendImpl.cpp:140
> +        if (!sqliteDatabase.executeCommand(commands[i])) {

This all should be done in a transaction probably.

> WebKit/chromium/public/WebIDBKey.h:-51
> -    WebIDBKey(int32_t number) { assign(number); }

So implicit type conversion is going to work without errors that break any of the builds?

[Andrei Popescu]

> if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER 

Ugh, this won't scale very well at all if we need another modification later. I'd definitely do what Jeremy suggested.

[Hans Wennborg]

(In reply to comment #2)
> (From update of attachment 75883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75883&action=review
> 
> Looks great!
> 
> > LayoutTests/storage/indexeddb/objectstore-cursor.html:19
> >      1,
> 
> Each one increases the size of the test exponentially.  Let's just get rid of 1 and 2 maybe?
Done.

> 
> Also add something to index-cursor.  Replace 2 of the identical integers with identical doubles maybe?
Done.

> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:127
> > +    if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER WHERE type = 'table' AND tbl_name = 'ObjectStoreData' AND sql = 'CREATE TABLE ObjectStoreData (id INTEGER PRIMARY KEY, objectStoreId INTEGER NOT NULL REFERENCES ObjectStore(id), keyString TEXT, keyDate INTEGER, keyNumber INTEGER, value TEXT NOT NULL)'"))
> 
> I wonder if this is too fragile.  If some SQLite somewhere will not use this exact string or something like that.  Maybe it is worth looking for a version table and having the migration create one and add a version number of 1?
Doing this.

I think this means we need to change the way createTables() work, though. Until now, we've been doing "CREATE TABLE IF NOT EXISTS foo ...", which looks like it is pretty fault tolerant, i.e. if one table or index is lost somehow (which doesn't really seem likely, it's more likely that it gets corrupted), it is re-created.

With versioning, we can't really re-create tables like this, because if the database is using the schema of version 1, then we can't just create missing tables using schema from version 2, because that could cause trouble with migration. I think we have to trust that the tables are there, and fail if not.


> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:134
> > +        "DROP TABLE ObjectStoreData",
> 
> Add a comment that this depends on SQLite not enforcing the referential consistency.
Done.

> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:140
> > +        if (!sqliteDatabase.executeCommand(commands[i])) {
> 
> This all should be done in a transaction probably.
Done.

> 
> > WebKit/chromium/public/WebIDBKey.h:-51
> > -    WebIDBKey(int32_t number) { assign(number); }
> 
> So implicit type conversion is going to work without errors that break any of the builds?
Yes.

Type conversion will keep the build from breaking. In multi-process mode this means non-integer keys won't work until the Chromium side is fixed, but running layout tests works fine.

Assuming I thought of everything, that is :)

[Hans Wennborg]

Created attachment 76053 [details]
Patch

[Jeremy Orlow]

Comment on attachment 76053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76053&action=review

> WebCore/storage/IDBFactoryBackendImpl.cpp:119
> +        "INSERT INTO MetaData VALUES ('version', 2)",

I think we should probably just keep this as it was originally and let the migrations bring it up to date so we dont' add antoher code path.

> WebCore/storage/IDBFactoryBackendImpl.cpp:153
> +        query.finalize();

Whats this?  Should we be doing it elsewhere?

[Hans Wennborg]

(In reply to comment #6)
> (From update of attachment 76053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76053&action=review
> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:119
> > +        "INSERT INTO MetaData VALUES ('version', 2)",
> 
> I think we should probably just keep this as it was originally and let the migrations bring it up to date so we dont' add antoher code path.
Done.

> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:153
> > +        query.finalize();
> 
> Whats this?  Should we be doing it elsewhere?
Don't remember where I saw it, but now I realize it's unnecessary since it's done in the SQLiteStatment destructor.


I realized we should migrate the IndexData table too. Doing that and tweaking the index-basics test to exercise it.

[Hans Wennborg]

Created attachment 76067 [details]
Patch

[Jeremy Orlow]

Comment on attachment 76067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76067&action=review

> WebCore/storage/IDBFactoryBackendImpl.cpp:134
> +        "BEGIN TRANSACTION",

Use the SQLiteTransaction class instead?

> WebCore/storage/IDBFactoryBackendImpl.cpp:170
> +            "BEGIN TRANSACTION",

Use the SQLiteTransaction class instead?

[Hans Wennborg]

(In reply to comment #9)
> (From update of attachment 76067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76067&action=review
> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:134
> > +        "BEGIN TRANSACTION",
> 
> Use the SQLiteTransaction class instead?
Okay.

[Hans Wennborg]

Created attachment 76072 [details]
Patch

[Jeremy Orlow]

Comment on attachment 76072 [details]
Patch

r=me

[Hans Wennborg]

Committed r73616: <http://trac.webkit.org/changeset/73616>

[WebKit Review Bot]

http://trac.webkit.org/changeset/73616 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
inspector/styles-source-offsets.html

[Hans Wennborg]

This was rolled out in r73633 because it broke the windows build.

[Hans Wennborg]

Committed r73697: <http://trac.webkit.org/changeset/73697>