Fix RUBY-2558 :secondary_preferred read preference in 3.6+ sharded clusters performs primary read

lib/mongo/operation/parallel_scan/command.rb

@@ -40,8 +40,7 @@ def selector(connection)
               read_concern)
           end
           sel[:maxTimeMS] = max_time_ms if max_time_ms
-          update_selector_for_read_pref(sel, connection)
-          sel
+          add_read_preference_legacy(sel, connection)
         end
 
         def message(connection)

lib/mongo/operation/shared/read_preference_supported.rb

@@ -39,47 +39,44 @@ module ReadPreferenceSupported
       #
       # @since 2.0.0
       def options(connection)
-        add_slave_ok_flag_maybe(super, connection)
+        options = super
+        if add_slave_ok_flag?(connection)
+          flags = options[:flags]&.dup || []
+          flags << :slave_ok
+          options = options.merge(flags: flags)
+        end
+        options
       end
 
-      # Adds :slave_ok flag to options based on the read preference specified
-      # in the operation or implied by the topology that the connection's
-      # server is a part of.
+      # Whether to add the :slave_ok flag to the request based on the
+      # read preference specified in the operation or implied by the topology
+      # that the connection's server is a part of.
       #
-      # @param [ Hash ] options The options calculated so far.
       # @param [ Server::Connection ] connection The connection that the
       #   operation will be executed on.
       #
-      # @return [ Hash ] The new options.
-      def add_slave_ok_flag_maybe(options, connection)
-        add_flag =
-          # https://github.com/mongodb/specifications/blob/master/source/server-selection/server-selection.rst#topology-type-single
-          if connection.description.standalone?
-            # Read preference is never sent to standalones.
-            false
-          elsif connection.server.cluster.single?
-            # In Single topology the driver forces primaryPreferred read
-            # preference mode (via the slave_ok flag, in case of old servers)
-            # so that the query is satisfied.
-            true
-          else
-            # In replica sets and sharded clusters, read preference is passed
-            # to the server if one is specified by the application, and there
-            # is no default.
-            read && read.slave_ok?
-          end
-
-        if add_flag
-          options= options.dup
-          (options[:flags] ||= []) << :slave_ok
+      # @return [ true | false ] Whether the :slave_ok flag should be added.
+      def add_slave_ok_flag?(connection)
+        # https://github.com/mongodb/specifications/blob/master/source/server-selection/server-selection.rst#topology-type-single
+        if connection.description.standalone?
+          # Read preference is never sent to standalones.
+          false
+        elsif connection.server.cluster.single?
+          # In Single topology the driver forces primaryPreferred read
+          # preference mode (via the slave_ok flag, in case of old servers)
+          # so that the query is satisfied.
+          true
+        else
+          # In replica sets and sharded clusters, read preference is passed
+          # to the server if one is specified by the application, and there
+          # is no default.
+          read && read.slave_ok? || false
         end
-
-        options
       end
 
       def command(connection)
         sel = super
-        update_selector_for_read_pref(sel, connection)
+        add_read_preference_legacy(sel, connection)
       end
 
       # Adds $readPreference field to the command document.
@@ -98,14 +95,19 @@ def command(connection)
       #   operation will be executed on.
       #
       # @return [ Hash ] New command document to send to the server.
-      def update_selector_for_read_pref(sel, connection)
+      def add_read_preference_legacy(sel, connection)
         if read && connection.description.mongos? && read_pref = read.to_mongos
-          Mongo::Lint.validate_camel_case_read_preference(read_pref)
-          sel = sel[:$query] ? sel : {:$query => sel}
-          sel = sel.merge(:$readPreference => read_pref)
-        else
-          sel
+          # If the read preference contains only mode and mode is secondary
+          # preferred and we are sending to a pre-OP_MSG server, this read
+          # preference is indicated by the :slave_ok wire protocol flag
+          # and $readPreference command parameter isn't sent.
+          if read_pref != {mode: 'secondaryPreferred'}
+            Mongo::Lint.validate_camel_case_read_preference(read_pref)
+            sel = sel[:$query] ? sel : {:$query => sel}
+            sel = sel.merge(:$readPreference => read_pref)
+          end
         end
+        sel
       end
     end
   end

lib/mongo/operation/shared/sessions_supported.rb

@@ -174,9 +174,10 @@ def add_read_preference(sel, connection)
         elsif connection.description.mongos?
           # When server is a mongos:
           # - $readPreference is never sent when mode is 'primary'
-          # - When mode is 'secondaryPreferred' $readPreference is only sent
-          #   when a non-mode field (i.e. tag_sets) is present
           # - Otherwise $readPreference is sent
+          # When mode is 'secondaryPreferred' $readPreference is currently
+          # required to only be sent when a non-mode field (i.e. tag_sets)
+          # is present, but this causes wrong behavior (DRIVERS-1642).
           if read
             doc = read.to_mongos
             if doc

lib/mongo/protocol/msg.rb

@@ -46,8 +46,8 @@ class Msg < Message
       #   Msg.new([:more_to_come], {}, { ismaster: 1 },
       #           { type: 1, payload: { identifier: 'documents', sequence: [..] } })
       #
-      # @param [ Array<Symbol> ] flags The flag bits. Current supported values
-      #   are :more_to_come and :checksum_present.
+      # @param [ Array<Symbol> ] flags The flag bits. Currently supported
+      #   values are :more_to_come and :checksum_present.
       # @param [ Hash ] options The options.
       # @param [ BSON::Document, Hash ] main_document The document that will
       #   become the payload type 0 section. Can contain global args as they

lib/mongo/protocol/query.rb

@@ -49,18 +49,18 @@ class Query < Message
       # @example Find all user ids.
       #   Query.new('xgen', 'users', {}, :fields => {:id => 1})
       #
-      # @param database [String, Symbol] The database to query.
-      # @param collection [String, Symbol] The collection to query.
-      # @param selector [Hash] The query selector.
-      # @param options [Hash] The additional query options.
+      # @param [ String, Symbol ] database The database to query.
+      # @param [ String, Symbol ] collection The collection to query.
+      # @param [ Hash ] selector The query selector.
+      # @param [ Hash ] options The additional query options.
       #
-      # @option options :project [Hash] The projection.
-      # @option options :skip [Integer] The number of documents to skip.
-      # @option options :limit [Integer] The number of documents to return.
-      # @option options :flags [Array] The flags for the query message.
-      #
-      #   Supported flags: +:tailable_cursor+, +:slave_ok+, +:oplog_replay+,
-      #   +:no_cursor_timeout+, +:await_data+, +:exhaust+, +:partial+
+      # @option options [ Array<Symbol> ] :flags The flag bits.
+      #   Currently supported values are :await_data, :exhaust,
+      #   :no_cursor_timeout, :oplog_replay, :partial, :slave_ok,
+      #   :tailable_cursor.
+      # @option options [ Integer ] :limit The number of documents to return.
+      # @option options [ Hash ] :project The projection.
+      # @option options [ Integer ] :skip The number of documents to skip.
       def initialize(database, collection, selector, options = {})
         @database = database
         @namespace = "#{database}.#{collection}"

lib/mongo/server_selector/secondary_preferred.rb

@@ -89,13 +89,8 @@ def to_doc
       #
       # @since 2.0.0
       def to_mongos
-        if tag_sets.empty? && max_staleness.nil? && hedge.nil?
-          # The server preference is not sent to mongos as part of the query
-          # selector if there are no tag sets, for maximum backwards compatibility.
-          nil
-        else
-          to_doc
-        end
+        # Always send the read preference to mongos: DRIVERS-1642.
+        to_doc
       end
 
       private

spec/integration/secondary_reads_spec.rb

@@ -0,0 +1,102 @@
+require 'spec_helper'
+
+describe 'Secondary reads' do
+  before do
+    root_authorized_client.use('sr')['secondary_reads'].drop
+    root_authorized_client.use('sr')['secondary_reads'].insert_one(test: 1)
+  end
+
+  shared_examples 'performs reads as per read preference' do
+
+    %i(primary primary_preferred).each do |mode|
+
+      context mode.inspect do
+
+        let(:client) do
+          root_authorized_client.with(read: {mode: mode}).use('sr')
+        end
+
+        it 'reads from primary' do
+          start_stats = get_read_counters
+
+          30.times do
+            client['secondary_reads'].find.to_a
+          end
+
+          end_stats = get_read_counters
+
+          end_stats[:secondary].should be_within(10).of(start_stats[:secondary])
+          end_stats[:primary].should >= start_stats[:primary] + 30
+        end
+      end
+    end
+
+    %i(secondary secondary_preferred).each do |mode|
+
+      context mode.inspect do
+        let(:client) do
+          root_authorized_client.with(read: {mode: mode}).use('sr')
+        end
+
+        it 'reads from secondaries' do
+          start_stats = get_read_counters
+
+          30.times do
+            client['secondary_reads'].find.to_a
+          end
+
+          end_stats = get_read_counters
+
+          end_stats[:primary].should be_within(10).of(start_stats[:primary])
+          end_stats[:secondary].should >= start_stats[:secondary] + 30
+        end
+      end
+    end
+  end
+
+  context 'replica set' do
+    require_topology :replica_set
+
+    include_examples 'performs reads as per read preference'
+  end
+
+  context 'sharded cluster' do
+    require_topology :sharded
+
+    include_examples 'performs reads as per read preference'
+  end
+
+  def get_read_counters
+    client = ClientRegistry.instance.global_client('root_authorized')
+    addresses = []
+    if client.cluster.sharded?
+      doc = client.use('admin').command(listShards: 1).documents.first
+      doc['shards'].each do |shard|
+        addresses += shard['host'].split('/').last.split(',')
+      end
+    else
+      client.cluster.servers.each do |server|
+        next unless server.primary? || server.secondary?
+        addresses << server.address.seed
+      end
+    end
+    stats = Hash.new(0)
+    addresses.each do |address|
+      ClientRegistry.instance.new_local_client(
+        [address],
+        SpecConfig.instance.all_test_options.merge(connect: :direct),
+      ) do |c|
+        server = c.cluster.servers.first
+        next unless server.primary? || server.secondary?
+        stat = c.command(serverStatus: 1).documents.first
+        queries = stat['opcounters']['query']
+        if server.primary?
+          stats[:primary] += queries
+        else
+          stats[:secondary] += queries
+        end
+      end
+    end
+    stats
+  end
+end

spec/mongo/operation/read_preference_legacy_spec.rb

@@ -49,10 +49,10 @@
     end
   end
 
-  describe '#add_slave_ok_flag_maybe' do
+  describe '#add_slave_ok_flag?' do
 
     let(:actual) do
-      operation.send(:add_slave_ok_flag_maybe, operation.send(:options), connection)
+      operation.send(:add_slave_ok_flag?, connection)
     end
 
     shared_examples_for 'sets the slave_ok flag as expected' do
@@ -63,9 +63,7 @@
 
     shared_examples_for 'never sets slave_ok' do
 
-      let(:expected) do
-        { }
-      end
+      let(:expected) { false }
 
       context 'when no read preference is specified' do
         let(:read_pref) { Mongo::ServerSelector.get }
@@ -88,9 +86,7 @@
 
     shared_examples_for 'always sets slave_ok' do
 
-      let(:expected) do
-        { :flags => [ :slave_ok ] }
-      end
+      let(:expected) { true }
 
       context 'when no read preference is specified' do
         let(:read_pref) { Mongo::ServerSelector.get }
@@ -117,9 +113,7 @@
 
         let(:read_pref) { Mongo::ServerSelector.get }
 
-        let(:expected) do
-          { }
-        end
+        let(:expected) { false }
 
         it_behaves_like 'sets the slave_ok flag as expected'
       end
@@ -130,9 +124,7 @@
 
           let(:read_pref) { Mongo::ServerSelector.get(:mode => :secondary) }
 
-          let(:expected) do
-            { :flags => [ :slave_ok ] }
-          end
+          let(:expected) { true }
 
           it_behaves_like 'sets the slave_ok flag as expected'
         end
@@ -141,9 +133,7 @@
 
           let(:read_pref) { Mongo::ServerSelector.get(:mode => :primary) }
 
-          let(:expected) do
-            { }
-          end
+          let(:expected) { false }
 
           it_behaves_like 'sets the slave_ok flag as expected'
         end
@@ -209,7 +199,7 @@
     end
   end
 
-  describe '#update_selector_for_read_pref' do
+  describe '#add_read_preference_legacy' do
 
     let(:read_pref) do
       Mongo::ServerSelector.get(:mode => mode)
@@ -218,7 +208,7 @@
     # Behavior of sending $readPreference is the same regardless of topology.
     shared_examples_for '$readPreference in the command' do
       let(:actual) do
-        operation.send(:update_selector_for_read_pref, operation.send(:selector), connection)
+        operation.send(:add_read_preference_legacy, operation.send(:selector), connection)
       end
 
       let(:expected_read_preference) do

spec/mongo/operation/read_preference_op_msg_spec.rb

@@ -178,13 +178,13 @@
         let(:tag_sets) { nil }
 
         context 'without tag_sets specified' do
-          it_behaves_like 'does not modify selector'
+          it_behaves_like 'adds read preference'
         end
 
         context 'with empty tag_sets' do
           let(:tag_sets) { [] }
 
-          it_behaves_like 'does not modify selector'
+          it_behaves_like 'adds read preference'
         end
 
         context 'with tag_sets specified' do
@@ -259,7 +259,7 @@
           let(:hedge) { nil }
 
           context 'when tag_sets and hedge are not specified' do
-            it_behaves_like 'does not modify selector'
+            it_behaves_like 'adds read preference'
           end
 
           context 'when tag_sets are specified' do