[SERVER-39360] assert.commandWorked with writeError sets error code to object Created: 03/Feb/19  Updated: 06/Dec/22

Status: Backlog
Project: Core Server
Component/s: Shell
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: A. Jesse Jiryu Davis Assignee: Backlog - Server Tooling and Methods (STM) (Inactive)
Resolution: Unresolved Votes: 0
Labels: tig-assertjs
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-38936 Create unittests for txn_override.js Closed
is related to SERVER-38583 Transactional bulkWrite error missing... Closed
Assigned Teams:
Server Tooling & Methods
Operating System: ALL
Sprint: Repl 2019-02-11
Participants:

 Description   

Try this:

try {
    assert.commandWorked({
        "n" : 0,
        "writeErrors" : [
                {
                        "index" : 0,
                        "code" : 263,
                        "errmsg" : "foo"
                }
        ],
        "ok" : 1
    });
} catch (ex) {
    print(typeof ex.code);
}

Expected: number
Actual: object

The problem is _getErrorWithCode(), compiled into the shell from utils.js. That function expects the provided error object to be a command failure with ok: 0, or an object with a "writeError" field, not a "writeErrors" array.

I think _getErrorWithCode() should check for writeErrors, too. I'm not sure "writeError" is ever actually present.

This is a problem for tests that check things like "ex.code === 123". If the command failed with a writeErrors array, the exception's code is an object, not the expected number, and the check may be false incorrectly.



 Comments   
Comment by Steven Vannelli [ 10/May/22 ]

Moving this ticket to the Backlog and removing the "Backlog" fixVersion as per our latest policy for using fixVersions.

Comment by Jack Mulrow [ 25/Apr/19 ]

Just a heads up, as part of SERVER-40186 I made a couple changes to _getErrorWithCode():

  1. Only set "e.code" if codeOrObj is a non-null object with a code property or codeOrObj is a number type
  2. Set any writeErrors on codeOrObj as "e.writeErrors"

I didn't make any changes to the places that don't pass "res" to doassert(), so that work can still be tracked by this ticket.

Comment by Max Hirschhorn [ 25/Feb/19 ]

I think trying to tie writeErrors from the server response and the code property in the resulting Error object is going to be challenging due to how using the === and !== operators are fairly entrenched in how we write JavaScript tests and we'd need to make those into function calls to be able allow comparing against more than one code. I think the action to take on this ticket to change the else-branch in the _getErrorWithCode() function to be based around whether the codeOrObj argument is typeof === 'number'.

Comment by A. Jesse Jiryu Davis [ 05/Feb/19 ]

Max noticed a couple places where `res` wasn't being given as an argument to doassert() inside the _assertCommandFailed() and assert.writeErrorWithCode() functions.

https://github.com/mongodb/mongo/blob/r4.1.7/src/mongo/shell/assert.js#L655
https://github.com/mongodb/mongo/blob/r4.1.7/src/mongo/shell/assert.js#L786

Comment by A. Jesse Jiryu Davis [ 03/Feb/19 ]

Agreed. The WriteResult has writeErrors but no writeError property. So it seems like the intent of this code had been to set "e.code" to one of the codes in the writeErrors array. (If there are multiple, I don't know which it should be.) That would be useful in passthrough tests with logic like this (fsm.js):

try {
    // Try func in a transaction.
    // We don't know what func() does, can't predict if it will fail with a top-level
    // error or a writeErrors array.
    session.startTransaction();
    func();
    session.commitTransaction();
} catch (e) {
    // Retry state functions that threw OperationNotSupportedInTransaction or
    // InvalidOptions errors outside of a transaction. Rethrow any other error.
    // Oops, "e.code" doesn't contain the code if it was in a writeErrors Array
    if (e.code !== ErrorCodes.OperationNotSupportedInTransaction) {
        throw e;
    }
 
    // Retry outside transaction.
    session.abortTransaction();
    func();
}

Comment by Max Hirschhorn [ 03/Feb/19 ]

Can you point to what object includes a "writeError" field? I can't find anything called "writeError", only the "writeErrors" array.

I didn't find anything either. My best guess is that there was a misunderstanding of how to access properties of a WriteResult instance when it represents a write error.

> db.mycoll.insert({_id: 1})
WriteResult({ "nInserted" : 1 })
> db.mycoll.insert({_id: 1})
WriteResult({
	"nInserted" : 0,
	"writeError" : {
		"code" : 11000,
		"errmsg" : "E11000 duplicate key error collection: test.mycoll index: _id_ dup key: { : 1.0 }"
	}
})

Comment by A. Jesse Jiryu Davis [ 03/Feb/19 ]

Thanks Max. Can you point to what object includes a "writeError" field? I can't find anything called "writeError", only the "writeErrors" array.

Comment by Max Hirschhorn [ 03/Feb/19 ]

The problem is _getErrorWithCode(), compiled into the shell from utils.js. That function expects the provided error object to be a command failure with ok: 0, or an object with a "writeError" field, not a "writeErrors" array.

The _getErrorWithCode() function set the "code" property to the entire response object because it is assumes the res argument to be a number. The _getErrorWithCode() function hasn't really been changed much since SERVER-19493 but at the time checking for .writeError rather than .writeErrors was found to be desirable.

} else {
    // At this point assume codeOrObj is a number type
    e.code = codeOrObj;
}

I think _getErrorWithCode() should check for writeErrors, too. I'm not sure "writeError" is ever actually present.

This is a problem for tests that check things like "ex.code === 123". If the command failed with a writeErrors array, the exception's code is an object, not the expected number, and the check may be false incorrectly.

I would argue that the _getErrorWithCode() function should have left the "code" property as undefined in this case. If you want to assert on a specific error code being returned, why not using the assert.commandFailedWithCode() function?

assert.commandFailedWithCode({
  n: 0,
  writeErrors: [
    {
      index: 0,
      code: 263,
      errmsg: 'foo'
    }
  ],
  ok: 1
}, ErrorCodes.OperationNotSupportedInTransaction);

Generated at Thu Feb 08 04:51:48 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.