TableExists does not work for temporary tables

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: mak, Assigned: capella)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

11 years ago
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.
Reporter

Comment 1

11 years ago
and i suppose the issue is valid also for IndexExists
Whiteboard: [good first bug]

Comment 2

10 years ago
Are you sure this was omitted by mistake? maybe it was intentionally designed like that?
Reporter

Comment 3

7 years ago
(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)
Reporter

Comment 5

6 years ago
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
Reporter

Comment 7

6 years ago
hi, done. If you need help feel free to ask.
Assignee: nobody → allamsetty.anup
Assignee: allamsetty.anup → markcapella
Assignee

Comment 8

6 years ago
Posted 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)
Assignee

Updated

6 years ago
Status: NEW → ASSIGNED
Assignee

Comment 9

6 years ago
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)
Reporter

Comment 10

6 years ago
You are right, I'm on it.
Reporter

Comment 11

6 years ago
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+
Assignee

Comment 12

6 years ago
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)
Reporter

Comment 13

6 years ago
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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.