Closed Bug 261861 Opened 18 years ago Closed 14 years ago

initial mozStorage interfaces

Categories

(Toolkit :: Storage, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: vlad, Unassigned)

Details

Attachments

(1 file)

Attached will be a tarball containing the initial mozStorage IDL interfaces. 
There are some issues that need to get resolved, based on a recent discussion --
specifically, mozIStorageConnection needs to get a CreateTable call, and needs
to lose the sqlite3Handle attribute.

Also, "mozIStorage" might want to get changed to "stI" or similar.

Other feedback welcome.
some comments...

note: I do not know sqlite. I consider this an advantage in this case :) I do
think these interfaces/their docs should make sense w/o knowing sqlite.

mozIStorageConnection:

maybe move the initialize method to the top

what would connectionReady be used for? Is it true iff initialize was called and
succeeded?

what's a row id and what can I do with one?

what's the error code on this interface? Do I need sqlite headers to make sense
of them?

is the error string meant to be human-readable? if so, is it localized?


Why do nsresult errors from the methods not suffice?

   * Initialize the connection by opening the specified database.

does the file need to exist?

   * Returns true if a transaction is active on this connection.
so if no txn is active, updates are immediately committed?

   * Begin a new transaction.  If a transaction is active,
   * @throws NS_ERROR_STORAGE_INVALID_OPERATION.

that will not make any sense in doxygen-generated documentation. try:
   * Begin a new transaction.
   * @throws NS_ERROR_STORAGE_INVALID_OPERATION if a transaction ...

  void createFunction (in string aFunctionName,

so the function name must be ASCII?

this is, apparently, not a stored procedure?
what are sqlite functions good for?

ah... for use with triggers, I see :)

  void createTrigger (in string aTriggerName,
                      in long aTriggerType,
                      in string aTableName,
                      in string aTriggerFunction,
                      in string aParameters);

all of those parameters are limited to ascii? especially, the table name, and
parameters are?

name is only used for use with removeTrigger?

   * Abort any current operation, forcing a return
   * with an aborted error message.

does this mean executeSimpleSql and the like is async? how would I get notified
for completion?

  [noscript] readonly attribute sqlite3ptr sqliteHandle;

I agree that this should go :) (along with the #include)


these interfaces could generally use a comment above them describing their purpose.

mozIStorageDataSchema seems a bit unused - not referenced by the other files?
on this iface:
  readonly attribute nsIArray columnNames;

maybe an nsIUTF8StringEnumerator would be more pleasant to use from JS... but if
not, can you document the type of the array elements? (presumably
nsISupportsString?)

mozIStorageDataSet:
   * All the rows in this data set, as a nsIArray

...with elements of type mozIStorageValueArray, right?

is an nsISupportsArray better than an nsIArray? (no idea how the two compare)

mozIStorageFunction:
wouldn't it make sense to pass the connection to it, so it can easily do stuff
with the database?

mozIStorageService:
leaving aside what the meaning of "profile" and "cert" are - do I need to
initialize() the resulting connection?
if this is the only way that I'm supposed to create connections (I hope it
isn't! I want to CreateInstance them), why an initialize method on the connection?

   * the appropriate storage for the current thread.
multiple threads have different storages?

mozIStorageStatement:
is there a need for initialize()? the connection has a createStatement function,
which should be enough...

  void bindCStringParameter (in unsigned long aParamIndex, in string aValue);

I am not sure that it is wise to ignore charsets here.

  void bindDataParameter (in unsigned long aParamIndex,
[array,size_is(aValueSize)] in octet aValue, in unsigned long aValueSize);

that array could be [const], I think?
it is for BLOBs, right?


each call to executeStep gets the next result row, right? what happens if there
are no more rows?
is there a need for this method, why does executeDataSet not suffice?

mozIStorageValueArray:
why is this in a file called mozIStorageValue?

  string getAsCString(in unsigned long aIndex);
_please_ reconsider adding this method. I have a fear that people will call it
where they shouldn't, causing i18n bugs.

  /* XXX handle blobs! */
other than the getAsBlob method above?

  [notxpcom] string asSharedCString(in unsigned long aIndex, out unsigned long
aLength);
which charset?
can these notxpcom methods ever fail?

SQLite3 appears to use UTF-8 strings without restriction to ASCII, so perhaps
anywhere where |string| appears, |AUTF8String| should appear instead.  That is
unless we intentionally wish to limit certain names to ASCII.


mozIStorageConnection::initialize is going away (or so i understood) in favor of
using mozIStorageService to access storage connections.


>   readonly attribute nsIArray columnNames;
> 
> maybe an nsIUTF8StringEnumerator would be more pleasant to use from JS... but 
> if not, can you document the type of the array elements? (presumably
> nsISupportsString?)

yeah, an enumerator interface is usually preferrable to a readonly array interface.


>    * Abort any current operation, forcing a return
>    * with an aborted error message.
>
> does this mean executeSimpleSql and the like is async? how would I get 
> notified for completion?

Abort is part of the mozIStorageStatement::executeCallback feature, and AFAIK it
is going away, so I presume that Abort will be going away too.  executeSimpleSql
completes synchronously.  all of the |execute| functions complete synchronously.
  if you need to do things asynchronously then you can use executeStep to access
one result row at a time, but vlad is to verify that SQLite3 supports
overlapping statements with pending result data.  (if not, then implementing
iterators on top of sqlite3 is going to suck.)


>   void bindCStringParameter (in unsigned long aParamIndex, in string aValue);
> I am not sure that it is wise to ignore charsets here.

If that method is implemented as a direct call to sqlite3, then aValue must be
UTF-8, but XPConnect of course restricts it to ASCII.  I don't think there's any
value in exposing this method to script.


biesi: nsISupportsArray is deprecated in favor of nsIArray.
(In reply to comment #2)
> some comments...
> 
> note: I do not know sqlite. I consider this an advantage in this case :) I do
> think these interfaces/their docs should make sense w/o knowing sqlite.

I agree; a lot of the stuff is fairly SQL-specific, and needs to be explained
better.

> mozIStorageConnection:
> 
> maybe move the initialize method to the top

Initialize is going away; mozIStorageService will take care of setting up the
connection to the right file, etc.

> what would connectionReady be used for? Is it true iff initialize was called and
> succeeded?

It's true if the connection is ready to execute a statement; if it's in the
middle of another execution (e.g. someone's calling executeStep somewhere), then
it's not ready.  However, as this is entirely single threaded, the connection
should never be not-ready, so this can probably go away.

> what's a row id and what can I do with one?

The database works in terms of rows in tables; each row in each table has a
unique oid ("object id")/row id.  You can retreive the oid of the most recently
created row using this, so that you can reference it later without having to
search for it.

> what's the error code on this interface? Do I need sqlite headers to make sense
> of them?

You do for now; I'll copy the values into the idl, but there's some oddness with
sqlite's error handling (some error states aren't getting saved as they should be).

> is the error string meant to be human-readable? if so, is it localized?

It's human readable, but it's not localized.  I've sent mail to the sqlite
mailing list about this and got no response; I'll probably do a patch to make it
localizable and send it out to them.  Internally it'll be trivial to make it
localizable, as the string formatting already goes through a table of strings
and the like.

> Why do nsresult errors from the methods not suffice?

Things like errmsg can provide extra information, e.g. "Syntax error after 'ON'"
or somesuch.

>    * Returns true if a transaction is active on this connection.
> so if no txn is active, updates are immediately committed?

Right; if there is no transaction active, each writing statement has an implicit
transaction wrapped around it.

>    * Begin a new transaction.  If a transaction is active,
>    * @throws NS_ERROR_STORAGE_INVALID_OPERATION.
> 
> that will not make any sense in doxygen-generated documentation. try:
>    * Begin a new transaction.
>    * @throws NS_ERROR_STORAGE_INVALID_OPERATION if a transaction ...

Ah; I need to do a bit of reading up on doxygen, I was sort of making things up
as I went along ;)

>   void createFunction (in string aFunctionName,
> 
> so the function name must be ASCII?

It doesn't have to be for sqlite, but I'd like to force it to be.

> this is, apparently, not a stored procedure?
> what are sqlite functions good for?

For defining custom functions that can execute as part of SQL statements; in
particular, triggers (notifications when a row is added/deleted/modified in a
table).

> ah... for use with triggers, I see :)

Er, right :)

>   void createTrigger (in string aTriggerName,
>                       in long aTriggerType,
>                       in string aTableName,
>                       in string aTriggerFunction,
>                       in string aParameters);
> 
> all of those parameters are limited to ascii? especially, the table name, and
> parameters are?

As with the function name, they're not in sqlite, but I'd like them to be.  I
see no benefit to allowing arbitrary utf8/utf16 for
function/trigger/table/column/etc. names.  Are there any reasons for doing so?

> name is only used for use with removeTrigger?

Yep; it also can't conflict with any currently existing trigger.

>    * Abort any current operation, forcing a return
>    * with an aborted error message.

> does this mean executeSimpleSql and the like is async? how would I get
notified for completion?

Nope, it's all sync.  The abort stuff is actually only useful with executeStep()
stuff -- but as everything is happening on a single thread, I'm not sure now
what the value is of exposing abort.

>   [noscript] readonly attribute sqlite3ptr sqliteHandle;
> 
> I agree that this should go :) (along with the #include)

Yes :)

> these interfaces could generally use a comment above them describing their
purpose.
> 
> mozIStorageDataSchema seems a bit unused - not referenced by the other files?
> on this iface:
>   readonly attribute nsIArray columnNames;

Whoops, yeah.. mozIStorageDataSchema was used for something that I removed;
however, it might make a comeback.  I think the right thing here would be to
just have "readonly attribute long numColumns;" and "AUTF8String columnName(in
long aColumnIndex)" to simplify things in both C++ and JS.

> mozIStorageDataSet:
>    * All the rows in this data set, as a nsIArray
> 
> ...with elements of type mozIStorageValueArray, right?

Yep.

> mozIStorageFunction:
> wouldn't it make sense to pass the connection to it, so it can easily do stuff
> with the database?

At the time it gets called, I have no access to the mozIStorageConnection
without creating my own closure.  I figure since someone will have to implement
a class to use mozIStorageFunction anyway, they can just pass in the connection
as part of the constructor.

> mozIStorageService:
> leaving aside what the meaning of "profile" and "cert" are - do I need to
> initialize() the resulting connection?
> if this is the only way that I'm supposed to create connections (I hope it
> isn't! I want to CreateInstance them), why an initialize method on the connection?

mozIStorageService needs to be fleshed out more; the "profile" "cert" etc. stuff
was a last-minute note in there.


>    * the appropriate storage for the current thread.
> multiple threads have different storages?
> 
> mozIStorageStatement:
> is there a need for initialize()? the connection has a createStatement function,
> which should be enough...

True.
 
>   void bindCStringParameter (in unsigned long aParamIndex, in string aValue);
> 
> I am not sure that it is wise to ignore charsets here.

This should probably be [noscript].

>   void bindDataParameter (in unsigned long aParamIndex,
> [array,size_is(aValueSize)] in octet aValue, in unsigned long aValueSize);
> 
> that array could be [const], I think?
> it is for BLOBs, right?

Yep, forgot to explicitly specify const.


> each call to executeStep gets the next result row, right? what happens if there
> are no more rows?
> is there a need for this method, why does executeDataSet not suffice?

null gets returned when there are no more rows, and eventually the nsresult will
be some non-NS_OK success value.

> mozIStorageValueArray:
> why is this in a file called mozIStorageValue?

Mm. Good question.  There was going to be a mozIStorageValue at some point, but
it turned into a ValueArray.

>   string getAsCString(in unsigned long aIndex);
> _please_ reconsider adding this method. I have a fear that people will call it
> where they shouldn't, causing i18n bugs.

Hmm. We can probably live without it; I put it in for completeness.

>   /* XXX handle blobs! */
> other than the getAsBlob method above?

Erm. comment bug :)

>   [notxpcom] string asSharedCString(in unsigned long aIndex, out unsigned long
> aLength);
> which charset?
> can these notxpcom methods ever fail?

charset will always be UTF8 here.  In theory they can fail if aIndex is out of
range, but they're way too convenient to not include them.

Thanks for the feedback!  I'll incorporate the suggestions in the next few days.
Status: NEW → ASSIGNED
Product: Browser → Toolkit
Version: Trunk → unspecified
Have you looked at http://lxr.mozilla.org/mozilla/source/extensions/sql ?
I don't say my component is better, but it is very similar to yours and already
supports PostgreSQL, MySQL and SQLite.

I didn't watch the tree for some time, but I'm very surprised that nobody else
mentioned this component. Was it on purpose?
Assignee: vladimir → nobody
Status: ASSIGNED → NEW
mozStorage has been in the tree for quite a while. Any objections to closing this bug?
I'm keeping it open because there are a few comments that haven't been addressed, after which I'll close it.
Is this http://lxr.mozilla.org/mozilla/source/extensions/sql/odbc/ compatible with the XulRunner trunk 1.9? Where can I find a binary package of this extension, where a documentation of how to install it to XulRunner?
I'm gonna go ahead and close this now.  It's not worth keeping open.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.