PYTHON-5676 Consolidate command execution logic#2852
Open
aclark4life wants to merge 7 commits into
Open
Conversation
…hrough it Introduce pymongo/asynchronous/command_runner.py (auto-generates the sync mirror), the single async code path for command execution. run_command owns the full skeleton: STARTED/SUCCEEDED/FAILED command logging AND APM publishing together, the network round trip, $clusterTime gossip, _process_response, _check_command_response, failure conversion, and auto-encryption decryption. Route network.command() through it, removing the duplicated logging/APM/send/ receive/decrypt block. Behavior is preserved byte-for-byte (logging and APM event documents unchanged); no per-command object is allocated, so the hot path is unchanged.
Extend run_command with the cursor transport (conn.send_message/receive_message, exhaust more_to_come receive-only) and output hooks (unpack_res, cursor_id, is_command_response for legacy OP_QUERY, pool_opts, command_name, ensure_db for $db gossip, and a reply_doc_builder for the find/getMore/explain APM reply format). run_operation now builds the message, supplies the reply-doc builder, and keeps the Response/PinnedResponse wrapping; everything between is the shared run_command path. The legacy OP_QUERY response shaping is preserved (is_command_response=use_cmd), not deleted -- that dead-code cleanup stays out of this consolidation. Behavior (logging, APM events, exhaust/pinning, decryption) is unchanged.
Add process_response and decrypt_reply flags plus the conn.unack_write transport to run_command, then route bulk.write_command (acknowledged) and bulk.unack_write through it. The bulk paths pass process_response=False (they run _process_response at the call site, preserving check -> APM-succeed -> process ordering) and decrypt_reply=False (their commands are encrypted up front). The unack path publishes a copy of the command carrying the docs field while logging the bare command, matching the prior asymmetry. Drops the duplicated logging/APM/failure-conversion blocks (and the unreachable _convert_write_result-on-failure branch for unacknowledged writes).
Route client_bulk.write_command and client_bulk.unack_write through run_command
(process_response=False, decrypt_reply=False, conn.unack_write transport for the
unack path). The client-level swallow semantics stay at the call site: the
except wraps the raised error into reply={"error": exc} and runs the
$clusterTime gossip (exc.details for OperationFailure, else {}); the unack path
publishes a copy carrying ops/nsInfo while logging the bare command.
With this, all command execution -- standard commands, cursor find/getMore, and
both bulk write families -- flows through the single run_command path.
After the consolidation, this module no longer does any networking -- the send/receive round trip moved into command_runner.run_command. It now only encodes a command and runs its pre-flight (read preference/concern, collation, $clusterTime, auto-encryption, CSOT, OP_MSG encoding), so 'network' was misleading and collided with the lower-level network_layer.py (raw sockets). Pure rename: git mv the async module (synchro regenerates the sync mirror) and update the two pool.py imports. No behavior change.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
Failures related to #2853 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates MongoDB command execution (logging + command monitoring/APM publishing + send/receive + response processing/checking + clusterTime gossip + optional decryption) into a single shared implementation (run_command) and updates the major call sites (standard command path, cursor operations, bulk writes) to route through it. This reduces duplicated “command execution skeleton” logic while keeping behavioral compatibility.
Changes:
- Introduces
pymongo.{a,}synchronous.command_runner.run_command()as the central command execution path. - Refactors
Server.run_operation()(cursor operations) and bulk/client-bulk write paths to userun_command()with hooks for legacy reply shaping and ordering constraints. - Renames/repurposes the former
networkcommand module intocommand_encoderto reflect its role as an encoder + pre-flight layer (with the actual I/O handled byrun_command/network_layer).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pymongo/asynchronous/command_runner.py | Adds the async shared run_command() implementation (logging/APM + transport + response handling). |
| pymongo/synchronous/command_runner.py | Adds the sync shared run_command() implementation (mirror of async via synchro). |
| pymongo/asynchronous/command_encoder.py | Routes standard async command execution through run_command(); updates module docstring. |
| pymongo/synchronous/command_encoder.py | Routes standard sync command execution through run_command(); updates module docstring. |
| pymongo/asynchronous/server.py | Refactors async cursor operations to use run_command() with reply shaping for monitoring. |
| pymongo/synchronous/server.py | Refactors sync cursor operations to use run_command() with reply shaping for monitoring. |
| pymongo/asynchronous/bulk.py | Refactors async bulk write command/unack paths to use run_command() and preserve ordering. |
| pymongo/synchronous/bulk.py | Refactors sync bulk write command/unack paths to use run_command() and preserve ordering. |
| pymongo/asynchronous/client_bulk.py | Refactors async client-bulk write command/unack paths to use run_command() while preserving top-level error embedding behavior. |
| pymongo/synchronous/client_bulk.py | Refactors sync client-bulk write command/unack paths to use run_command() while preserving top-level error embedding behavior. |
| pymongo/asynchronous/pool.py | Updates import to use command_encoder.command after module rename. |
| pymongo/synchronous/pool.py | Updates import to use command_encoder.command after module rename. |
Comments suppressed due to low confidence (1)
pymongo/synchronous/command_encoder.py:21
- Docstring references
pymongo.command_runner.run_command, but there is no top-levelpymongo.command_runnermodule in this codebase. Since this file importsrun_commandfrompymongo.synchronous.command_runner, the Sphinx reference should match to avoid broken cross-references / confusion.
NoahStapp
requested changes
Jun 5, 2026
| if bwc.publish: | ||
| bwc._start(cmd, request_id, op_docs, ns_docs) | ||
| try: | ||
| reply = bwc.conn.write_command(request_id, msg, bwc.codec) # type: ignore[misc, arg-type] |
Contributor
There was a problem hiding this comment.
AsyncConnection.write_command doesn't appear to have any remaining callers, so it (and its sync counterpart) are dead code that should be removed.
| result = await bwc.conn.unack_write(msg, max_doc_size) # type: ignore[func-returns-value, misc, override] | ||
| duration = datetime.datetime.now() - bwc.start_time | ||
| if result is not None: | ||
| reply = _convert_write_result(bwc.name, cmd, result) # type: ignore[arg-type] |
Contributor
There was a problem hiding this comment.
_convert_write_result is also dead code now.
| serviceId=bwc.conn.service_id, | ||
| ) | ||
| if bwc.publish: | ||
| bwc._start(cmd, request_id, docs) |
Contributor
There was a problem hiding this comment.
_BulkWriteContext._start, _BulkWriteContext._succeed, _BulkWriteContext._fail, and _ClientBulkWriteContext._start are all dead code.
| _IS_SYNC = False | ||
|
|
||
|
|
||
| async def run_command( |
Contributor
There was a problem hiding this comment.
This number of kwargs, several of which directly gate which network behavior to use, is extremely hard to parse and makes every call site much more complex than before. What if we split those different paths into three public methods that all wrap a private _run_command: one for acknowledged commands, one for unacknowledged commands, and one for cursor commands? That way we still have one unified command execution point, but multiple entry points that don't require a huge list of kwargs on every invocation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PYTHON-5676
Changes in this PR
Consolidates command execution into a single code path, the ticket's definition of done
("all database operations are done from a central location").
Previously the same execution skeleton —
start clock → log/publish STARTED → send → receive → _process_response → _check_command_response → log/publish SUCCEEDED|FAILED— was copy-pasted acrossfive sites that differed only in how bytes hit the wire. This PR introduces a new module
pymongo/asynchronous/command_runner.py(auto-generating the sync mirror) whoserun_command()owns that entire skeleton: command logging and APM publishing together, the send/receive round
trip,
$clusterTimegossip,_process_response,_check_command_response, failure conversion, andauto-encryption decryption. Callers pass only what varies (the encoded message plus a few
transport/output hooks).
All five sites now route through
run_command(), one per commit:network.command()— rawasync_sendall/async_receive_messagetransport.Server.run_operation()—conntransport with exhaustmore_to_come,unpack_res, areply_doc_builderfor the find/getMore/explain APM reply format; keeps theResponse/PinnedResponsewrapping at the call site.bulk.write_command/unack_write—conn.write_command/conn.unack_write; passprocess_response=False(process at the call site to preserve check → APM-succeed → processordering) and
decrypt_reply=False(bulk commands are encrypted up front).client_bulk.write_command/unack_write— same, with the client-level{"error": exc}swallow kept at the call site.
New private API:
pymongo.asynchronous.command_runner.run_command(+ sync mirror). No public APIchanges.
A final commit renames the standard-command module
network.py→command_encoder.py. Afterthis consolidation it no longer does any networking — the send/receive round trip moved into
run_command— so it now only encodes a command and runs its pre-flight (read pref/concern,collation,
$clusterTime, encryption, CSOT, OP_MSG). The old name was misleading and collided withthe lower-level
network_layer.py(raw sockets). Pure rename:git mvthe async module (synchroregenerates the sync mirror) plus the two
pool.pyimport updates; no behavior change.Differences from #2789
pymongo/asynchronous/so it isasyncand is synchro-converted — thereverted
_telemetry.pywas top-level (sync-only) and structurally could not own the I/O.duplicated at every call site).
is_command_response=use_cmd), not deleted —that dead-code cleanup is intentionally out of scope.
log-omits-documents / publish-includes-documents asymmetry, and client_bulk's swallow +
{}-vs-exc.detailsgossip.Test Plan
No new tests: this is a behavior-preserving refactor, and the existing APM/command-monitoring and
command-logging spec suites assert the exact event/log documents that any regression here would change.
Checklist
Checklist for Author
user-facing behavior change.
now-unreachable legacy OP_QUERY shaping (kept out of this PR by design); no ticket yet.
Checklist for Reviewer