test: add tests for PeerList type

josecelano · josecelano · commit 40182b49e0c2 · 2024-04-16T12:00:28.000+01:00
diff --git a/packages/primitives/src/peer.rs b/packages/primitives/src/peer.rs
@@ -362,6 +362,38 @@ pub mod fixture {
     }
 
     impl PeerBuilder {
+        #[allow(dead_code)]
+        #[must_use]
+        pub fn seeder() -> Self {
+            let peer = Peer {
+                peer_id: Id(*b"-qB00000000000000001"),
+                peer_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080),
+                updated: DurationSinceUnixEpoch::new(1_669_397_478_934, 0),
+                uploaded: NumberOfBytes(0),
+                downloaded: NumberOfBytes(0),
+                left: NumberOfBytes(0),
+                event: AnnounceEvent::Completed,
+            };
+
+            Self { peer }
+        }
+
+        #[allow(dead_code)]
+        #[must_use]
+        pub fn leecher() -> Self {
+            let peer = Peer {
+                peer_id: Id(*b"-qB00000000000000002"),
+                peer_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)), 8080),
+                updated: DurationSinceUnixEpoch::new(1_669_397_478_934, 0),
+                uploaded: NumberOfBytes(0),
+                downloaded: NumberOfBytes(0),
+                left: NumberOfBytes(10),
+                event: AnnounceEvent::Started,
+            };
+
+            Self { peer }
+        }
+
         #[allow(dead_code)]
         #[must_use]
         pub fn with_peer_id(mut self, peer_id: &Id) -> Self {
@@ -390,6 +422,13 @@ pub mod fixture {
             self
         }
 
+        #[allow(dead_code)]
+        #[must_use]
+        pub fn last_updated_on(mut self, updated: DurationSinceUnixEpoch) -> Self {
+            self.peer.updated = updated;
+            self
+        }
+
         #[allow(dead_code)]
         #[must_use]
         pub fn build(self) -> Peer {
diff --git a/packages/torrent-repository/src/entry/peer_list.rs b/packages/torrent-repository/src/entry/peer_list.rs
@@ -1,7 +1,13 @@
+//! A peer list.
 use std::net::SocketAddr;
 use std::sync::Arc;
 
-use torrust_tracker_primitives::peer;
+use torrust_tracker_primitives::{peer, DurationSinceUnixEpoch};
+
+// code-review: the current implementation uses the peer Id as the ``BTreeMap``
+// key. That would allow adding two identical peers except for the Id.
+// For example, two peers with the same socket address but a different peer Id
+// would be allowed. That would lead to duplicated peers in the tracker responses.
 
 #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct PeerList {
@@ -19,45 +25,48 @@ impl PeerList {
         self.peers.is_empty()
     }
 
-    pub fn insert(&mut self, key: peer::Id, value: Arc<peer::Peer>) -> Option<Arc<peer::Peer>> {
-        self.peers.insert(key, value)
+    pub fn upsert(&mut self, value: Arc<peer::Peer>) -> Option<Arc<peer::Peer>> {
+        self.peers.insert(value.peer_id, value)
     }
 
     pub fn remove(&mut self, key: &peer::Id) -> Option<Arc<peer::Peer>> {
         self.peers.remove(key)
     }
 
-    pub fn retain<F>(&mut self, f: F)
-    where
-        F: FnMut(&peer::Id, &mut Arc<peer::Peer>) -> bool,
-    {
-        self.peers.retain(f);
+    pub fn remove_inactive_peers(&mut self, current_cutoff: DurationSinceUnixEpoch) {
+        self.peers
+            .retain(|_, peer| peer::ReadInfo::get_updated(peer) > current_cutoff);
     }
 
     #[must_use]
-    pub fn seeders_and_leechers(&self) -> (usize, usize) {
-        let seeders = self.peers.values().filter(|peer| peer.is_seeder()).count();
-        let leechers = self.len() - seeders;
-
-        (seeders, leechers)
+    pub fn get(&self, peer_id: &peer::Id) -> Option<&Arc<peer::Peer>> {
+        self.peers.get(peer_id)
     }
 
     #[must_use]
-    pub fn get_peers(&self, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {
+    pub fn get_all(&self, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {
         match limit {
             Some(limit) => self.peers.values().take(limit).cloned().collect(),
             None => self.peers.values().cloned().collect(),
         }
     }
 
     #[must_use]
-    pub fn get_peers_for_client(&self, client: &SocketAddr, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {
+    pub fn seeders_and_leechers(&self) -> (usize, usize) {
+        let seeders = self.peers.values().filter(|peer| peer.is_seeder()).count();
+        let leechers = self.len() - seeders;
+
+        (seeders, leechers)
+    }
+
+    #[must_use]
+    pub fn get_peers_excluding_addr(&self, peer_addr: &SocketAddr, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {
         match limit {
             Some(limit) => self
                 .peers
                 .values()
                 // Take peers which are not the client peer
-                .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *client)
+                .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *peer_addr)
                 // Limit the number of peers on the result
                 .take(limit)
                 .cloned()
@@ -66,9 +75,215 @@ impl PeerList {
                 .peers
                 .values()
                 // Take peers which are not the client peer
-                .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *client)
+                .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *peer_addr)
                 .cloned()
                 .collect(),
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+
+    mod it_should {
+        use std::net::{IpAddr, Ipv4Addr, SocketAddr};
+        use std::sync::Arc;
+
+        use torrust_tracker_primitives::peer::fixture::PeerBuilder;
+        use torrust_tracker_primitives::peer::{self};
+        use torrust_tracker_primitives::DurationSinceUnixEpoch;
+
+        use crate::entry::peer_list::PeerList;
+
+        #[test]
+        fn be_empty_when_no_peers_have_been_inserted() {
+            let peer_list = PeerList::default();
+
+            assert!(peer_list.is_empty());
+        }
+
+        #[test]
+        fn have_zero_length_when_no_peers_have_been_inserted() {
+            let peer_list = PeerList::default();
+
+            assert_eq!(peer_list.len(), 0);
+        }
+
+        #[test]
+        fn allow_inserting_a_new_peer() {
+            let mut peer_list = PeerList::default();
+
+            let peer = PeerBuilder::default().build();
+
+            assert_eq!(peer_list.upsert(peer.into()), None);
+        }
+
+        #[test]
+        fn allow_updating_a_preexisting_peer() {
+            let mut peer_list = PeerList::default();
+
+            let peer = PeerBuilder::default().build();
+
+            peer_list.upsert(peer.into());
+
+            assert_eq!(peer_list.upsert(peer.into()), Some(Arc::new(peer)));
+        }
+
+        #[test]
+        fn allow_getting_all_peers() {
+            let mut peer_list = PeerList::default();
+
+            let peer = PeerBuilder::default().build();
+
+            peer_list.upsert(peer.into());
+
+            assert_eq!(peer_list.get_all(None), [Arc::new(peer)]);
+        }
+
+        #[test]
+        fn allow_getting_one_peer_by_id() {
+            let mut peer_list = PeerList::default();
+
+            let peer = PeerBuilder::default().build();
+
+            peer_list.upsert(peer.into());
+
+            assert_eq!(peer_list.get(&peer.peer_id), Some(Arc::new(peer)).as_ref());
+        }
+
+        #[test]
+        fn increase_the_number_of_peers_after_inserting_a_new_one() {
+            let mut peer_list = PeerList::default();
+
+            let peer = PeerBuilder::default().build();
+
+            peer_list.upsert(peer.into());
+
+            assert_eq!(peer_list.len(), 1);
+        }
+
+        #[test]
+        fn decrease_the_number_of_peers_after_removing_one() {
+            let mut peer_list = PeerList::default();
+
+            let peer = PeerBuilder::default().build();
+
+            peer_list.upsert(peer.into());
+
+            peer_list.remove(&peer.peer_id);
+
+            assert!(peer_list.is_empty());
+        }
+
+        #[test]
+        fn allow_removing_an_existing_peer() {
+            let mut peer_list = PeerList::default();
+
+            let peer = PeerBuilder::default().build();
+
+            peer_list.upsert(peer.into());
+
+            peer_list.remove(&peer.peer_id);
+
+            assert_eq!(peer_list.get(&peer.peer_id), None);
+        }
+
+        #[test]
+        fn allow_getting_all_peers_excluding_peers_with_a_given_address() {
+            let mut peer_list = PeerList::default();
+
+            let peer1 = PeerBuilder::default()
+                .with_peer_id(&peer::Id(*b"-qB00000000000000001"))
+                .with_peer_addr(&SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 6969))
+                .build();
+            peer_list.upsert(peer1.into());
+
+            let peer2 = PeerBuilder::default()
+                .with_peer_id(&peer::Id(*b"-qB00000000000000002"))
+                .with_peer_addr(&SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)), 6969))
+                .build();
+            peer_list.upsert(peer2.into());
+
+            assert_eq!(peer_list.get_peers_excluding_addr(&peer2.peer_addr, None), [Arc::new(peer1)]);
+        }
+
+        #[test]
+        fn return_the_number_of_seeders_in_the_list() {
+            let mut peer_list = PeerList::default();
+
+            let seeder = PeerBuilder::seeder().build();
+            let leecher = PeerBuilder::leecher().build();
+
+            peer_list.upsert(seeder.into());
+            peer_list.upsert(leecher.into());
+
+            let (seeders, _leechers) = peer_list.seeders_and_leechers();
+
+            assert_eq!(seeders, 1);
+        }
+
+        #[test]
+        fn return_the_number_of_leechers_in_the_list() {
+            let mut peer_list = PeerList::default();
+
+            let seeder = PeerBuilder::seeder().build();
+            let leecher = PeerBuilder::leecher().build();
+
+            peer_list.upsert(seeder.into());
+            peer_list.upsert(leecher.into());
+
+            let (_seeders, leechers) = peer_list.seeders_and_leechers();
+
+            assert_eq!(leechers, 1);
+        }
+
+        #[test]
+        fn remove_inactive_peers() {
+            let mut peer_list = PeerList::default();
+            let one_second = DurationSinceUnixEpoch::new(1, 0);
+
+            // Insert the peer
+            let last_update_time = DurationSinceUnixEpoch::new(1_669_397_478_934, 0);
+            let peer = PeerBuilder::default().last_updated_on(last_update_time).build();
+            peer_list.upsert(peer.into());
+
+            // Remove peers not updated since one second after inserting the peer
+            peer_list.remove_inactive_peers(last_update_time + one_second);
+
+            assert_eq!(peer_list.len(), 0);
+        }
+
+        #[test]
+        fn not_remove_active_peers() {
+            let mut peer_list = PeerList::default();
+            let one_second = DurationSinceUnixEpoch::new(1, 0);
+
+            // Insert the peer
+            let last_update_time = DurationSinceUnixEpoch::new(1_669_397_478_934, 0);
+            let peer = PeerBuilder::default().last_updated_on(last_update_time).build();
+            peer_list.upsert(peer.into());
+
+            // Remove peers not updated since one second before inserting the peer.
+            peer_list.remove_inactive_peers(last_update_time - one_second);
+
+            assert_eq!(peer_list.len(), 1);
+        }
+
+        #[test]
+        fn allow_inserting_two_identical_peers_except_for_the_id() {
+            let mut peer_list = PeerList::default();
+
+            let peer1 = PeerBuilder::default()
+                .with_peer_id(&peer::Id(*b"-qB00000000000000001"))
+                .build();
+            peer_list.upsert(peer1.into());
+
+            let peer2 = PeerBuilder::default()
+                .with_peer_id(&peer::Id(*b"-qB00000000000000002"))
+                .build();
+            peer_list.upsert(peer2.into());
+
+            assert_eq!(peer_list.len(), 2);
+        }
+    }
+}
diff --git a/packages/torrent-repository/src/entry/single.rs b/packages/torrent-repository/src/entry/single.rs
@@ -43,11 +43,11 @@ impl Entry for EntrySingle {
     }
 
     fn get_peers(&self, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {
-        self.swarm.get_peers(limit)
+        self.swarm.get_all(limit)
     }
 
     fn get_peers_for_client(&self, client: &SocketAddr, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {
-        self.swarm.get_peers_for_client(client, limit)
+        self.swarm.get_peers_excluding_addr(client, limit)
     }
 
     fn upsert_peer(&mut self, peer: &peer::Peer) -> bool {
@@ -58,23 +58,22 @@ impl Entry for EntrySingle {
                 drop(self.swarm.remove(&peer::ReadInfo::get_id(peer)));
             }
             AnnounceEvent::Completed => {
-                let previous = self.swarm.insert(peer::ReadInfo::get_id(peer), Arc::new(*peer));
+                let previous = self.swarm.upsert(Arc::new(*peer));
                 // Don't count if peer was not previously known and not already completed.
                 if previous.is_some_and(|p| p.event != AnnounceEvent::Completed) {
                     self.downloaded += 1;
                     downloaded_stats_updated = true;
                 }
             }
             _ => {
-                drop(self.swarm.insert(peer::ReadInfo::get_id(peer), Arc::new(*peer)));
+                drop(self.swarm.upsert(Arc::new(*peer)));
             }
         }
 
         downloaded_stats_updated
     }
 
     fn remove_inactive_peers(&mut self, current_cutoff: DurationSinceUnixEpoch) {
-        self.swarm
-            .retain(|_, peer| peer::ReadInfo::get_updated(peer) > current_cutoff);
+        self.swarm.remove_inactive_peers(current_cutoff);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -43,11 +43,11 @@ impl Entry for EntrySingle {`
`43`	`43`	`}`
`44`	`44`
`45`	`45`	`fn get_peers(&self, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {`
`46`		`- self.swarm.get_peers(limit)`
	`46`	`+ self.swarm.get_all(limit)`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`fn get_peers_for_client(&self, client: &SocketAddr, limit: Option<usize>) -> Vec<Arc<peer::Peer>> {`
`50`		`- self.swarm.get_peers_for_client(client, limit)`
	`50`	`+ self.swarm.get_peers_excluding_addr(client, limit)`
`51`	`51`	`}`
`52`	`52`
`53`	`53`	`fn upsert_peer(&mut self, peer: &peer::Peer) -> bool {`
`@@ -58,23 +58,22 @@ impl Entry for EntrySingle {`
`58`	`58`	`drop(self.swarm.remove(&peer::ReadInfo::get_id(peer)));`
`59`	`59`	`}`
`60`	`60`	`AnnounceEvent::Completed => {`
`61`		`- let previous = self.swarm.insert(peer::ReadInfo::get_id(peer), Arc::new(*peer));`
	`61`	`+ let previous = self.swarm.upsert(Arc::new(*peer));`
`62`	`62`	`// Don't count if peer was not previously known and not already completed.`
`63`	`63`	`if previous.is_some_and(\|p\| p.event != AnnounceEvent::Completed) {`
`64`	`64`	`self.downloaded += 1;`
`65`	`65`	`downloaded_stats_updated = true;`
`66`	`66`	`}`
`67`	`67`	`}`
`68`	`68`	`_ => {`
`69`		`- drop(self.swarm.insert(peer::ReadInfo::get_id(peer), Arc::new(*peer)));`
	`69`	`+ drop(self.swarm.upsert(Arc::new(*peer)));`
`70`	`70`	`}`
`71`	`71`	`}`
`72`	`72`
`73`	`73`	`downloaded_stats_updated`
`74`	`74`	`}`
`75`	`75`
`76`	`76`	`fn remove_inactive_peers(&mut self, current_cutoff: DurationSinceUnixEpoch) {`
`77`		`- self.swarm`
`78`		`- .retain(\|_, peer\| peer::ReadInfo::get_updated(peer) > current_cutoff);`
	`77`	`+ self.swarm.remove_inactive_peers(current_cutoff);`
`79`	`78`	`}`
`80`	`79`	`}`