Bug 44164 – Implement persistance for IndexedDB ObjectStores

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Implement persistance for IndexedDB ObjectStores

[Jeremy Orlow]

Created attachment 64703 [details]
Patch

[Jeremy Orlow]

Created attachment 64713 [details]
Patch

[Marcus Bulach]

nice stuff!
few comments below:

WebCore/storage/IDBDatabaseBackendImpl.cpp:101
 +      importObjectStores();
loadObjectStoreadMetadata() might be better.. "import" sounds like your transforming / dealing with actual data.

WebCore/platform/sql/SQLiteStatement.cpp:265
 +      if (!m_statement)
does the "{" rule apply per statement or per lines? should this be a block?

WebCore/storage/IDBDatabaseBackendImpl.cpp:137
 +      insert.bindInt(3, (int)autoIncrement);
are c-style casts allowed?

WebCore/storage/IDBObjectStoreBackendImpl.cpp:206
 +      insert.bindInt(3, (int)unique);
ditto

WebCore/storage/IDBObjectStoreBackendImpl.cpp:262
 +  void IDBObjectStoreBackendImpl::importIndexes()
loadIndexesMetadata() ?

WebCore/storage/IDBFactoryBackendImpl.cpp:85
 +          "CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)",
shouldn't index have an FK to ObjectStores(id) ?
also, is there "alternate key"? I suppose (name, objectStoreId) should be unique.

WebCore/storage/IDBObjectStoreBackendImpl.cpp:66
 +  static String whereClause(IDBKey* key)
passing IDBKeyType would be clearer (I thought this would be dealing with the actual value..)

WebCore/storage/IDBObjectStoreBackendImpl.cpp:101
 +      SQLiteStatement query(sqliteDatabase(), "SELECT value FROM ObjectStoreData WHERE" + whereClause(key.get()));
yeah, passing type would be clearer. also, adding the space after the WHERE and removing from the three places above would look simpler.

WebCore/storage/IDBObjectStoreBackendImpl.cpp:111
 +      callbacks->onSuccess(SerializedScriptValue::createFromWire(query.getColumnText(0)));
hmm, AFAICT this will break JSC, it doesn't provide "fromWire" :(

WebCore/storage/IDBObjectStoreBackendImpl.cpp:169
 +      putQuery.bindText(4, value->toWireString());
ditto

[Jeremy Orlow]

(In reply to comment #3)
> nice stuff!
> few comments below:
> 
> WebCore/storage/IDBDatabaseBackendImpl.cpp:101
>  +      importObjectStores();
> loadObjectStoreadMetadata() might be better.. "import" sounds like your transforming / dealing with actual data.

done 
 
> WebCore/platform/sql/SQLiteStatement.cpp:265
>  +      if (!m_statement)
> does the "{" rule apply per statement or per lines? should this be a block?

I'd agree normally, but the rest of the file does it this way.
 
> WebCore/storage/IDBDatabaseBackendImpl.cpp:137
>  +      insert.bindInt(3, (int)autoIncrement);
> are c-style casts allowed?

fixed

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:206
>  +      insert.bindInt(3, (int)unique);
> ditto

fixed

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:262
>  +  void IDBObjectStoreBackendImpl::importIndexes()
> loadIndexesMetadata() ?

fixed

> WebCore/storage/IDBFactoryBackendImpl.cpp:85
>  +          "CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)",
> shouldn't index have an FK to ObjectStores(id) ?
> also, is there "alternate key"? I suppose (name, objectStoreId) should be unique.

did foreign key...couldn't do the alternate key as far as I could tell (without making it be my primary key)

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:66
>  +  static String whereClause(IDBKey* key)
> passing IDBKeyType would be clearer (I thought this would be dealing with the actual value..)

done

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:101
>  +      SQLiteStatement query(sqliteDatabase(), "SELECT value FROM ObjectStoreData WHERE" + whereClause(key.get()));
> yeah, passing type would be clearer. also, adding the space after the WHERE and removing from the three places above would look simpler.

done

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:111
>  +      callbacks->onSuccess(SerializedScriptValue::createFromWire(query.getColumnText(0)));
> hmm, AFAICT this will break JSC, it doesn't provide "fromWire" :(

It will be a major change to implement that for JSC.  Given that no one from JSC land seems to care about IndexedDB at the moment, we're going to punt on it.

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:169
>  +      putQuery.bindText(4, value->toWireString());
> ditto

yup

[Jeremy Orlow]

Created attachment 64718 [details]
Patch

[Andrei Popescu]

> CREATE TABLE IF NOT EXISTS ObjectStores (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, doAutoIncrement INTEGER)"

You are doing a delete based on name. Please create an index on 'name'.


> CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)"

You may want to add a foreign key on ObjectStores and use cascade delete like so:

objectStoreID INTEGER REFERENCES ObjectStore(id) ON DELETE CASCADE

[Jeremy Orlow]

(In reply to comment #6)
> > CREATE TABLE IF NOT EXISTS ObjectStores (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, doAutoIncrement INTEGER)"
> 
> You are doing a delete based on name. Please create an index on 'name'.
> 
> 
> > CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)"
> 
> You may want to add a foreign key on ObjectStores and use cascade delete like so:
> 
> objectStoreID INTEGER REFERENCES ObjectStore(id) ON DELETE CASCADE

Added foreign keys, indexes, and fixed some style issue.  Please recheck all SQL stuff in this.

[Jeremy Orlow]

Created attachment 64727 [details]
Patch

[Andrei Popescu]

Umm, what happened to IndexData table? You had that in the previous patch.

[Jeremy Orlow]

(In reply to comment #9)
> Umm, what happened to IndexData table? You had that in the previous patch.

Deleted since it's not used yet.  Will re add in next patch.

[Marcus Bulach]

(In reply to comment #10)
> (In reply to comment #9)
> > Umm, what happened to IndexData table? You had that in the previous patch.
> 
> Deleted since it's not used yet.  Will re add in next patch.

looks fine by me, ignorable comment:

WebCore/storage/IDBFactoryBackendImpl.cpp:93
 +          "DROP TABLE IF EXISTS ObjectStoreData",
perhads drop "Data" ? it doesn't seem to add much.

[Steve Block]

Comment on attachment 64727 [details]
Patch

WebCore/storage/IDBFactoryBackendImpl.cpp:81
 +          "CREATE TABLE IF NOT EXISTS MetaData (id INTEGER PRIMARY KEY, name TEXT, description TEXT, version TEXT)",
What's this id used for?

WebCore/storage/IDBObjectStoreBackendImpl.cpp:55
 +      loadIndexes();
Can we do this lazily? It looks like m_indexes isn't needed until script requests it?

WebCore/storage/IDBObjectStoreBackendImpl.cpp:75
 +          return "keyString IS NULL  AND  keyDate IS NULL  AND  keyNumber IS NULL  AND  objectStoreId = ?";
Do we need the NULL tests for the 'other' columns (except maybe the 'NULL' type)? These should be NULL unless we have a logic error, right? If we want to test for logic errors, we could do that in an ASSERT/debug statement?

WebCore/storage/IDBObjectStoreBackendImpl.cpp:66
 +  static String whereClause(IDBKey::Type type)
When I first read this method, I assumed the returned clause would include 'WHERE'. I think that makes more sense.

WebCore/storage/IDBObjectStoreBackendImpl.cpp:106
 +      query.bindInt64(2, m_id);
Could you use constants for these column indexes? Especially as they're implementation details of whereClause().

WebCore/storage/IDBObjectStoreBackendImpl.cpp:207
 +      SQLiteStatement insert(sqliteDatabase(), "INSERT INTO Indexes (name, keyPath, isUnique) VALUES (?, ?, ?)");
This is missing the foreign key value

[Jeremy Orlow]

Comment on attachment 64727 [details]
Patch

(In reply to comment #12)
> (From update of attachment 64727 [details])
> WebCore/storage/IDBFactoryBackendImpl.cpp:81
>  +          "CREATE TABLE IF NOT EXISTS MetaData (id INTEGER PRIMARY KEY, name TEXT, description TEXT, version TEXT)",
> What's this id used for?

Nothing.  But if you don't have one, SQLite will create one anyway and just leave it unnamed.  So there's no reason not to.

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:55
>  +      loadIndexes();
> Can we do this lazily? It looks like m_indexes isn't needed until script requests it?

No, because meta-data is accessed synchronously.
 
> WebCore/storage/IDBObjectStoreBackendImpl.cpp:75
>  +          return "keyString IS NULL  AND  keyDate IS NULL  AND  keyNumber IS NULL  AND  objectStoreId = ?";
> Do we need the NULL tests for the 'other' columns (except maybe the 'NULL' type)? These should be NULL unless we have a logic error, right? If we want to test for logic errors, we could do that in an ASSERT/debug statement?

Yeah, I guess that makes sense.
 
> WebCore/storage/IDBObjectStoreBackendImpl.cpp:66
>  +  static String whereClause(IDBKey::Type type)
> When I first read this method, I assumed the returned clause would include 'WHERE'. I think that makes more sense.

k

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:106
>  +      query.bindInt64(2, m_id);
> Could you use constants for these column indexes? Especially as they're implementation details of whereClause().

I don't see a point to using constants.  They're tied to the SELECT statements directly above.  Putting a constant at the beginning of the file makes no sense.  Putting it next to the select seems only marginally better.

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:207
>  +      SQLiteStatement insert(sqliteDatabase(), "INSERT INTO Indexes (name, keyPath, isUnique) VALUES (?, ?, ?)");
> This is missing the foreign key value

Indeed.


Will post another version soon.

[Steve Block]

> > WebCore/storage/IDBObjectStoreBackendImpl.cpp:106
> >  +      query.bindInt64(2, m_id);
> > Could you use constants for these column indexes? Especially as they're implementation details of whereClause().
> 
> I don't see a point to using constants.  They're tied to the SELECT statements
> directly above.  Putting a constant at the beginning of the file makes no
> sense.  Putting it next to the select seems only marginally better.
My point is that they's tied to the implementation of whereClause(), so having a numbers like this elsewhere in the code seems brittle. Maybe put the constants next to whereClause()?

[Jeremy Orlow]

(In reply to comment #14)
> > > WebCore/storage/IDBObjectStoreBackendImpl.cpp:106
> > >  +      query.bindInt64(2, m_id);
> > > Could you use constants for these column indexes? Especially as they're implementation details of whereClause().
> > 
> > I don't see a point to using constants.  They're tied to the SELECT statements
> > directly above.  Putting a constant at the beginning of the file makes no
> > sense.  Putting it next to the select seems only marginally better.
> My point is that they's tied to the implementation of whereClause(), so having a numbers like this elsewhere in the code seems brittle. Maybe put the constants next to whereClause()?

Actually I see another problem with it as well.  Will try to make it better.

[Jeremy Orlow]

Created attachment 64831 [details]
Patch

[Steve Block]

Comment on attachment 64831 [details]
Patch

WebCore/storage/IDBObjectStoreBackendImpl.cpp:83
 +  static int bindKey(SQLiteStatement& query, int column, IDBKey* key)
I'm not sure it makes much sense for this to be a separate method

WebCore/storage/IDBObjectStoreBackendImpl.cpp:101
 +  static void bindWhereClause(SQLiteStatement& query, int64_t id, IDBKey* key)
Nice. Is bindWhereClauseStatement() more clear?

[Jeremy Orlow]

(In reply to comment #17)
> (From update of attachment 64831 [details])
> WebCore/storage/IDBObjectStoreBackendImpl.cpp:83
>  +  static int bindKey(SQLiteStatement& query, int column, IDBKey* key)
> I'm not sure it makes much sense for this to be a separate method

In my next patch, I use it.  :-)
 
> WebCore/storage/IDBObjectStoreBackendImpl.cpp:101
>  +  static void bindWhereClause(SQLiteStatement& query, int64_t id, IDBKey* key)
> Nice. Is bindWhereClauseStatement() more clear?

Personally, I don't think so.

[Jeremy Orlow]

Committed r65667: <http://trac.webkit.org/changeset/65667>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65667 might have broken Chromium Mac Release