Closed Bug 481178 Opened 11 years ago Closed 6 years ago

mozIStorageConnection createTable and tableExists should support attached databases

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: TheOne, Assigned: Natch)

Details

(Whiteboard: [good first bug][mentor=mak][lang=cpp])

Attachments

(1 file, 4 obsolete files)

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
Does it not work if you specify mydb.mytable?  It's not clear in comment 0 how exactly you are calling tableExists.
Calling connection.tableExists("mydb.mytable") returns always false.
Makes sense.  We need to take off the first dot found and everything before it, and put it before sqlite_master.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug]
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Component: Bookmarks & History → Storage
Product: Firefox → Toolkit
Andreas, would you be willing to mentor this bug?
Flags: needinfo?(mail)
I may also mentor this, in case.
OS: Windows Vista → All
Hardware: x86 → All
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).
this will need an automated test (likely an xpcshell test)
Flags: in-testsuite?
(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.
Assignee: nobody → highmind63
Status: NEW → ASSIGNED
Flags: needinfo?(mail)
Whiteboard: [good first bug] → [good first bug][mentor=mak][lang=cpp]
Attached patch patch v1 (obsolete) — Splinter Review
The only function I found that was affected by this bug is databaseElementExists.

Tests included.

Thanks.
Attachment #8404338 - Flags: review?(mak77)
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...
Attachment #8404338 - Flags: review?(mak77) → feedback+
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.
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #8404338 - Attachment is obsolete: true
Attachment #8404741 - Flags: review?(mak77)
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
Attachment #8404741 - Flags: review?(mak77) → feedback+
(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.
I think the previous version of the patch was fine there, even if there's some duplicated boilerplate that may be fine.
Attached patch patch v3 (obsolete) — Splinter Review
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
Attachment #8404741 - Attachment is obsolete: true
Attachment #8405401 - Flags: review?(mak77)
Attached patch patch v3 (obsolete) — Splinter Review
Err, umm...forgot about the comment whitespace...fixed.
Attachment #8405401 - Attachment is obsolete: true
Attachment #8405401 - Flags: review?(mak77)
Attachment #8405403 - Flags: review?(mak77)
Attachment #8405403 - Attachment is patch: true
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
Attachment #8405403 - Flags: review?(mak77) → review+
Attached patch for checkinSplinter Review
Thank you mak!
Attachment #8405403 - Attachment is obsolete: true
Attachment #8409008 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b29ed4f8b260
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.