481178 - mozIStorageConnection createTable and tableExists should support attached databases

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Andreas Wagner [:TheOne] [use NI]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6

After attaching the database "mydb", something like

connection.createTable("mydb.mytable", "id INTEGER PRIMARY KEY, foo TEXT, bar TEXT");

(and similar for tableExists())
should work.

Reproducible: Always

Actual Results:  
NS_ERROR_FAILURE

Comment 1

[Shawn Wilsher :sdwilsh]

Does it not work if you specify mydb.mytable?  It's not clear in comment 0 how exactly you are calling tableExists.

Comment 2

[Andreas Wagner [:TheOne] [use NI]]

Calling connection.tableExists("mydb.mytable") returns always false.

Comment 3

[Shawn Wilsher :sdwilsh]

Makes sense.  We need to take off the first dot found and everything before it, and put it before sqlite_master.

Comment 4

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 5

[Mike Hoye [:mhoye]]

Andreas, would you be willing to mentor this bug?

Comment 6

[Marco Bonardo [:mak]]

I may also mentor this, in case.

Comment 7

[Marco Bonardo [:mak]]

attaching a database is done through SQL, using the ATTACH database query (https://www.sqlite.org/lang_attach.html)
Basically any API getting TableName as input is affected by the bug cause to act on an attached database you must name the tables as
DatabaseName.TableName, but we are instead considering all of that as a table name. Instead we should split aTableName on ". " and consider the first part  a database name and the second part a table name.
CreateTable should then create the table in the appropriate database, and TableExists should search in the appropriate sqlite_master (depending on the database name).

Comment 8

[Marco Bonardo [:mak]]

this will need an automated test (likely an xpcshell test)

Comment 9

[Nochum Sossonko [:Natch]]

(In reply to Marco Bonardo [:mak] from comment #7)
Ok, I thought there was some attach API.

I'll take this bug for now, if no one objects.

Comment 10

[Nochum Sossonko [:Natch]]

Attachment: patch v1

The only function I found that was affected by this bug is databaseElementExists.

Tests included.

Thanks.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8404338 [details] [diff] [review]
patch v1

Review of attachment 8404338 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take a second look at this. Thanks.

::: storage/src/mozStorageConnection.cpp
@@ +704,5 @@
>  {
>    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>  
> +  nsCString query("SELECT name FROM (SELECT * FROM ");
> +  int32_t ind = aElementName.FindChar('.');

please add a comment above explaining why we are doing this, for future reference

@@ +705,5 @@
>    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>  
> +  nsCString query("SELECT name FROM (SELECT * FROM ");
> +  int32_t ind = aElementName.FindChar('.');
> +  nsCString table;

may be an nsDependentCSubstring, there's no risk the underlying string is going away here, so we can avoid some allocation

@@ +707,5 @@
> +  nsCString query("SELECT name FROM (SELECT * FROM ");
> +  int32_t ind = aElementName.FindChar('.');
> +  nsCString table;
> +
> +  if (kNotFound != ind) {

I don't like much double negations, they tend to make conditions hard to read.
I'd just do

if (aElementName.FindChar('.') == kNotFound) {
  ...
}
else {
  ...
}

@@ +708,5 @@
> +  int32_t ind = aElementName.FindChar('.');
> +  nsCString table;
> +
> +  if (kNotFound != ind) {
> +    nsCString db(Substring(aElementName, 0, ind + 1));

nsDependentCSubstring

@@ +715,5 @@
> +    query.Append("sqlite_master UNION ALL SELECT * FROM sqlite_temp_master) WHERE type = '");
> +  }
> +  else {
> +    table.Assign(aElementName);
> +    query.Append("sqlite_master UNION ALL SELECT * FROM sqlite_temp_master) WHERE type = '");

the last Append should happen outside the if/else, it's identical in both branches

@@ +728,4 @@
>        break;
>    }
>    query.Append("' AND name ='");
> +  query.Append(table.get());

do you really have to use get() here?

::: storage/src/mozStorageConnection.h
@@ +234,5 @@
>    // if there is one. Do nothing in other cases.
>    int progressHandler();
>  
> +  nsresult getDBAndTable(const nsCString& aElementName,
> +                         nsTArray<nsCString> *_rv);

looks like a leftover from a previous patch using an helper method...

::: storage/test/unit/test_storage_connection.js
@@ +158,5 @@
> +    msc.executeSimpleSQL("ATTACH DATABASE '" + file.path + "' AS sample");
> +  }
> +  catch (e) {
> +    do_throw("Getting an error on attach: " + msc.lastErrorString);
> +  }

do_throw works differently, basically you do: 
try {
  something
  do_throw("Should have thrown!");
} catch (ex) {
  // Expected.
}

in this case you expect your method to NOT throw, so you should just invoke it, if it throws the test will fail by itself.

@@ +163,5 @@
> +
> +  try {
> +    do_check_false(msc.tableExists("sample.test"));
> +  } catch (e) {
> +    do_throw("tableExists should not throw on attached dbs");

ditto

@@ +169,5 @@
> +  msc.createTable("sample.test", "id INTEGER PRIMARY KEY, name TEXT");
> +  do_check_true(msc.tableExists("sample.test"));
> +  try {
> +    msc.createTable("sample.test", "id INTEGER PRIMARY KEY, name TEXT");
> +    do_throw("We shouldn't get here!");

yes, this is the correct way to use it :)

@@ +171,5 @@
> +  try {
> +    msc.createTable("sample.test", "id INTEGER PRIMARY KEY, name TEXT");
> +    do_throw("We shouldn't get here!");
> +  } catch (e) {
> +    do_check_eq(Cr.NS_ERROR_FAILURE, e.result);

just put a comment stating why the exception is expected and rather restrict the catch to the specific exception we expect: catch (e if e.result == Components.results.NS_ERROR_FAILURE)

@@ +172,5 @@
> +    msc.createTable("sample.test", "id INTEGER PRIMARY KEY, name TEXT");
> +    do_throw("We shouldn't get here!");
> +  } catch (e) {
> +    do_check_eq(Cr.NS_ERROR_FAILURE, e.result);
> +  }

could you please also check indexExists works (I know it goes through the same code, but that would improve the test coverage).?

@@ +176,5 @@
> +  }
> +
> +  msc.executeSimpleSQL("DETACH DATABASE sample");
> +  db.close();
> +  if (file.exists()) {

may it not exist? if so I'd expect the test to fail on ATTACH...

Comment 12

[Marco Bonardo [:mak]]

ehr regarding

if (aElementName.FindChar('.') == kNotFound) {

you clearly need the index, so you will still have to assign to a var.
though please still do

if (ind == kNotFound)

to avoid the double negation.

Comment 13

[Nochum Sossonko [:Natch]]

Attachment: patch v2

(In reply to Marco Bonardo [:mak] from comment #11)
> the last Append should happen outside the if/else, it's identical in both
> branches

Yeah, this came from me not understanding that sqlite_temp_master doesn't get prefixed by the db name. I couldn't find documentation on that...

> > +  nsresult getDBAndTable(const nsCString& aElementName,
> > +                         nsTArray<nsCString> *_rv);
> 
> looks like a leftover from a previous patch using an helper method...

Yup, removed.

> in this case you expect your method to NOT throw, so you should just invoke
> it, if it throws the test will fail by itself.

This was debugging code that made me realize why tableExists was failing (sqlite_temp_master doesn't want a prefix).

> could you please also check indexExists works (I know it goes through the
> same code, but that would improve the test coverage).?

I had to separate out the tests as it was timing out with all the activity, so I generalized the secondary db handling into head.js and created two tests. What do you think?

All other comments addressed.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8404741 [details] [diff] [review]
patch v2

Review of attachment 8404741 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/src/mozStorageConnection.cpp
@@ +704,5 @@
>  {
>    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>  
> +  // When constructing the query, make sure to SELECT the correct db's sqlite_temp_master
> +  // if the user is prefixing the element with a specific db. ex: sample.test 

trailing space

@@ +707,5 @@
> +  // When constructing the query, make sure to SELECT the correct db's sqlite_temp_master
> +  // if the user is prefixing the element with a specific db. ex: sample.test 
> +  nsCString query("SELECT name FROM (SELECT * FROM ");
> +  int32_t ind = aElementName.FindChar('.');
> +  nsDependentCSubstring table;

hm, this is not just a table, may be an index. I think calling this "element" may be fine, cause the method is called databaseElementExists...

::: storage/test/unit/head_storage.js
@@ +51,4 @@
>    // close the connection
>    print("*** Storage Tests: Trying to close!");
>    getOpenedDatabase().close();
> +  getSecondaryDatabase().close();

the problem of this, is that while getOpenedDatabse is basically used by most of the tests, this secondary database is only used by your new tests, but this code causes it to be opened and closed for every test. That's not nice cause we should be trying to reduce work done by tests, to reduce time they take... I think I preferred the previous version of the boilerplate

Comment 15

[Nochum Sossonko [:Natch]]

(In reply to Marco Bonardo [:mak] from comment #14)
> the problem of this, is that while getOpenedDatabse is basically used by
> most of the tests, this secondary database is only used by your new tests,
> but this code causes it to be opened and closed for every test. That's not
> nice cause we should be trying to reduce work done by tests, to reduce time
> they take... I think I preferred the previous version of the boilerplate

Would it be ok if I check if gSecondaryDBConn != null and only then go through the cleaning codepaths? Or do you just want me to remove completely from head.js? I can also just add these functions to test_storage_connection.js at the top and run the cleanup at the end of this file. That would duplicate the cleanup code a bit though, is that an issue?

Let me know which you prefer and I'll just update the test(s) accordingly.

Thanks.

Comment 16

[Marco Bonardo [:mak]]

I think the previous version of the patch was fine there, even if there's some duplicated boilerplate that may be fine.

Comment 17

[Nochum Sossonko [:Natch]]

Attachment: patch v3

Ok, changes made.

s/table/element

I also condensed both tests into a single function, as it turns out it does NOT time out. I must've mistook a previous error failure as a timeout.

Thanks

Comment 18

[Nochum Sossonko [:Natch]]

Attachment: patch v3

Err, umm...forgot about the comment whitespace...fixed.

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8405403 [details] [diff] [review]
patch v3

Review of attachment 8405403 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/src/mozStorageConnection.cpp
@@ +707,5 @@
> +  // When constructing the query, make sure to SELECT the correct db's sqlite_master
> +  // if the user is prefixing the element with a specific db. ex: sample.test
> +  nsCString query("SELECT name FROM (SELECT * FROM ");
> +  int32_t ind = aElementName.FindChar('.');
> +  nsDependentCSubstring element;

please invert these 2 lines (so ind is defined just before first use)

@@ +709,5 @@
> +  nsCString query("SELECT name FROM (SELECT * FROM ");
> +  int32_t ind = aElementName.FindChar('.');
> +  nsDependentCSubstring element;
> +
> +  if (ind == kNotFound) {

the newline above the if is not very useful for readability, please remove it

Comment 20

[Nochum Sossonko [:Natch]]

Attachment: for checkin

Thank you mak!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b29ed4f8b260

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b29ed4f8b260