Skip to content

Commit 44dd995

Browse files
jamisCopilot
andauthored
RUBY-3816 Remove core driver antipatterns (#3078)
* RUBY-3816 Remove instance_variable_set reach-ins in core types 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. * RUBY-3816 Replace send with public_send/direct calls on public methods 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. * RUBY-3816 Use Monitor#running? instead of reaching into @thread 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. * RUBY-3816 Call with_session and Index::View#limit without send 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. * RUBY-3816 Expose internal Session and Client methods instead of 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. * RUBY-3816 Expose view server_selector and FSBucket#ensure_indexes! 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. * RUBY-3816 Expose message field helpers and connection socket interrupt 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. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * make this `@api private` instead of `private`, so it can be invoked internally * bump drivers-evergreen-tools to fix npm installation failure --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 7fa0679 commit 44dd995

27 files changed

Lines changed: 151 additions & 140 deletions

File tree

lib/mongo/auth/user/view.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ def generate(user, options)
152152
end
153153

154154
def execute_operation(options)
155-
client.send(:with_session, options) do |session|
155+
client.with_session(options) do |session|
156156
op = yield session
157157
op.execute(next_primary(nil, session), context: Operation::Context.new(client: client, session: session))
158158
end

lib/mongo/bulk_write/result_combiner.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ def result
9292

9393
def combine_counts!(result)
9494
Result::FIELDS.each do |field|
95-
if result.respond_to?(field) && (value = result.send(field))
95+
if result.respond_to?(field) && (value = result.public_send(field))
9696
results.merge!(field => (results[field] || 0) + value)
9797
end
9898
end

lib/mongo/client.rb

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,6 @@ def monitoring
172172
@monitoring
173173
end
174174
end
175-
private :monitoring
176175

177176
# Determine if this client is equivalent to another object.
178177
#
@@ -796,10 +795,10 @@ def use(name)
796795
def with(new_options = nil)
797796
clone.tap do |client|
798797
opts = client.update_options(new_options || Options::Redacted.new)
799-
Database.create(client)
798+
client.reset_database!
800799
# We can't use the same cluster if some options that would affect it
801800
# have changed.
802-
Cluster.create(client, monitoring: opts[:monitoring]) if cluster_modifying?(opts)
801+
client.reset_cluster!(monitoring: opts[:monitoring]) if cluster_modifying?(opts)
803802
end
804803
end
805804

@@ -855,6 +854,31 @@ def update_options(new_options)
855854
end
856855
end
857856

857+
# Replaces this client's database with a fresh instance built from the
858+
# client's current options. Used by #with so a reconfigured client does
859+
# not share its database with the client it was cloned from.
860+
#
861+
# @api private
862+
def reset_database!
863+
@database = Database.new(self, options[:database], options)
864+
end
865+
866+
# Replaces this client's cluster with a fresh instance built from the
867+
# client's current options. Used by #with so a reconfigured client does
868+
# not share its cluster with the client it was cloned from.
869+
#
870+
# @param [ Monitoring | nil ] monitoring The monitoring instance to use
871+
# with the new cluster. If nil, a new instance of Monitoring is created.
872+
#
873+
# @api private
874+
def reset_cluster!(monitoring: nil)
875+
@cluster = Cluster.new(
876+
cluster.addresses.map(&:to_s),
877+
monitoring || Monitoring.new,
878+
cluster_options
879+
)
880+
end
881+
858882
# Get the read concern for this client.
859883
#
860884
# @example Get the client read concern.

lib/mongo/cluster.rb

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -269,30 +269,6 @@ def initialize(seeds, monitoring, options = Options::Redacted.new)
269269
start_stop_srv_monitor
270270
end
271271

272-
# Create a cluster for the provided client, for use when we don't want the
273-
# client's original cluster instance to be the same.
274-
#
275-
# @example Create a cluster for the client.
276-
# Cluster.create(client)
277-
#
278-
# @param [ Client ] client The client to create on.
279-
# @param [ Monitoring | nil ] monitoring. The monitoring instance to use
280-
# with the new cluster. If nil, a new instance of Monitoring will be
281-
# created.
282-
#
283-
# @return [ Cluster ] The cluster.
284-
#
285-
# @since 2.0.0
286-
# @api private
287-
def self.create(client, monitoring: nil)
288-
cluster = Cluster.new(
289-
client.cluster.addresses.map(&:to_s),
290-
monitoring || Monitoring.new,
291-
client.cluster_options
292-
)
293-
client.instance_variable_set(:@cluster, cluster)
294-
end
295-
296272
# @return [ Hash ] The options hash.
297273
attr_reader :options
298274

lib/mongo/cluster/sdam_flow.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ def update_rs_without_primary
385385
def add_servers_from_desc(updated_desc)
386386
added_servers = []
387387
%w[hosts passives arbiters].each do |m|
388-
updated_desc.send(m).each do |address_str|
388+
updated_desc.public_send(m).each do |address_str|
389389
if (server = cluster.add(address_str, monitor: false))
390390
added_servers << server
391391
end
@@ -402,7 +402,7 @@ def add_servers_from_desc(updated_desc)
402402
# good primary).
403403
def remove_servers_not_in_desc(updated_desc)
404404
updated_desc_address_strs = %w[hosts passives arbiters].map do |m|
405-
updated_desc.send(m)
405+
updated_desc.public_send(m)
406406
end.flatten
407407
servers_list.each do |server|
408408
next if updated_desc_address_strs.include?(address_str = server.address.to_s)

lib/mongo/collection.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,7 @@ def create(opts = {})
393393
operation = { create: name }.merge(options)
394394
operation.delete(:write)
395395
operation.delete(:write_concern)
396-
client.send(:with_session, opts) do |session|
396+
client.with_session(opts) do |session|
397397
write_concern = if opts[:write_concern]
398398
WriteConcern.get(opts[:write_concern])
399399
else

lib/mongo/collection/view.rb

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,14 @@ def operation_timeouts(opts = {})
236236
end
237237
end
238238

239+
# Executes the provided block within the context of a session, using
240+
# this view's options merged with the given ones.
241+
#
242+
# @api private
243+
def with_session(opts = {}, &block)
244+
client.with_session(@options.merge(opts), &block)
245+
end
246+
239247
private
240248

241249
def initialize_copy(other)
@@ -252,10 +260,6 @@ def new(options)
252260
def view
253261
self
254262
end
255-
256-
def with_session(opts = {}, &block)
257-
client.with_session(@options.merge(opts), &block)
258-
end
259263
end
260264
end
261265
end

lib/mongo/collection/view/aggregation/behavior.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ def perform_setup(view, options, forbid: [])
8585
end
8686

8787
def server_selector
88-
@view.send(:server_selector)
88+
@view.server_selector
8989
end
9090

9191
def aggregate_spec(session, read_preference = nil)

lib/mongo/collection/view/map_reduce.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ def verbose(value = nil)
227227
#
228228
# @since 2.5.0
229229
def execute
230-
view.send(:with_session, @options) do |session|
230+
view.with_session(@options) do |session|
231231
write_concern = view.write_concern_with_session(session)
232232
context = Operation::Context.new(client: client, session: session)
233233
nro_write_with_retry(write_concern, context: context) do |connection, _txn_num, context|
@@ -241,7 +241,7 @@ def execute
241241
OUT_ACTIONS = %i[replace merge reduce].freeze
242242

243243
def server_selector
244-
@view.send(:server_selector)
244+
@view.server_selector
245245
end
246246

247247
def inline?

lib/mongo/collection/view/readable.rb

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -678,7 +678,7 @@ def timeout_ms(timeout_ms = nil)
678678
# @api private
679679
def read_concern
680680
if options[:session] && options[:session].in_transaction?
681-
options[:session].send(:txn_read_concern) || collection.client.read_concern
681+
options[:session].txn_read_concern || collection.client.read_concern
682682
else
683683
collection.read_concern
684684
end
@@ -749,12 +749,10 @@ def parallel_scan(cursor_count, options = {})
749749
end
750750
end
751751

752-
private
753-
754-
def collation(doc = nil)
755-
configure(:collation, doc)
756-
end
757-
752+
# The server selector for this view, derived from its read
753+
# preference (or the collection/client default).
754+
#
755+
# @api private
758756
def server_selector
759757
@server_selector ||= if options[:session] && options[:session].in_transaction?
760758
ServerSelector.get(read_preference || client.server_selector)
@@ -763,6 +761,12 @@ def server_selector
763761
end
764762
end
765763

764+
private
765+
766+
def collation(doc = nil)
767+
configure(:collation, doc)
768+
end
769+
766770
def validate_doc!(doc)
767771
raise Error::InvalidDocument.new unless doc.respond_to?(:keys)
768772
end

0 commit comments

Comments
 (0)