CDRIVER-3964 Append session to all operations in UTR

[benjirewis]

CDRIVER-3964

Appends session to all possible operations in the unified test runner.

Not all operations seem to take the session option. I noticed that operation_list_databases and bulk_op_append cause errors if session is appended, and the operation_find_one_and_* operations cannot take a session option since they use mongoc_find_and_modify_opts_t * instead of bson_t * as their options param. Let me know if you think I missed an operation/some operation does not need the session option.

[jmikola]

I noticed that operation_list_databases and bulk_op_append cause errors if session is appended

mongoc_client_get_database_names_with_opts is documented as taking a sessionId parameter, so it should certainly work with mongoc_client_session_append. If not, that sounds like a bug.

As for bulk writes, the session option should be added directly to the mongoc_bulk_operation_t object via mongoc_bulk_operation_set_client_session.

and the operation_find_one_and_* operations cannot take a session option since they use mongoc_find_and_modify_opts_t * instead of bson_t * as their options param.

This looks like an oversight and should be reported as a new ticket. One possible workaround would be to forgo mongoc_collection_find_and_modify_with_opts in favor of executing a raw command with mongoc_database_read_write_command_with_opts; however, that would defeat the point of the test, which is to exercise the actual API.

@kevinAlbs Is this something worth addressing for the 5.0-compat release? Otherwise, I suppose we'll need to skip any tests that use findAndModify with a session.

[jmikola]

I don't think this works. mongoc_client_session_append is going to append a "sessionId" to opts, but that's only the internal libmongoc identifier and should not end up in the raw command document being sent to the server.

This test currently uses mongoc_collection_command_simple, which doesn't support sessions. I think you'll need to change it to use mongoc_database_read_command_with_opts instead. And in doing so, you should pass opts as its own parameter (removing bson_concat entirely).

FWIW, operation_distinct appears to be the only operation to use mongoc_collection_command_simple so I don't think this is a concern anywhere else.

[benjirewis]

Gotcha, thanks for the guidance. I believe you meant mongoc_collection_read_command_with_opts, so I switched to that with a separate opts parameter and no bson_concat.

[jmikola]

I was definitely referring to mongoc_database_read_command_with_opts. I forgot there were similarly named collection methods, but if you look at both functions you'll see they're just syntactic sugar for _mongoc_client_command_with_opts.

In my mind, commands are scoped to a database, not a collection, so I don't think about having a helper at the collection level. The command itself may refer to a collection name (e.g. find: <name>), but that's still something you'd have to manually specify in the command document.

Anywho, either method should work. And since distinct is a collection-level operation, it makes sense that you'll already have the necessary collection object on hand here.

[benjirewis]

Ah gotcha! I'll just use the mongoc_collection_read_command_with_opts since I have the collection handle already.

[jmikola]

Sorry for not bringing this up sooner, but re-reading this changing I now wonder if operation_distinct should be implemented at all if libmongoc doesn't actually provide a helper method for it.

@kevinAlbs: Any thoughts on this? Looking at operation_run in operation.c, it appears that an error will be thrown for unsupported operations in lieu of skipping the test. That seems odd given my experience with retryable reads, where drivers tend to skip tests for unsupported read operations (e.g. optional collection or database enumeration methods).

[benjirewis]

Good point. As it is, operation_distinct does not really test any C driver functionality (and I don't think it's documented that users should create their own distinct in this manner).

The Go and CXX test runners also error on unsupported operations. I think it would be ok to skip distinct tests since the C driver has no helper, but always skipping on unsupported operations might make it harder to recognize when a new operation requires a new case in the test runner.

[jmikola]

I'd be OK with leaving this as-is and then opening a separate CDRIVER issue to follow-up. That issue can propose an explicit skip for tests utilizing distinct operations. Alternatively, you might decide to utilize the is_test_skipped function I'm adding in #801 and just explicitly add tests using distinct to that list.

Either way, I think it makes sense to figure out how we want to handle this before the retryable read tests get ported and we need to skip its tests for unsupported operations (e.g. collection enumeration variants). In that case, the explicit list in is_test_skipped probably makes the most sense.

[benjirewis]

Filed CDRIVER-4024. Using something like is_test_skipped seems best to me; then we can explicitly list the tests with unsupported operations like distinct or collection enumeration variants. Leaving operation_distinct as-is for now, though.

[benjirewis]

Appended session to operation_list_databases; I just needed to make sure op->arguments wasn't NULL before appending it to opts.

Per the discussion here, bulk_op_append does not need a session appension since it's already appended in operation_bulk_write.

The operation_find_and_* tests still have no session appension, but I'll file a ticket to add a session option to mongoc_find_and_modify_opts_t.

[benjirewis]

Gotcha, thanks for the guidance. I believe you meant mongoc_collection_read_command_with_opts, so I switched to that with a separate opts parameter and no bson_concat.

[kevinAlbs]

This looks like an oversight and should be reported as a new ticket. One possible workaround would be to forgo mongoc_collection_find_and_modify_with_opts in favor of executing a raw command with mongoc_database_read_write_command_with_opts; however, that would defeat the point of the test, which is to exercise the actual API.

@kevinAlbs Is this something worth addressing for the 5.0-compat release? Otherwise, I suppose we'll need to skip any tests that use findAndModify with a session.

It's funky, but session options can be passed by first appending the session with mongoc_client_session_append to a bson_t. Then append the bson_t opts to the mongoc_find_and_modify_opts_t via mongoc_find_and_modify_opts_append (example). This is documented in mongoc_find_and_modify_opts_append

[kevinAlbs]

LGTM

[jmikola]

One question in #798 (comment), but LGTM otherwise.