Fix RUBY-1442 Topology(Description) has no server knowledge (#1121)

* Fix RUBY-1442 Topology(Description) has no server knowledge

* Redo server descriptions as a hash

Author: p-mongo

lib/mongo/cluster.rb

@@ -120,10 +120,14 @@ def initialize(seeds, monitoring, options = Options::Redacted.new)
         add(seed, monitor: false)
       end
 
-      publish_sdam_event(
-        Monitoring::TOPOLOGY_CHANGED,
-        Monitoring::Event::TopologyChanged.new(opening_topology, @topology)
-      ) if seeds.size >= 1
+      if seeds.size >= 1
+        # Recreate the topology to get the current server list into it
+        @topology = topology.class.new(topology.options, topology.monitoring, self)
+        publish_sdam_event(
+          Monitoring::TOPOLOGY_CHANGED,
+          Monitoring::Event::TopologyChanged.new(opening_topology, @topology)
+        )
+      end
 
       servers.each do |server|
         server.start_monitoring

lib/mongo/cluster/sdam_flow.rb

@@ -413,6 +413,10 @@ def commit_changes
 
       topology_changed_event_published = false
       if topology.object_id != cluster.topology.object_id || @need_topology_changed_event
+        # We are about to publish topology changed event.
+        # Recreate the topology instance to get its server descriptions
+        # up to date.
+        @topology = topology.class.new(topology.options, topology.monitoring, cluster)
         # This sends the SDAM event
         cluster.update_topology(topology)
         topology_changed_event_published = true
@@ -433,15 +437,13 @@ def commit_changes
         return
       end
 
-      # TODO previous and updated topologies should differ in
-      # their server descriptions but currently they are the same
-      # exact object - https://jira.mongodb.org/browse/RUBY-1442
-      # and https://jira.mongodb.org/browse/RUBY-1519
-      publish_sdam_event(
-        ::Mongo::Monitoring::TOPOLOGY_CHANGED,
-        ::Mongo::Monitoring::Event::TopologyChanged.new(topology, topology)
-      )
-      @previous_desc = updated_desc
+      # If we are here, there has been a change in the server descriptions
+      # in our topology, but topology class has not changed.
+      # Publish the topology changed event and recreate the topology to
+      # get the new list of server descriptions into it.
+      @topology = topology.class.new(topology.options, topology.monitoring, cluster)
+      # This sends the SDAM event
+      cluster.update_topology(topology)
     end
 
     # Checks if the cluster has a primary, and if not, transitions the topology

lib/mongo/cluster/topology/base.rb

@@ -56,6 +56,13 @@ def initialize(options, monitoring, cluster)
           @options = options
           @monitoring = monitoring
           @cluster = cluster
+          # The list of server descriptions is simply fixed at the time of
+          # topology creation. If server description change later, a
+          # new topology instance should be created.
+          @server_descriptions = {}
+          cluster.servers_list.each do |server|
+            @server_descriptions[server.address.to_s] = server.description
+          end
         end
 
         # @return [ Hash ] options The options.
@@ -74,6 +81,12 @@ def addresses
         # @return [ monitoring ] monitoring the monitoring.
         attr_reader :monitoring
 
+        # @return [ Hash ] server_descriptions The map of address strings to
+        #   server descriptions, one for each server in the cluster.
+        #
+        # @since 2.7.0
+        attr_reader :server_descriptions
+
         # The largest electionId ever reported by a primary.
         # May be nil.
         #

spec/mongo/cluster/topology/replica_set_spec.rb

@@ -14,6 +14,14 @@
     Mongo::Monitoring.new(monitoring: false)
   end
 
+  # Cluster needs a topology and topology needs a cluster...
+  # This temporary cluster is used for topology construction.
+  let(:temp_cluster) do
+    double('temp cluster').tap do |cluster|
+      allow(cluster).to receive(:servers_list).and_return([])
+    end
+  end
+
   let(:cluster) do
     double('cluster').tap do |cl|
       allow(cl).to receive(:topology).and_return(topology)
@@ -66,7 +74,7 @@
     context 'when a replica set name is provided' do
 
       let(:topology) do
-        described_class.new({ :replica_set_name => 'testing' }, monitoring, nil)
+        described_class.new({ :replica_set_name => 'testing' }, monitoring, temp_cluster)
       end
 
       let(:servers) do
@@ -82,21 +90,21 @@
   describe '.replica_set?' do
 
     it 'returns true' do
-      expect(described_class.new({replica_set_name: 'foo'}, monitoring, nil)).to be_replica_set
+      expect(described_class.new({replica_set_name: 'foo'}, monitoring, temp_cluster)).to be_replica_set
     end
   end
 
   describe '.sharded?' do
 
     it 'returns false' do
-      expect(described_class.new({replica_set_name: 'foo'}, monitoring, nil)).to_not be_sharded
+      expect(described_class.new({replica_set_name: 'foo'}, monitoring, temp_cluster)).to_not be_sharded
     end
   end
 
   describe '.single?' do
 
     it 'returns false' do
-      expect(described_class.new({replica_set_name: 'foo'}, monitoring, nil)).to_not be_single
+      expect(described_class.new({replica_set_name: 'foo'}, monitoring, temp_cluster)).to_not be_single
     end
   end
 
@@ -105,7 +113,7 @@
 
     it 'returns value given in constructor options' do
       topology = described_class.new({replica_set_name: 'foo', max_election_id: election_id},
-        monitoring, nil)
+        monitoring, temp_cluster)
 
       expect(topology.max_election_id).to eql(election_id)
     end
@@ -114,7 +122,7 @@
   describe '#max_set_version' do
     it 'returns value given in constructor options' do
       topology = described_class.new({replica_set_name: 'foo', max_set_version: 5},
-        monitoring, nil)
+        monitoring, temp_cluster)
 
       expect(topology.max_set_version).to eq(5)
     end
@@ -123,7 +131,7 @@
   describe '#has_readable_servers?' do
 
     let(:topology) do
-      described_class.new({replica_set_name: 'foo'}, monitoring, [])
+      described_class.new({replica_set_name: 'foo'}, monitoring, temp_cluster)
     end
 
     let(:cluster) do
@@ -290,7 +298,7 @@
   describe '#has_writable_servers?' do
 
     let(:topology) do
-      described_class.new({replica_set_name: 'foo'}, monitoring, [])
+      described_class.new({replica_set_name: 'foo'}, monitoring, temp_cluster)
     end
 
     context 'when a primary server exists' do
@@ -331,7 +339,7 @@
   describe '#new_max_set_version' do
     context 'initially nil' do
       let(:topology) do
-        described_class.new({replica_set_name: 'foo'}, monitoring, nil).tap do |topology|
+        described_class.new({replica_set_name: 'foo'}, monitoring, temp_cluster).tap do |topology|
           expect(topology.max_set_version).to be nil
         end
       end
@@ -364,7 +372,7 @@
     context 'initially not nil' do
       let(:topology) do
         described_class.new({replica_set_name: 'foo', max_set_version: 4},
-          monitoring, nil
+          monitoring, temp_cluster
         ).tap do |topology|
           expect(topology.max_set_version).to eq(4)
         end
@@ -411,7 +419,9 @@
   describe '#new_max_election_id' do
     context 'initially nil' do
       let(:topology) do
-        described_class.new({replica_set_name: 'foo'}, monitoring, nil).tap do |topology|
+        described_class.new({replica_set_name: 'foo'},
+          monitoring, temp_cluster,
+        ).tap do |topology|
           expect(topology.max_election_id).to be nil
         end
       end
@@ -448,7 +458,7 @@
 
       let(:topology) do
         described_class.new({replica_set_name: 'foo', max_election_id: old_election_id},
-          monitoring, nil
+          monitoring, temp_cluster,
         ).tap do |topology|
           expect(topology.max_election_id).to be old_election_id
         end

spec/mongo/cluster/topology/sharded_spec.rb

@@ -6,8 +6,16 @@
     Mongo::Address.new('127.0.0.1:27017')
   end
 
+  # Cluster needs a topology and topology needs a cluster...
+  # This temporary cluster is used for topology construction.
+  let(:temp_cluster) do
+    double('temp cluster').tap do |cluster|
+      allow(cluster).to receive(:servers_list).and_return([])
+    end
+  end
+
   let(:topology) do
-    described_class.new({}, monitoring, nil)
+    described_class.new({}, monitoring, temp_cluster)
   end
 
   let(:monitoring) do

spec/mongo/cluster/topology/single_spec.rb

@@ -10,8 +10,16 @@
     Mongo::Monitoring.new(monitoring: false)
   end
 
+  # Cluster needs a topology and topology needs a cluster...
+  # This temporary cluster is used for topology construction.
+  let(:temp_cluster) do
+    double('temp cluster').tap do |cluster|
+      allow(cluster).to receive(:servers_list).and_return([])
+    end
+  end
+
   let(:topology) do
-    described_class.new({}, monitoring, nil)
+    described_class.new({}, monitoring, temp_cluster)
   end
 
   let(:listeners) do

spec/mongo/cluster/topology/unknown_spec.rb

@@ -6,8 +6,16 @@
     Mongo::Monitoring.new(monitoring: false)
   end
 
+  # Cluster needs a topology and topology needs a cluster...
+  # This temporary cluster is used for topology construction.
+  let(:temp_cluster) do
+    double('temp cluster').tap do |cluster|
+      allow(cluster).to receive(:servers_list).and_return([])
+    end
+  end
+
   let(:topology) do
-    described_class.new({}, monitoring, nil)
+    described_class.new({}, monitoring, temp_cluster)
   end
 
   describe '.servers' do

spec/mongo/cluster_spec.rb

@@ -190,7 +190,7 @@
     context 'when the cluster has no servers' do
 
       let(:servers) do
-        [nil]
+        []
       end
 
       before do

spec/mongo/monitoring/event/server_closed_spec.rb

@@ -11,6 +11,7 @@
   let(:cluster) do
     double('cluster').tap do |cluster|
       allow(cluster).to receive(:addresses).and_return([address])
+      allow(cluster).to receive(:servers_list).and_return([])
     end
   end
 

spec/mongo/monitoring/event/server_description_changed_spec.rb

@@ -11,6 +11,7 @@
   let(:cluster) do
     double('cluster').tap do |cluster|
       allow(cluster).to receive(:addresses).and_return([address])
+      allow(cluster).to receive(:servers_list).and_return([])
     end
   end