Closed Bug 489897 Opened 12 years ago Closed 3 years ago

implement StatementRow::NewEnumerate

Categories

(Toolkit :: Storage, defect, P3)

x86
All
defect

Tracking

()

RESOLVED DUPLICATE of bug 1390489
mozilla1.9.3a1

People

(Reporter: ddahl, Unassigned)

Details

Attachments

(1 file, 7 obsolete files)

The row enumeration in the statement. just like Params
Summary: implement mozStorageStatementRow::NewEnumerate → implement StatementRow::NewEnumerate
Assignee: nobody → ddahl
Attached patch v 0.1 (obsolete) — Splinter Review
This cannot possibly actually work, Really. Please tell me I fucked up:)
Attachment #374517 - Flags: review?
Attachment #374517 - Flags: review? → review?(sdwilsh)
No - that could not have worked - do I need an execute_soon type call for the executeAnsync call?
This doesn't get created for the stuff returned from executeAsync, just executeStep.  You can then call stmt.row off of that.  See also https://developer.mozilla.org/en/mozIStorageStatementRow
Horrors! I thought I was helpoing out with async stuff:)
Attached patch v 0.2 (obsolete) — Splinter Review
Attachment #374534 - Flags: review?(sdwilsh)
Attachment #374517 - Attachment is obsolete: true
Attachment #374517 - Flags: review?(sdwilsh)
Comment on attachment 374534 [details] [diff] [review]
v 0.2

wrong patch, sorry
Attachment #374534 - Attachment is obsolete: true
Attachment #374534 - Flags: review?(sdwilsh)
Attached patch v 0.2 (obsolete) — Splinter Review
Attachment #374535 - Flags: review?(sdwilsh)
Comment on attachment 374535 [details] [diff] [review]
v 0.2

>+++ b/storage/test/unit/test_js_helpers_enumerate.js
>+function test_row_enumerate()
>+{
>+  var that = this;
nit: appears to be unused

>+  getOpenedDatabase().createTable('test_row_tbl','name TEXT, car TEXT');
nit: store getOpenedDatabase in a local variable called db or something please?

>+  let sql = "INSERT INTO test_row_tbl (name, car) VALUES ('david', 'Fiat 500')";
>+  let insertStmt = getOpenedDatabase().createStatement(sql);
>+  insertStmt.executeStep();
>+  insertStmt.finalize();
normally you'd want to use something like executeSimpleSQL off of the connection if you have a one-off like this.  However, I'd prefer it if you bound the parameters.

>+  let sql = "SELECT * FROM test_row_tbl";
>+  let selectStmt = getOpenedDatabase().createStatement(sql);
generally we just use the variable name stmt here, and you shouldn't need the sql variable (also, I think you've redeclared sql here).

I'd also just prefer if you modified the table creation run_test to make it have more than one column so we don't have to create another table.

>+  while(selectStmt.executeStep()){
You only have one row, so you should just do_check_true(stmt.executeStep())

>+    let name = selectStmt.row.name;
>+    do_check_eq(name,'david');
>+    let car  = selectStmt.row.car;
>+    do_check_eq(car,'Fiat 500');
This isn't actually testing enumeration.  You'll want to do a for (let name in stmt.row) and check for the column names similar to how test_params_enumerate does it for stmt.params

> let tests = [
>   test_params_enumerate,
>+  test_row_enumerate
nit: trailing comma please
Attachment #374535 - Flags: review?(sdwilsh) → review-
Attached patch WIP patch, fail! (obsolete) — Splinter Review
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/ddahl/code/moz/mozilla-central/mozilla/storage/src/mozStorageStatementRow.cpp, line 233
[Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_js_helpers_enumerate.js :: test_row_enumerate :: line 78"  data: no]
Attachment #374535 - Attachment is obsolete: true
Attachment #376317 - Flags: review?(sdwilsh)
Attachment #376317 - Flags: review?(sdwilsh)
Comment on attachment 376317 [details] [diff] [review]
WIP patch, fail!

>+++ b/storage/src/mozStorageStatementRow.cpp
>+      // Get column count.
>+      mStatement->GetColumnCount(&mColumnCount);
we should actually get and set this in the constructor

>+++ b/storage/src/mozStorageStatementRow.h
>@@ -56,16 +56,16 @@ public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_MOZISTORAGESTATEMENTROW
>   NS_DECL_NSIXPCSCRIPTABLE
> 
>   StatementRow(mozStorageStatement *aStatement);
> protected:
> 
>   mozStorageStatement *mStatement;
>-
>+  PRUint32 mColumnCount;
nit: newline before friend declatarion

>+++ b/storage/test/unit/test_js_helpers_enumerate.js
>+  let stmt = db.createStatement("INSERT INTO test (driver, car) VALUES (:driver, :car)");
you can actually just call createStatement (global function)

>+  stmt.params['driver'] = 'David';
>+  stmt.params['car'] = 'Fiat 500';
stmt.params.driver and stmt.paramd.car please

>+  stmt.executeStep();
just execute

>+  stmt.finalize();
and this should be in a finally block and execute should be in a try block

>+  let stmt = db.createStatement("SELECT driver FROM test WHERE driver = :driver");
redeclared stmt

>+  stmt.params['driver'] = 'David';
same as above

>+  stmt.executeStep();
do_check_true this

>+  let expected = ["driver",];
I'd like to have more than one column here, just to test more than one thing for an enumeration

>+  let index = 0;
>+  for (let colName in stmt.row){
>+    do_check_eq(colName,expected[index]);
>+    index++;
you can just do do_check_eq(colName, execpted[index++]) and lose the bracing.  To debug this, I'd add a dump statement indicating what colName is that you are getting.

>+  stmt.finalize();
needs to be in a finally block, and everything starting with executeStep needs to be in a try block

>   getOpenedDatabase().executeSimpleSQL(
>     "CREATE TABLE test (" +
>-      "id INTEGER PRIMARY KEY " +
>+      "id INTEGER PRIMARY KEY, driver VARCHAR(32) NULL, car VARCHAR(32) NULL" +
each column on a new line please
Attached patch Another hot cup of fail (obsolete) — Splinter Review
Everyone's favorite error:

[Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_js_helpers_enumerate.js :: test_row_enumerate :: line 79"  data: no]

Seems like a pretty fundamental JS error. Out of my league:)
Attachment #376317 - Attachment is obsolete: true
Attachment #376373 - Flags: review?(sdwilsh)
Attached patch updated hot cup of fail (obsolete) — Splinter Review
updated from some bitrot
Attachment #376373 - Attachment is obsolete: true
Attachment #376373 - Flags: review?(sdwilsh)
Attached patch v1.0 (obsolete) — Splinter Review
Changes to David's patch here are pretty minimal, but I'm going to have him take a look anyway.  sr for the API change.
Attachment #404132 - Attachment is obsolete: true
Attachment #404154 - Flags: superreview?(mrbkap)
Attachment #404154 - Flags: review?(ddahl)
Status: NEW → ASSIGNED
Whiteboard: [needs review ddahl][needs sr mrbkap]
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 404154 [details] [diff] [review]
v1.0

Looks good to me. I like the new clean style of storage. I had a bunch of failed tests reported, but i am sure it is a borked obj dir, rebuilding now just to make sure
Attachment #404154 - Flags: review?(ddahl) → review+
Whiteboard: [needs review ddahl][needs sr mrbkap] → [needs sr mrbkap]
Comment on attachment 404154 [details] [diff] [review]
v1.0

>     nsDependentCString jsid(::JS_GetStringBytes(JSVAL_TO_STRING(aId)));
> 
>+    // If we are trying to get the __iterator__ property, return early.  Our
>+    // NewEnumerate function will handle that.
>+    if (0 == strcmp(jsid.get(), "__iterator__"))

Given that jsid is already an nsDependentCString, can you just call EqualsLiteral here?

>@@ -147,6 +155,70 @@ StatementRow::GetProperty(nsIXPConnectWr
>+      JSString *jsname = ::JS_NewStringCopyN(aCtx, &(name.get()[1]),
>+                                             name.Length());
>+      NS_ENSURE_TRUE(jsname, NS_ERROR_OUT_OF_MEMORY);

I don't understand using &(name.get()[1]) but keeping the length. Does that not include a trailing null character or something? Either way, this needs a comment.
Attachment #404154 - Flags: superreview?(mrbkap) → superreview+
(In reply to comment #15)
> I don't understand using &(name.get()[1]) but keeping the length. Does that not
> include a trailing null character or something? Either way, this needs a
> comment.
This looks like a copy/paste error.  Should just be using name.get() here.
Attachment #404154 - Attachment is obsolete: true
I was hoping to get this into 3.6 - probably too late, eh? sadface.
Does this just need to be checked in then?
I though we were waiting on blake for sr+. Looks like i forgot to flag the patch.
Attachment #405963 - Flags: superreview?(mrbkap)
Attachment #405963 - Flags: superreview?(mrbkap)
Oh, the patch says it is already sr+
sdwilsh:

do you think this can land? this will be quite helpful for the placesQueryApi refactor
Keywords: checkin-needed
Whiteboard: [needs sr mrbkap]
http://hg.mozilla.org/mozilla-central/rev/64d3466e3964
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Fascinating.

The failing test code is inside pwmgr_common.js#131, where we remove all logins, add 1 test login, and then sanity check that it actually got added. Surprisingly, the sanity check fails.

With signon.debug on in a local test run, I see getAllLogins() failing with "PwMgr cryptoSDR: Failed to decrypt string:  (NS_ERROR_FAILURE)". We can't decrypt the login that just got added, so we return nothing. A dump of signons.sqlite at this point looks like it should be OK, so I dunno why this patch triggers a failure.

Next step would be to look at if the encrypted value is getting mangled somewhere, and why NSS thinks it suddenly can't decrypt things.
So, the data in the DB is the same as the data saved by addLogin.

The problem is that when we query the DB, we get back one row (as expected), but all the values (eg stmt.row.hostname) are |undefined|. And that's why the decryption fails.

Looks like there are other xpcshell test failures associated with this landing too... http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262239166.1262242381.12184.gz
Looks like some storage tests also failed. Weird - as I ran the entire storage test suite while testing this patch.
really strange. did another storage patch make this one toxic?  

8 tests fail now:

http://pastebin.mozilla.org/694177
I attempted to run the xpcshell tests that cover this patch via gdb and set a breakpoint in Statement::GetColumnIndex, but failed. check-interactive was not helpful. The breakpoint was not set, but would be set if gdb detected the libaray loading.

anyway... we seem to be failing due to a type issue:

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_js_helpers_enumerate.js :: test_row_enumerate :: line 81"  data: no]

which points to this method:

NS_IMETHODIMP
Statement::GetColumnIndex(const nsACString &aName,
                          PRUint32 *_index)
{
  if (!mDBStatement)
      return NS_ERROR_NOT_INITIALIZED;

  // Surprisingly enough, SQLite doesn't provide an API for this.  We have to
  // determine it ourselves sadly.
  for (PRUint32 i = 0; i < mResultColumnCount; i++) {
    if (mColumnNames[i].Equals(aName)) {
      *_index = i;
      return NS_OK;
    }
  }

  return NS_ERROR_INVALID_ARG;
}


I guess the NS_ERROR_INVALID_ARG is the same as  the ILLEGAL_VALUE above?
Assignee: ddahl → nobody
Priority: -- → P3
Bug 1390489 fixes this.
Status: REOPENED → RESOLVED
Closed: 11 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1390489
You need to log in before you can comment on or make changes to this bug.