Closed Bug 381549 Opened 13 years ago Closed 12 years ago

mozStorageStatement ignores return values from SQLite

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cbarrett, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 4 obsolete files)

http://mxr.mozilla.org/mozilla/source/storage/src/mozStorageStatement.cpp#287
http://www.sqlite.org/capi3ref.html#sqlite3_bind_text

We also do completely unnecessary checks that the implementation does for us. Filed on behalf of dwitte and sdwilsh.
Summary: mozStorage ignores return values from SQLite → mozStorageStatement ignores return values from SQLite
Attached patch v1.0 (obsolete) — Splinter Review
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #267510 - Flags: review?(sspitzer)
Attached patch v1.1 (obsolete) — Splinter Review
I missed two success codes that are kinda important in the last patch.
Attachment #267510 - Attachment is obsolete: true
Attachment #267517 - Flags: review?(sspitzer)
Attachment #267510 - Flags: review?(sspitzer)
Comment on attachment 267517 [details] [diff] [review]
v1.1

two minor nits:

1)

sean tells me the reason he didn't remove the "if (aParamIndex < 0 || aParamIndex >= mParamCount)" check before
sqlite3_bind_parameter_name() and sqlite3_column_name() is on bad values, they will return null, which we aren't treating as an error case.

sean, can you add a comment to that effect to those two places?


2)

+ * The Original Code is mozStorage Code.
+ *
+ * The Initial Developer of the Original Code is
+ *   Mozilla Corporation

To be fair, while you did extend it, I think Oracle should still get credit for ConvertResultCode(), and not Mozilla
and the Initial Developer should be Vladimir Vukicevic <vladimir.vukicevic@oracle.com>

This seems like the right thing to do when moving existing around to new files.
Attachment #267517 - Flags: review?(sspitzer) → review+
Attached patch v1.2 (obsolete) — Splinter Review
I actually missed mozStorageStatement::Clone in the last patch, so I did the changes to that in this one, as well as addressed the review comments.
Attachment #267517 - Attachment is obsolete: true
Attachment #267593 - Flags: review?(sspitzer)
Comment on attachment 267593 [details] [diff] [review]
v1.2

r=sspitzer

nit:  s/Sicne/Since

also, please run through the mozStorage unit tests and the places unit tests before landing.
Attachment #267593 - Flags: review?(sspitzer) → review+
Attached patch v1.3 (obsolete) — Splinter Review
With nit fixed.
Attachment #267593 - Attachment is obsolete: true
There's a slight issue with this patch still - in fact, it's the same as Bug 383673.  We only assert if we are out of bounds when everyone I've asked agrees that it should fail.

New patch coming to fix that...
Attached patch v1.4Splinter Review
Seth, I know we talked about keeping the assertions, but I think this error message is clear enough to not also have the assertion.
Attachment #267641 - Attachment is obsolete: true
Attachment #267669 - Flags: review?(sspitzer)
Comment on attachment 267669 [details] [diff] [review]
v1.4

r=sspitzer
Attachment #267669 - Flags: review?(sspitzer) → review+
Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.16; previous revision: 1.15
Checking in storage/src/mozStorageConnection.cpp;
new revision: 1.17; previous revision: 1.16
Checking in storage/src/mozStorage.h;
initial revision: 1.1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.