bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Add convenience method for getting the database schema version

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Toolkit
Storage
--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9alpha8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

6.17 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
Useful for all of our consumers.
(Assignee)

Comment 1

11 years ago
Created attachment 270360 [details] [diff] [review]
v1.0
Attachment #270360 - Flags: review?(sspitzer)
(Assignee)

Updated

11 years ago
Blocks: 386368
(Assignee)

Updated

11 years ago
Blocks: 386371
(Assignee)

Updated

11 years ago
Blocks: 386369
(Assignee)

Updated

11 years ago
Blocks: 385871
1)

from http://www.sqlite.org/pragma.html

"The pragmas schema_version and user_version are used to set or get the value of the schema-version and user-version, respectively. Both the schema-version and the user-version are 32-bit signed integers stored in the database header."

So, in the idl:

attribute unsigned long schemaVersion;

that should not be unsigned, right?

2)

+    *version = 0;
+    PRBool hasResult;
+    if (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult)
+        *version = stmt->AsInt64(0);

Should that be AsInt32(), not AsInt64()

3)

for your tests, should verify that you can set the version to -1, too?  perhaps also test setting the version if it is already set (go from 1 to 2).
(Assignee)

Comment 3

11 years ago
(In reply to comment #2)
> that should not be unsigned, right?
ah, yes.  Good catch.

> Should that be AsInt32(), not AsInt64()
Now, yes, but before we needed AsInt64 to get the full range of a PRUint32 (or almost)

> for your tests, should verify that you can set the version to -1, too?  perhaps
> also test setting the version if it is already set (go from 1 to 2).
I can do that
(Assignee)

Comment 4

11 years ago
Created attachment 270365 [details] [diff] [review]
v1.1
Attachment #270360 - Attachment is obsolete: true
Attachment #270365 - Flags: review?(sspitzer)
should we be returning an error when the schema is not set?

see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#523

and cool, you test both the negative schema and the modification of a set schema (going from 1 to -1) with just one additional test.
(Assignee)

Comment 6

11 years ago
(In reply to comment #5)
> should we be returning an error when the schema is not set?
Well, in the idl I say that we return 0 if it isn't set, which is what happens anyway in my experience using SQLite Database Browser (and makes sense based on their docs saying that is always stored with the database).

Consumers can always do an NS_ENSURE_TRUE on the result.
Comment on attachment 270365 [details] [diff] [review]
v1.1

r=sspitzer, thanks for answering my questions.  I'm getting 0 back when the schema is not set as well, but also, I like return 0 in either case (not set or 0.)
Attachment #270365 - Flags: review?(sspitzer) → review+
storage/public/mozIStorageConnection.idl 1.9
storage/src/mozStorageConnection.cpp 1.22
storage/test/unit/test_storage_connection.js 1.2
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Keywords: dev-doc-needed

Updated

11 years ago
Blocks: 399535
Removing doc needed keyword in favor of a new bug that covers the fact that in general we actually need proper documentation for Storage; that bug (bug 399535) is set as blocked by this one.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.