Skip to content

src: lore_api_client: use color_eyre::Result #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 10 additions & 16 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@ use std::{collections::HashMap, path::Path, process::Command};
use color_eyre::eyre::bail;
use config::Config;
use patch_hub::{
lore_session::{
lore_api_client::{BlockingLoreAPIClient, FailedFeedRequest}, lore_session::{
self, LoreSession
},
lore_api_client::{
BlockingLoreAPIClient, FailedFeedRequest
},
mailing_list::MailingList,
patch::Patch
}, mailing_list::MailingList, patch::Patch
};

mod config;
Expand Down Expand Up @@ -74,15 +69,14 @@ impl LatestPatchsetsState {
}

pub fn fetch_current_page(self: &mut Self) -> color_eyre::Result<()> {
if let Err(failed_feed_request) = self
.lore_session.process_n_representative_patches(&self.lore_api_client, self.page_size * &self.page_number) {
match failed_feed_request {
FailedFeedRequest::UnknownError(error) => bail!("[FailedFeedRequest::UnknownError]\n*\tFailed to request feed\n*\t{error:#?}"),
FailedFeedRequest::StatusNotOk(feed_response) => bail!("[FailedFeedRequest::StatusNotOk]\n*\tRequest returned with non-OK status\n*\t{feed_response:#?}"),
FailedFeedRequest::EndOfFeed => (),
}
Comment on lines -79 to -83
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention in our offline conversation that it is important for the caller to handle the variants of FailedFeedRequest. Especially EndOfFeed, which is an edge case when we navigate through the whole history of a list. Below, is an explanation of why we need to match here, but my bad on my end for not explaining this earlier... Try accessing a small list (I found the ath12k to have ~1500 patchsets) and navigate to the end of the feed to see the panic happening (it may take a while without the VPN stuff we discussed).

Explanation:

Because of logic on the LoreSession type, we make subsequent fetches until we hit the number of patchsets we want, so if the page size is 30 and there are 45 patchsets total on a list when the user prompts for the second page, the program would panic, as it would try to access something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in this case, if the request returns an Err(EndOfFeed), this caller should return an Ok(()) rather than propagating the error

My fault, didn't read the code with enough attention

};
Ok(())
match self.lore_session.process_n_representative_patches(&self.lore_api_client, self.page_size * self.page_number) {
Ok(_) => Ok(()),
Err(report) => match report.downcast::<FailedFeedRequest>() {
Ok(FailedFeedRequest::EndOfFeed) => Ok(()),
Ok(other) => bail!(other),
Err(err) => Err(err),
},
}
}

pub fn select_below_patchset(self: &mut Self) {
Expand Down
25 changes: 19 additions & 6 deletions src/lore_api_client.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
use std::fmt::Display;

use color_eyre::eyre::{bail, Result};
use reqwest::blocking::Response;
use reqwest::Error;

Expand All @@ -14,6 +17,16 @@ pub enum FailedFeedRequest {
EndOfFeed,
}

impl Display for FailedFeedRequest {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
FailedFeedRequest::UnknownError(error) => write!(f, "Unknown error: {}", error),
FailedFeedRequest::StatusNotOk(response) => write!(f, "Status not OK: {}", response.status()),
FailedFeedRequest::EndOfFeed => write!(f, "End of feed"),
}
}
}

pub struct BlockingLoreAPIClient {}

impl BlockingLoreAPIClient {
Expand All @@ -23,11 +36,11 @@ impl BlockingLoreAPIClient {
}

pub trait PatchFeedRequest {
fn request_patch_feed(self: &Self, target_list: &str, min_index: u32) -> Result<String, FailedFeedRequest>;
fn request_patch_feed(self: &Self, target_list: &str, min_index: u32) -> Result<String>;
}

impl PatchFeedRequest for BlockingLoreAPIClient {
fn request_patch_feed(self: &Self, target_list: &str, min_index: u32) -> Result<String, FailedFeedRequest> {
fn request_patch_feed(self: &Self, target_list: &str, min_index: u32) -> Result<String> {
let feed_request: String;
let feed_response: Response;
let feed_response_body: String;
Expand All @@ -36,17 +49,17 @@ impl PatchFeedRequest for BlockingLoreAPIClient {

match reqwest::blocking::get(feed_request) {
Ok(response) => feed_response = response,
Err(error) => return Err(FailedFeedRequest::UnknownError(error)),
Err(error) => bail!(FailedFeedRequest::UnknownError(error)),
};

match feed_response.status().as_u16() {
200 => (),
_ => return Err(FailedFeedRequest::StatusNotOk(feed_response)),
_ => bail!(FailedFeedRequest::StatusNotOk(feed_response)),
};

feed_response_body = feed_response.text().unwrap();
feed_response_body = feed_response.text()?;
if feed_response_body.eq(r"</feed>") {
return Err(FailedFeedRequest::EndOfFeed);
bail!(FailedFeedRequest::EndOfFeed);
};

Ok(feed_response_body)
Expand Down
10 changes: 6 additions & 4 deletions src/lore_api_client/tests.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use color_eyre::eyre::Context;

use super::*;
use crate::patch::PatchFeed;

Expand All @@ -21,18 +23,18 @@ fn blocking_client_should_detect_failed_patch_feed_request() {
let lore_api_client = BlockingLoreAPIClient::new();

if let Err(failed_feed_request) = lore_api_client.request_patch_feed("invalid-list", 0) {
match failed_feed_request {
match failed_feed_request.downcast::<FailedFeedRequest>().context("Downcasting report to FailedFeedRequest").unwrap() {
FailedFeedRequest::StatusNotOk(_) => (),
_ => panic!("Invalid request should return non 200 OK status.\n{failed_feed_request:#?}")
err => panic!("Invalid request should return non 200 OK status.\n{:#?}", err)
}
} else {
panic!("Invalid request shouldn't be successful");
}

if let Err(failed_feed_request) = lore_api_client.request_patch_feed("amd-gfx", 300000) {
match failed_feed_request {
match failed_feed_request.downcast::<FailedFeedRequest>().context("Downcasting report to FailedFeedRequest").unwrap() {
FailedFeedRequest::EndOfFeed => (),
_ => panic!("Out-of-bounds request should return end of feed.\n{failed_feed_request:#?}")
err => panic!("Out-of-bounds request should return end of feed.\n{err:#?}")
}
} else {
panic!("Out-of-bounds request shouldn't be successful");
Expand Down
7 changes: 4 additions & 3 deletions src/lore_session.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
use crate::mailing_list::MailingList;
use crate::patch::{Patch, PatchFeed, PatchRegex};
use crate::lore_api_client::{
AvailableListsRequest, FailedAvailableListsRequest, FailedFeedRequest, FailedPatchHTMLRequest, PatchFeedRequest, PatchHTMLRequest
AvailableListsRequest, FailedAvailableListsRequest, FailedPatchHTMLRequest, PatchFeedRequest, PatchHTMLRequest
};
use std::collections::HashMap;
use std::mem::swap;
use std::{fs::{self, File}, io};
use std::io::{BufRead, BufReader};
use std::path::Path;
use std::process::{Command, Stdio};
use color_eyre::eyre::{bail, Result};
use regex::Regex;
use serde_xml_rs::from_str;

Expand Down Expand Up @@ -44,14 +45,14 @@ impl LoreSession {
self.processed_patches_map.get(message_id)
}

pub fn process_n_representative_patches<T: PatchFeedRequest>(self: &mut Self, lore_api_client: &T, n: u32) -> Result<(), FailedFeedRequest> {
pub fn process_n_representative_patches<T: PatchFeedRequest>(self: &mut Self, lore_api_client: &T, n: u32) -> Result<()> {
let mut patch_feed: PatchFeed;
let mut processed_patches_ids: Vec<String>;

while self.representative_patches_ids.len() < usize::try_from(n).unwrap() {
match lore_api_client.request_patch_feed(&self.target_list, self.min_index) {
Ok(feed_response_body) => patch_feed = from_str(&feed_response_body).unwrap(),
Err(failed_feed_request) => return Err(failed_feed_request),
Err(failed_feed_request) => bail!(failed_feed_request),
Comment on lines -54 to +55
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about recoverable errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about bailed errors still being recoverable

}

processed_patches_ids = self.process_patches(patch_feed);
Expand Down
2 changes: 1 addition & 1 deletion src/lore_session/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::fs;

struct FakeLoreAPIClient { src_path: String }
impl PatchFeedRequest for FakeLoreAPIClient {
fn request_patch_feed(self: &Self, target_list: &str, min_index: u32) -> Result<String, FailedFeedRequest> {
fn request_patch_feed(self: &Self, target_list: &str, min_index: u32) -> Result<String> {
let _ = min_index;
let _ = target_list;
Ok(fs::read_to_string(&self.src_path).unwrap())
Expand Down