From 0561c49d4744ba59eab6130c98de24aff9812bf0 Mon Sep 17 00:00:00 2001
From: Davide Polonio <poloniodavide@gmail.com>
Date: Mon, 15 Jan 2024 12:50:57 +0100
Subject: [PATCH] chore: bump Invidious client deps, update tests

* set result sorting by most viewed to return the mostly seen video -
  which probably is the one we want (yes, wild assumption I know)
---
 Cargo.toml                |  2 +-
 src/search/spotify/mod.rs | 16 +++++--
 src/search/tests.rs       |  1 -
 src/search/youtube/mod.rs | 91 ++++++++++++++++++---------------------
 4 files changed, 56 insertions(+), 54 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 0a47bb4..8d742ce 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -12,7 +12,7 @@ pretty_env_logger = "0.5.0"
 tokio = { version =  "1.20.0", features = ["rt-multi-thread", "macros"] }
 rspotify = { version = "0.12.0", features = ["default"]}
 sentry = "0.32.1"
-invidious = "0.2.1"
+invidious = { version = "0.7.4", no-default-features = true, features = ["reqwest_async"]}
 itertools = "0.12.0"
 async-trait = "0.1.56"
 
diff --git a/src/search/spotify/mod.rs b/src/search/spotify/mod.rs
index 1336242..b8c380a 100644
--- a/src/search/spotify/mod.rs
+++ b/src/search/spotify/mod.rs
@@ -1,9 +1,9 @@
 use async_trait::async_trait;
 #[cfg(test)]
-use mockall::{automock, mock, predicate::*};
+use mockall::{automock, predicate::*};
 use rspotify::model::Country::UnitedStates;
 use rspotify::model::PlayableItem::{Episode, Track};
-use rspotify::model::{AlbumId, PlaylistId, TrackId, Market};
+use rspotify::model::{AlbumId, Market, PlaylistId, TrackId};
 use rspotify::prelude::*;
 use rspotify::{ClientCredsSpotify, Credentials};
 use std::sync::Arc;
@@ -101,7 +101,11 @@ impl SearchableClient for Client {
         };
 
         // Search track from US market
-        match self.client.track(track_id, Some(Market::Country(UnitedStates))).await {
+        match self
+            .client
+            .track(track_id, Some(Market::Country(UnitedStates)))
+            .await
+        {
             Ok(track) => Some(TrackInfo {
                 name: track.name,
                 artists: track.artists.iter().map(|x| x.name.clone()).collect(),
@@ -118,7 +122,11 @@ impl SearchableClient for Client {
         };
 
         // Search album from US market
-        match self.client.album(album_id, Some(Market::Country(UnitedStates))).await {
+        match self
+            .client
+            .album(album_id, Some(Market::Country(UnitedStates)))
+            .await
+        {
             Ok(album) => Some(AlbumInfo {
                 name: album.name,
                 artists: album.artists.iter().map(|x| x.name.clone()).collect(),
diff --git a/src/search/tests.rs b/src/search/tests.rs
index cc5c9fb..6df7797 100644
--- a/src/search/tests.rs
+++ b/src/search/tests.rs
@@ -35,7 +35,6 @@ async fn should_search_track_by_spotify_id() {
                     published: 0,
                     published_text: "".to_string(),
                     live_now: false,
-                    paid: false,
                     premium: false,
                 }],
             })
diff --git a/src/search/youtube/mod.rs b/src/search/youtube/mod.rs
index e52bc35..c5eb748 100644
--- a/src/search/youtube/mod.rs
+++ b/src/search/youtube/mod.rs
@@ -1,6 +1,9 @@
 use async_trait::async_trait;
+use invidious::hidden::SearchItem;
+use invidious::{ClientAsync, ClientAsyncTrait, MethodAsync};
+use itertools::Itertools;
 #[cfg(test)]
-use mockall::{automock, mock, predicate::*};
+use mockall::{automock, predicate::*};
 use std::error::Error;
 use std::sync::Arc;
 
@@ -33,7 +36,6 @@ pub(crate) struct Video {
     pub(crate) published: u64,
     pub(crate) published_text: String,
     pub(crate) live_now: bool,
-    pub(crate) paid: bool,
     pub(crate) premium: bool,
 }
 
@@ -52,16 +54,18 @@ const BY_UPLOAD_DATE: &SearchSortBy = "upload_date";
 #[allow(unused_variables)]
 const BY_VIEW_COUNT: &SearchSortBy = "view_count";
 
-#[derive(Debug, Clone)]
+#[derive(Clone)]
 pub(crate) struct Client {
-    client: Arc<invidious::asynchronous::Client>,
+    client: Arc<ClientAsync>,
 }
 
 impl Client {
     pub(crate) async fn new() -> Self {
-        // TODO check for a stable instance
-        let client =
-            invidious::asynchronous::Client::new(String::from("https://inv.bp.projectsegfau.lt"));
+        // TODO check for a stable instance, or rotate between a pool of stable ones
+        let client = ClientAsync::new(
+            String::from("https://inv.bp.projectsegfau.lt"),
+            MethodAsync::default(),
+        );
 
         Client {
             client: Arc::new(client),
@@ -71,7 +75,7 @@ impl Client {
     #[allow(dead_code)]
     #[allow(unused_variables)]
     #[cfg(test)]
-    pub(crate) fn new_with_dependencies(client: invidious::asynchronous::Client) -> Self {
+    pub(crate) fn new_with_dependencies(client: invidious::ClientAsync) -> Self {
         Client {
             client: Arc::new(client),
         }
@@ -99,44 +103,31 @@ impl SearchableClient for Client {
         let videos: Vec<Video> = search
             .items
             .iter()
-            .map(|search_result| match search_result {
-                invidious::structs::hidden::SearchItem::Video {
-                    title,
-                    videoId,
-                    author,
-                    authorId,
-                    authorUrl,
-                    lengthSeconds,
-                    videoThumbnails: _,
-                    description,
-                    descriptionHtml,
-                    viewCount,
-                    published,
-                    publishedText,
-                    liveNow,
-                    paid,
-                    premium,
-                } => Some(Video {
-                    title: title.clone(),
-                    video_id: videoId.clone(),
-                    author: author.clone(),
-                    author_id: authorId.clone(),
-                    author_url: authorUrl.clone(),
-                    length_seconds: lengthSeconds.clone(),
-                    description: description.clone(),
-                    description_html: descriptionHtml.clone(),
-                    view_count: viewCount.clone(),
-                    published: published.clone(),
-                    published_text: publishedText.clone(),
-                    live_now: liveNow.clone(),
-                    paid: paid.clone(),
-                    premium: premium.clone(),
-                }),
-                _ => None,
+            .map(|search_result| {
+                let video = match search_result {
+                    SearchItem::Video(item) => Some(Video {
+                        title: item.title.clone(),
+                        video_id: item.id.clone(),
+                        author: item.author.clone(),
+                        author_id: item.author_id.clone(),
+                        author_url: item.author_url.clone(),
+                        length_seconds: item.length.clone() as u64,
+                        description: item.description.clone(),
+                        description_html: item.description_html.clone(),
+                        view_count: item.views.clone(),
+                        published: item.published.clone(),
+                        published_text: item.published_text.clone(),
+                        live_now: item.live.clone(),
+                        premium: item.premium.clone(),
+                    }),
+                    _ => None,
+                };
+                video
             })
             .filter(|x| x.is_some())
             .map(|x| x.unwrap())
-            .filter(|x| !x.premium && !x.paid && !x.live_now)
+            .filter(|x| !x.premium && !x.live_now)
+            .sorted_by(|a, b| b.view_count.cmp(&a.view_count))
             .collect();
 
         Result::Ok(VideoSearch { items: videos })
@@ -149,13 +140,17 @@ mod tests {
 
     #[test]
     fn should_search_properly() {
+        // FIXME: the real solution for properly testing this piece of code would be to spin up a Mocked Invidious
+        // instance (i.e. mock-server) and mock the server responses. With the current implementation, the tests
+        // will always be flaky.
         let client = tokio_test::block_on(Client::new());
 
-        let result = tokio_test::block_on(client.search_video(
-            "vfdskvnfdsjklvnfdsjklvnfsdjkldsvmdlfmvkdfslvsdfmklsdvlvfdnjkvnfdsjkvnfsdjk",
-            Some(BY_UPLOAD_DATE),
-        ))
+        let result = tokio_test::block_on(
+            client.search_video("Eminem - Lose Yourself", Some(BY_UPLOAD_DATE)),
+        )
         .unwrap();
-        assert_eq!(30, result.items.len())
+        assert_eq!(19, result.items.len());
+        // Just check the first one for now - in the future we can check for the whole array
+        assert_eq!("Eminem - Lose Yourself [HD]", result.items[0].title);
     }
 }