Closed Bug 386366 Opened 13 years ago Closed 13 years ago

Add convenience method for getting the database schema version

Categories

(Toolkit :: Storage, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 1 obsolete file)

Useful for all of our consumers.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #270360 - Flags: review?(sspitzer)
Blocks: 386368
Blocks: 386371
Blocks: 386369
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).
(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
Attached patch v1.1Splinter Review
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.
(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
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Keywords: dev-doc-needed
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.