Closed Bug 472963 Opened 12 years ago Closed 7 years ago

TableExists does not work for temporary tables

Categories

(Toolkit :: Storage, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: mak, Assigned: capella)

Details

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

Attachments

(1 file, 1 obsolete file)

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.
and i suppose the issue is valid also for IndexExists
Whiteboard: [good first bug]
Are you sure this was omitted by mistake? maybe it was intentionally designed like that?
(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.
Whiteboard: [good first bug] → [good first bug][mentor=mak]
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.
Flags: needinfo?(mak77)
yes, still valid.
Flags: needinfo?(mak77)
Whiteboard: [good first bug][mentor=mak] → [good-first-bug][mentor=mak][lang=cpp]
Hi, 

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

Thanks in Advance,

Regards,
A. Anup
hi, done. If you need help feel free to ask.
Assignee: nobody → allamsetty.anup
Assignee: allamsetty.anup → markcapella
Attached patch bug472963 (obsolete) — Splinter Review
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
Attachment #798251 - Flags: review?(mak77)
Status: NEW → ASSIGNED
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)
You are right, I'm on it.
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
Attachment #798251 - Flags: review?(mak77) → feedback+
Attached patch bug472963 (v2)Splinter Review
re: "autostring stack buffer" ... TIL  ;-)
   https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#Local_variables
Attachment #798251 - Attachment is obsolete: true
Attachment #808237 - Flags: review?(mak77)
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/
Attachment #808237 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/46583beb7d1b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.