RUBY-3816 Remove core driver antipatterns#3078
Open
jamis wants to merge 11 commits into
Open
Conversation
Replace metaprogramming reach-ins that mutated another object's private state with straightforward OO. Cluster.create and Database.create only ever mutated the client they were handed, via instance_variable_set, and were called only from Client#with. Move that behavior onto Client as reset_cluster! and reset_database!, and delete the two class factories. Convert Message.deserialize_array and .deserialize_field from class methods that set ivars on a passed-in message into instance methods, so the ivar access is ordinary self-assignment. Update both call sites.
Several sites used Object#send to invoke methods that are already public, which needlessly bypasses method visibility and hides intent. Switch them to public_send, or to a direct call where the method name is a literal. No behavior change: every affected method is public.
server_selection_diagnostic_message reached into each server monitor's private @thread ivar to decide whether the monitor was dead. BackgroundThread already exposes a public #running? predicate with exactly the needed semantics (nil thread or dead thread => not running), so use it. Guard with safe navigation: server.monitor can be nil, and the old instance_variable_get(:@thread) silently returned nil for that case (counting the monitor as dead), which &.running? preserves.
Client#with_session is already public, so three callers that invoked it via send now call it directly. Collection::View#with_session was private and reached via send from MapReduce; promote it to a public @api private method and call it directly. Also make Index::View#limit public (@api private). Cursor#limit calls @view.limit, and @view may be an Index::View, so the method must be public there as it already is on Collection::View. This removes the send that Cursor previously needed and fixes the case exposed when Cursor#limit stopped using send.
Session#txn_read_concern and Session#causal_consistency_doc were private and reached via send from operation and view code; make them public @api private and call them directly. Client#monitoring was public but then re-marked private, and reached via send from the auto encrypter; drop the private marker (it is already documented @api private) and call it directly.
Collection::View#server_selector (from Readable) was private and reached via send from MapReduce and Aggregation, whose @view is a Collection::View. Make it public @api private and call it directly. FSBucket#ensure_indexes! was private and reached via send from the write stream. Make it public @api private and call it directly.
Message#fields and Message#serialize_fields were private and reached via send during (de)serialization, including from Compressed. Make them public @api private and call them directly; promote the Compressed#serialize_fields override to match. PushMonitor#stop! reached into its connection with send(:socket).close to interrupt a blocking read. Add ConnectionCommon#interrupt_socket (@api private) that encapsulates this, and call it instead.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes several metaprogramming “reach-ins” (e.g., send, instance_variable_get/set) from the MongoDB Ruby driver by replacing them with explicit, encapsulated APIs and direct/public method calls, aiming to keep behavior unchanged while improving clarity and maintainability.
Changes:
- Replaces
send/instance_variable_*reach-ins with direct calls,public_send, and purpose-built@api privatepublic methods. - Refactors message field (de)serialization to use instance methods and explicit internal accessors.
- Moves client reconfiguration mutations into
Mongo::Client(reset_database!,reset_cluster!) and removes factory helpers onCluster/Database.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/mongo/socket/ssl.rb | Replaces reflective send calls for OpenSSL key parsing with direct calls. |
| lib/mongo/session.rb | Promotes internal session helpers to @api private public methods for direct callers. |
| lib/mongo/server/push_monitor.rb | Uses a dedicated connection interrupt API instead of reaching into private socket state. |
| lib/mongo/server/connection_common.rb | Adds interrupt_socket (@api private) to encapsulate socket interruption behavior. |
| lib/mongo/server_selector/base.rb | Uses Monitor#running? instead of reading a monitor’s private @thread. |
| lib/mongo/protocol/message.rb | Converts deserialization helpers to instance methods; exposes internal field helpers as @api private. |
| lib/mongo/protocol/compressed.rb | Removes send usage by calling serialize_fields directly on the wrapped message. |
| lib/mongo/options/redacted.rb | Switches to public_send for redaction string formatting. |
| lib/mongo/operation/shared/sessions_supported.rb | Calls session causal consistency helper directly (now @api private public). |
| lib/mongo/index/view.rb | Makes limit available as @api private to support polymorphic cursor behavior. |
| lib/mongo/grid/stream/write.rb | Calls FSBucket#ensure_indexes! directly instead of via send. |
| lib/mongo/grid/fs_bucket.rb | Promotes ensure_indexes! to @api private public so internal callers don’t use send. |
| lib/mongo/database.rb | Removes Database.create factory that mutated client private state. |
| lib/mongo/cursor.rb | Calls @view.limit directly (relies on Index::View#limit being available). |
| lib/mongo/crypt/handle.rb | Uses public_send for logger callback dispatch. |
| lib/mongo/crypt/auto_encrypter.rb | Uses client.monitoring directly (now public @api private). |
| lib/mongo/collection/view/readable.rb | Calls transaction read concern and exposes server_selector as @api private. |
| lib/mongo/collection/view/map_reduce.rb | Calls with_session/server_selector directly on view (@api private). |
| lib/mongo/collection/view/aggregation/behavior.rb | Calls server_selector directly on view (@api private). |
| lib/mongo/collection/view.rb | Promotes with_session to @api private public for other components to call directly. |
| lib/mongo/collection.rb | Calls client.with_session directly (avoids send). |
| lib/mongo/cluster/sdam_flow.rb | Uses public_send for descriptor accessors rather than send. |
| lib/mongo/cluster.rb | Removes Cluster.create factory that mutated client private state. |
| lib/mongo/client.rb | Adds reset_database!/reset_cluster!; makes monitoring callable directly (already @api private). |
| lib/mongo/bulk_write/result_combiner.rb | Uses public_send when reading result fields. |
| lib/mongo/auth/user/view.rb | Calls client.with_session directly (avoids send). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
Description
The ticket named
instance_variable_setinMongo::Cluster.createas one example of a structural antipattern. This branch audits the driver for that broader class of "brittle or surprising" patterns and removes the ones that are safe, behavior-preserving cleanups. Every change replaces a metaprogramming reach-in — mutating another object's private state, or calling past method visibility withsend— with straightforward, encapsulated OO.No behavior changes: the affected methods keep their existing contracts. Each batch was linted and run against a local replica set.
The audit also looked at structural OO smells (god objects, long parameter lists) and thread-safety, and cataloged concurrency and metaprogramming sites that are better handled as follow-ups (see below). This branch is scoped to the low-risk encapsulation fixes.
What changed
Stop mutating a caller's private state (the ticket's named example)
Cluster.createandDatabase.createonly ever mutated the client they were handed, viainstance_variable_set, and were called only fromClient#with. That behavior now lives onClientasreset_cluster!andreset_database!, and the two class factories are gone.Message.deserialize_array/.deserialize_fieldwere class methods that set ivars on a passed-in message. They are now instance methods, so the ivar access is ordinary self-assignment.Call methods directly or via
public_sendinstead ofsendsendto invoke methods that are already public (redacted.rb,sdam_flow.rb,crypt/handle.rb,socket/ssl.rb,cursor.rb,result_combiner.rb). Switched topublic_sendor a direct call.Client#with_sessionis public, so three callers now call it directly.Promote genuinely-internal methods to
@api privatepublicWhere
sendwas reaching a private method on another object, the method is now a documented@api privatepublic method and the call is direct:Session#txn_read_concern,Session#causal_consistency_docClient#monitoring(dropped a redundantprivatemarker)Collection::View#with_session,Collection::View#server_selectorIndex::View#limit(also needed byCursor#limit, whose@viewmay be an index view)FSBucket#ensure_indexes!Message#fields,Message#serialize_fieldsReplace reach-ins with purpose-built accessors
server_selection_diagnostic_messageread a monitor's private@threadivar; it now uses the existing publicMonitor#running?predicate (guarded with safe navigation, since a server's monitor may be nil).PushMonitor#stop!didconnection.send(:socket).closeto interrupt a blocking read; that is nowConnectionCommon#interrupt_socket(@api private).Testing
Each batch was checked with RuboCop and run against a local replica set (
spec/mongo/...for every changed class, plus the relevantspec/integration/specs). All pass; no new failures.One caught regression is worth calling out: an early swap changed
Cursor#limitfrom@view.send(:limit)to@view.limit, but@viewcan be aMongo::Index::View, wherelimitwas private. The specs caught it, and the fix was to makeIndex::View#limitpublic@api private(matchingCollection::View). General lesson for reviewers: replacing a metaprogramming reach with a real call can surface polymorphic or nil cases thatsend/instance_variable_getsilently tolerated.