Closed Bug 570529 Opened 10 years ago Closed 10 years ago

mozIStorageStatement.execute() should reset itself even if an error occurs

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
status1.9.2 --- .7-fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file)

mozIStorageStatement.execute reads to me like after it is complete the statement should be ready for re-use. However in the event of an error occurring the statement does not get reset.
Blocks: 570484
Attached patch patch rev 1Splinter Review
Kinda weird with the multiple nsresults but I think we want to return the result from ExecuteStep if it was a failure or the result from Reset otherwise.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #449677 - Flags: review?(sdwilsh)
Comment on attachment 449677 [details] [diff] [review]
patch rev 1

>+function test_failed_execute()
>+{
>+  var stmt = createStatement("INSERT INTO test (name) VALUES ('foo')");
>+  stmt.execute();
>+  stmt.finalize();
>+  var id = getOpenedDatabase().lastInsertRowID;
>+  stmt = createStatement("INSERT INTO test(id, name) VALUES(:id, 'bar')");
>+  stmt.params.id = id;
>+  try {
>+    // Should throw a constraint error
>+    stmt.execute();
>+    do_throw("Should have seen a constraint error");
>+  }
>+  catch (e) {
>+    do_check_eq(getOpenedDatabase().lastError, Ci.mozIStorageError.CONSTRAINT);
>+  }
>+  do_check_eq(Ci.mozIStorageStatement.MOZ_STORAGE_STATEMENT_READY, stmt.state);
>+  // Should succeed without needing to reset the statement manually
>+  stmt.finalize();
Did finalize actually fail before your change?  I don't think it would have (but executing another statement would have).

>-             test_getColumnDecltype];
>+             test_getColumnDecltype, test_failed_execute];
nit: place it on its own line with a trailing comma please (I know the rest of them are not like that, but moving forward, that's how it should be).

r=sdwilsh with comments addressed.
Attachment #449677 - Flags: review?(sdwilsh) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/47b178e78791
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
This was mistakenly landed on .7. We have decided to keep it as-is though, as it is low-risk.
You need to log in before you can comment on or make changes to this bug.