472963 - TableExists does not work for temporary tables

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Marco Bonardo [:mak]]

if you create a temporary table it's stored in sqlite_temp_master, TableExists only queries sqlite_master, so it will return false even if the table exists.

Comment 1

[Marco Bonardo [:mak]]

and i suppose the issue is valid also for IndexExists

Comment 2

[Hashem Masoud]

Are you sure this was omitted by mistake? maybe it was intentionally designed like that?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Hashem Masoud from comment #2)
> Are you sure this was omitted by mistake? maybe it was intentionally
> designed like that?

Yes, the main use-case for this API is to verify if a table exist mostly before creating it. It is mostly used in migration. In such a case you need to know if that name is already in use generally.
In your case you seem to point out the need to distinguish temp from real resources, and for such a thing I suggest filing a separate bug to add an API like isTempResource() to better handle that.

Comment 4

[Tiziana Sellitto [:tiziana]]

Marco, do you think this is still a valid issue? I'm going through old "good first bug" to verify if they are still valid or something is changed in the meanwhile.

Comment 5

[Marco Bonardo [:mak]]

yes, still valid.

Comment 6

[Anup Kumar [:reznord]]

Hi, 

I am willing to work on this bug. So, please assign me this bug.

Thanks in Advance,

Regards,
A. Anup

Comment 7

[Marco Bonardo [:mak]]

hi, done. If you need help feel free to ask.

Comment 8

[Mark Capella [:capella]]

Attachment: bug472963

I think this is something close to what you're looking for.

Local tests worked properly, and so I went for a quick TRY push: https://tbpl.mozilla.org/?tree=Try&rev=64b0a033e7f5

Let me know? -- mark

Comment 9

[Mark Capella [:capella]]

I think I'll nag ping the review request :-)

(I realize this bug has been on file for a couple years so it's obviously not high priority)

Comment 10

[Marco Bonardo [:mak]]

You are right, I'm on it.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 798251 [details] [diff] [review]
bug472963

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

good catch on fixing Sqlite.jsm
just some minor things to cleanup

::: storage/src/mozStorageConnection.cpp
@@ +705,5 @@
>  {
>    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>  
> +  nsAutoCString query("SELECT name FROM (SELECT * FROM sqlite_master UNION ALL ");
> +  query.Append("SELECT * FROM sqlite_temp_master) WHERE type = '");

you don't need to append, you should be able to just:
nsCString query("SELECT name FROM (SELECT * FROM sqlite_master UNION ALL "
                                  "SELECT * FROM sqlite_temp_master) "
                "WHERE type ='");

Notice I didn't use an autostring cause the string is bigger than the autostring stack buffer, so there's no win in using an autostring in this case.

::: storage/test/unit/test_storage_connection.js
@@ +111,5 @@
> +add_task(function test_temp_tableExists_created()
> +{
> +  var msc = getOpenedDatabase();
> +  msc.executeSimpleSQL("CREATE TEMP TABLE test_temp(id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)");
> +  do_check_true(msc.tableExists("test_temp"));

it's good habit to clean up whatever you do in a test to avoid confusing next tests, so please after the test DROP TABLE

@@ +118,5 @@
> +add_task(function test_temp_indexExists_created()
> +{
> +  var msc = getOpenedDatabase();
> +  msc.executeSimpleSQL("CREATE INDEX test_temp_ind ON test_temp (name)");
> +  do_check_true(msc.indexExists("test_temp_ind"));

ditto for dropping the index

::: toolkit/modules/Sqlite.jsm
@@ +608,5 @@
>     * @return Promise<bool>
>     */
>    tableExists: function (name) {
>      return this.execute(
> +      "SELECT name FROM (SELECT * FROM sqlite_master UNION ALL SELECT * FROM sqlite_temp_master) WHERE type = 'table' AND name=?",

please split the string across multiple lines

@@ +626,5 @@
>     * @return Promise<bool>
>     */
>    indexExists: function (name) {
>      return this.execute(
> +      "SELECT name FROM (SELECT * FROM sqlite_master UNION ALL SELECT * FROM sqlite_temp_master) WHERE type = 'index' AND name=?",

ditto

Comment 12

[Mark Capella [:capella]]

Attachment: bug472963 (v2)

re: "autostring stack buffer" ... TIL  ;-)
   https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#Local_variables

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 808237 [details] [diff] [review]
bug472963 (v2)

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

it looks good to me, thank you!

If you need someone to push the patch for you, after you attach the next version, you can set the checkin-needed keyword, in the keywords field up here in the bug.

::: toolkit/modules/Sqlite.jsm
@@ +599,5 @@
>      return deferred.promise;
>    },
>  
>    /**
> +   * Whether a table exists in the database (both master and temporary tables).

s/master/persistent/

Comment 14

[Mark Capella [:capella]]

Nit fixed up ...
https://tbpl.mozilla.org/?tree=Try&rev=1a5afda2e7e4

Comment 15

[Mark Capella [:capella]]

Off we go!
https://hg.mozilla.org/integration/fx-team/rev/46583beb7d1b

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46583beb7d1b