diff --git a/sync-team/src/github/api/mod.rs b/sync-team/src/github/api/mod.rs index f3b6d4830..6dbe3a114 100644 --- a/sync-team/src/github/api/mod.rs +++ b/sync-team/src/github/api/mod.rs @@ -92,7 +92,46 @@ impl HttpClient { } } + /// Send a request to the GitHub API and return the response. fn graphql(&self, query: &str, variables: V, org: &str) -> anyhow::Result + where + R: serde::de::DeserializeOwned, + V: serde::Serialize, + { + let res = self.send_graphql_req(query, variables, org)?; + + if let Some(error) = res.errors.first() { + bail!("graphql error: {}", error.message); + } + + read_graphql_data(res) + } + + /// Send a request to the GitHub API and return the response. + /// If the request contains the error type `NOT_FOUND`, this method returns `Ok(None)`. + fn graphql_opt(&self, query: &str, variables: V, org: &str) -> anyhow::Result> + where + R: serde::de::DeserializeOwned, + V: serde::Serialize, + { + let res = self.send_graphql_req(query, variables, org)?; + + if let Some(error) = res.errors.first() { + if error.type_ == Some(GraphErrorType::NotFound) { + return Ok(None); + } + bail!("graphql error: {}", error.message); + } + + read_graphql_data(res) + } + + fn send_graphql_req( + &self, + query: &str, + variables: V, + org: &str, + ) -> anyhow::Result> where R: serde::de::DeserializeOwned, V: serde::Serialize, @@ -105,19 +144,13 @@ impl HttpClient { let resp = self .req(Method::POST, &GitHubUrl::new("graphql", org))? .json(&Request { query, variables }) - .send()? + .send() + .context("failed to send graphql request")? .custom_error_for_status()?; - let res: GraphResult = resp.json_annotated().with_context(|| { + resp.json_annotated().with_context(|| { format!("Failed to decode response body on graphql request with query '{query}'") - })?; - if let Some(error) = res.errors.first() { - bail!("graphql error: {}", error.message); - } else if let Some(data) = res.data { - Ok(data) - } else { - bail!("missing graphql data"); - } + }) } fn rest_paginated(&self, method: &Method, url: &GitHubUrl, mut f: F) -> anyhow::Result<()> @@ -159,6 +192,17 @@ impl HttpClient { } } +fn read_graphql_data(res: GraphResult) -> anyhow::Result +where + R: serde::de::DeserializeOwned, +{ + if let Some(data) = res.data { + Ok(data) + } else { + bail!("missing graphql data"); + } +} + fn allow_not_found(resp: Response, method: Method, url: &str) -> Result<(), anyhow::Error> { match resp.status() { StatusCode::NOT_FOUND => { @@ -180,9 +224,19 @@ struct GraphResult { #[derive(Debug, serde::Deserialize)] struct GraphError { + #[serde(rename = "type")] + type_: Option, message: String, } +#[derive(Debug, serde::Deserialize, PartialEq, Eq)] +#[serde(rename_all = "SCREAMING_SNAKE_CASE")] +enum GraphErrorType { + NotFound, + #[serde(other)] + Other, +} + #[derive(serde::Deserialize)] struct GraphNodes { nodes: Vec>, diff --git a/sync-team/src/github/api/read.rs b/sync-team/src/github/api/read.rs index 19737a27c..fd82880a8 100644 --- a/sync-team/src/github/api/read.rs +++ b/sync-team/src/github/api/read.rs @@ -2,6 +2,7 @@ use crate::github::api::{ BranchProtection, GraphNode, GraphNodes, GraphPageInfo, HttpClient, Login, Repo, RepoTeam, RepoUser, Team, TeamMember, TeamRole, team_node_id, url::GitHubUrl, user_node_id, }; +use anyhow::Context as _; use reqwest::Method; use std::collections::{HashMap, HashSet}; @@ -271,16 +272,19 @@ impl GithubRead for GitHubApiRead { is_archived: bool, } - let result: Wrapper = self.client.graphql( - QUERY, - Params { - owner: org, - name: repo, - }, - org, - )?; + let result: Option = self + .client + .graphql_opt( + QUERY, + Params { + owner: org, + name: repo, + }, + org, + ) + .with_context(|| format!("failed to retrieve repo `{org}/{repo}`"))?; - let repo = result.repository.map(|repo_response| Repo { + let repo = result.and_then(|r| r.repository).map(|repo_response| Repo { node_id: repo_response.id, name: repo.to_string(), description: repo_response.description.unwrap_or_default(), diff --git a/sync-team/src/github/tests/mod.rs b/sync-team/src/github/tests/mod.rs index ce5f7cc0d..f74a8ae70 100644 --- a/sync-team/src/github/tests/mod.rs +++ b/sync-team/src/github/tests/mod.rs @@ -1,4 +1,6 @@ -use crate::github::tests::test_utils::{BranchProtectionBuilder, DataModel, RepoData, TeamData}; +use crate::github::tests::test_utils::{ + BranchProtectionBuilder, DEFAULT_ORG, DataModel, RepoData, TeamData, +}; use rust_team_data::v1::{BranchProtectionMode, RepoPermission}; mod test_utils; @@ -17,7 +19,7 @@ fn team_create() { let user = model.create_user("mark"); let user2 = model.create_user("jan"); let gh = model.gh_model(); - model.create_team(TeamData::new("admins").gh_team("admins-gh", &[user, user2])); + model.create_team(TeamData::new("admins").gh_team(DEFAULT_ORG, "admins-gh", &[user, user2])); let team_diff = model.diff_teams(gh); insta::assert_debug_snapshot!(team_diff, @r###" [ @@ -48,7 +50,7 @@ fn team_add_member() { let mut model = DataModel::default(); let user = model.create_user("mark"); let user2 = model.create_user("jan"); - model.create_team(TeamData::new("admins").gh_team("admins-gh", &[user])); + model.create_team(TeamData::new("admins").gh_team(DEFAULT_ORG, "admins-gh", &[user])); let gh = model.gh_model(); model.get_team("admins").add_gh_member("admins-gh", user2); @@ -85,11 +87,11 @@ fn team_dont_add_member_if_invitation_is_pending() { let mut model = DataModel::default(); let user = model.create_user("mark"); let user2 = model.create_user("jan"); - model.create_team(TeamData::new("admins").gh_team("admins-gh", &[user])); + model.create_team(TeamData::new("admins").gh_team(DEFAULT_ORG, "admins-gh", &[user])); let mut gh = model.gh_model(); model.get_team("admins").add_gh_member("admins-gh", user2); - gh.add_invitation("admins-gh", "jan"); + gh.add_invitation(DEFAULT_ORG, "admins-gh", "jan"); let team_diff = model.diff_teams(gh); insta::assert_debug_snapshot!(team_diff, @"[]"); @@ -100,7 +102,7 @@ fn team_remove_member() { let mut model = DataModel::default(); let user = model.create_user("mark"); let user2 = model.create_user("jan"); - model.create_team(TeamData::new("admins").gh_team("admins-gh", &[user, user2])); + model.create_team(TeamData::new("admins").gh_team(DEFAULT_ORG, "admins-gh", &[user, user2])); let gh = model.gh_model(); model @@ -142,8 +144,8 @@ fn team_delete() { // won't be generated, because no organization is known to scan for existing unmanaged teams. model.create_team( TeamData::new("admins") - .gh_team("admins-gh", &[user]) - .gh_team("users-gh", &[user]), + .gh_team(DEFAULT_ORG, "admins-gh", &[user]) + .gh_team(DEFAULT_ORG, "users-gh", &[user]), ); let gh = model.gh_model(); diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index 6db75cf8c..697e14559 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeSet, HashMap, HashSet}; use derive_builder::Builder; use rust_team_data::v1; @@ -13,7 +13,7 @@ use crate::github::{ RepoDiff, SyncGitHub, TeamDiff, api, construct_branch_protection, convert_permission, }; -const DEFAULT_ORG: &str = "rust-lang"; +pub const DEFAULT_ORG: &str = "rust-lang"; type UserId = u64; @@ -74,11 +74,12 @@ impl DataModel { .map(|user| (user.github_id, user.name.clone())) .collect(); - let mut team_memberships: HashMap> = HashMap::default(); - let mut teams = vec![]; + let mut orgs: HashMap = HashMap::default(); + for team in &self.teams { for gh_team in &team.gh_teams { - let res = team_memberships.insert( + let org = orgs.entry(gh_team.org.clone()).or_default(); + let res = org.team_memberships.insert( gh_team.name.clone(), gh_team .members @@ -96,27 +97,26 @@ impl DataModel { ); assert!(res.is_none()); - teams.push(api::Team { - id: Some(teams.len() as u64), + org.teams.push(api::Team { + id: Some(org.teams.len() as u64), name: gh_team.name.clone(), description: Some("Managed by the rust-lang/team repository.".to_string()), privacy: TeamPrivacy::Closed, slug: gh_team.name.clone(), - }) + }); + + org.members.extend(gh_team.members.iter().copied()); } } - let mut repos = HashMap::default(); - let mut repo_members: HashMap = HashMap::default(); - let mut branch_protections = HashMap::new(); - for repo in &self.repos { - repos.insert( + let org = orgs.entry(repo.org.clone()).or_default(); + org.repos.insert( repo.name.clone(), Repo { - node_id: repos.len().to_string(), + node_id: org.repos.len().to_string(), name: repo.name.clone(), - org: DEFAULT_ORG.to_string(), + org: repo.org.clone(), description: repo.description.clone(), homepage: repo.homepage.clone(), archived: false, @@ -146,7 +146,8 @@ impl DataModel { permission: convert_permission(&m.permission), }) .collect(); - repo_members.insert(repo.name.clone(), RepoMembers { teams, members }); + org.repo_members + .insert(repo.name.clone(), RepoMembers { teams, members }); let repo_v1: v1::Repo = repo.clone().into(); let mut protections = vec![]; @@ -156,19 +157,15 @@ impl DataModel { construct_branch_protection(&repo_v1, protection), )); } - branch_protections.insert(repo.name.clone(), protections); + org.branch_protections + .insert(repo.name.clone(), protections); } - GithubMock { - users, - owners: Default::default(), - teams, - team_memberships, - team_invitations: Default::default(), - repos, - repo_members, - branch_protections, + if orgs.is_empty() { + orgs.insert(DEFAULT_ORG.to_string(), GithubOrg::default()); } + + GithubMock { users, orgs } } pub fn diff_teams(&self, github: GithubMock) -> Vec { @@ -249,10 +246,10 @@ impl From for v1::Team { } impl TeamDataBuilder { - pub fn gh_team(mut self, name: &str, members: &[UserId]) -> Self { + pub fn gh_team(mut self, org: &str, name: &str, members: &[UserId]) -> Self { let mut gh_teams = self.gh_teams.unwrap_or_default(); gh_teams.push(GitHubTeam { - org: DEFAULT_ORG.to_string(), + org: org.to_string(), name: name.to_string(), members: members.to_vec(), }); @@ -265,6 +262,8 @@ impl TeamDataBuilder { #[builder(pattern = "owned")] pub struct RepoData { name: String, + #[builder(default = DEFAULT_ORG.to_string())] + org: String, #[builder(default)] pub description: String, #[builder(default)] @@ -307,6 +306,7 @@ impl From for v1::Repo { fn from(value: RepoData) -> Self { let RepoData { name, + org, description, homepage, bots, @@ -317,7 +317,7 @@ impl From for v1::Repo { branch_protections, } = value; Self { - org: DEFAULT_ORG.to_string(), + org, name: name.clone(), description, homepage, @@ -411,28 +411,30 @@ impl BranchProtectionBuilder { pub struct GithubMock { // user ID -> login users: HashMap, - // org name -> user ID - owners: HashMap>, - teams: Vec, - // Team name -> members - team_memberships: HashMap>, - // Team name -> list of invited users - team_invitations: HashMap>, - // Repo name -> repo data - repos: HashMap, - // Repo name -> (teams, members) - repo_members: HashMap, - // Repo name -> Vec<(protection ID, branch protection)> - branch_protections: HashMap>, + // org name -> organization data + orgs: HashMap, } impl GithubMock { - pub fn add_invitation(&mut self, repo: &str, user: &str) { - self.team_invitations + pub fn add_invitation(&mut self, org: &str, repo: &str, user: &str) { + self.get_org_mut(org) + .team_invitations .entry(repo.to_string()) .or_default() .push(user.to_string()); } + + fn get_org(&self, org: &str) -> &GithubOrg { + self.orgs + .get(org) + .unwrap_or_else(|| panic!("Org {org} not found")) + } + + fn get_org_mut(&mut self, org: &str) -> &mut GithubOrg { + self.orgs + .get_mut(org) + .unwrap_or_else(|| panic!("Org {org} not found")) + } } impl GithubRead for GithubMock { @@ -446,39 +448,38 @@ impl GithubRead for GithubMock { } fn org_owners(&self, org: &str) -> anyhow::Result> { - Ok(self - .owners - .get(org) - .cloned() - .unwrap_or_default() - .into_iter() - .collect()) + Ok(self.get_org(org).owners.iter().copied().collect()) } fn org_teams(&self, org: &str) -> anyhow::Result> { - assert_eq!(org, DEFAULT_ORG); Ok(self + .get_org(org) .teams .iter() .map(|team| (team.name.clone(), team.slug.clone())) .collect()) } - fn team(&self, _org: &str, team: &str) -> anyhow::Result> { - Ok(self.teams.iter().find(|t| t.name == team).cloned()) + fn team(&self, org: &str, team: &str) -> anyhow::Result> { + Ok(self + .get_org(org) + .teams + .iter() + .find(|t| t.name == team) + .cloned()) } fn team_memberships( &self, team: &Team, - _org: &str, + org: &str, ) -> anyhow::Result> { - let memberships = self + Ok(self + .get_org(org) .team_memberships .get(&team.name) .cloned() - .unwrap_or_default(); - Ok(memberships) + .unwrap_or_default()) } fn team_membership_invitations( @@ -486,8 +487,8 @@ impl GithubRead for GithubMock { org: &str, team: &str, ) -> anyhow::Result> { - assert_eq!(org, DEFAULT_ORG); Ok(self + .get_org(org) .team_invitations .get(team) .cloned() @@ -497,13 +498,15 @@ impl GithubRead for GithubMock { } fn repo(&self, org: &str, repo: &str) -> anyhow::Result> { - assert_eq!(org, DEFAULT_ORG); - Ok(self.repos.get(repo).cloned()) + Ok(self + .orgs + .get(org) + .and_then(|org| org.repos.get(repo).cloned())) } fn repo_teams(&self, org: &str, repo: &str) -> anyhow::Result> { - assert_eq!(org, DEFAULT_ORG); Ok(self + .get_org(org) .repo_members .get(repo) .cloned() @@ -512,9 +515,8 @@ impl GithubRead for GithubMock { } fn repo_collaborators(&self, org: &str, repo: &str) -> anyhow::Result> { - // The mock currently only supports mocking one GitHub organization. - assert_eq!(org, DEFAULT_ORG); Ok(self + .get_org(org) .repo_members .get(repo) .cloned() @@ -527,9 +529,7 @@ impl GithubRead for GithubMock { org: &str, repo: &str, ) -> anyhow::Result> { - assert_eq!(org, DEFAULT_ORG); - - let Some(protections) = self.branch_protections.get(repo) else { + let Some(protections) = self.get_org(org).branch_protections.get(repo) else { return Ok(Default::default()); }; let mut result = HashMap::default(); @@ -541,6 +541,23 @@ impl GithubRead for GithubMock { } } +#[derive(Default)] +struct GithubOrg { + members: BTreeSet, + owners: BTreeSet, + teams: Vec, + // Team name -> list of invited users + team_invitations: HashMap>, + // Team name -> members + team_memberships: HashMap>, + // Repo name -> repo data + repos: HashMap, + // Repo name -> (teams, members) + repo_members: HashMap, + // Repo name -> Vec<(protection ID, branch protection)> + branch_protections: HashMap>, +} + #[derive(Clone)] pub struct RepoMembers { teams: Vec, diff --git a/sync-team/src/utils.rs b/sync-team/src/utils.rs index 71708d9cb..6804638ae 100644 --- a/sync-team/src/utils.rs +++ b/sync-team/src/utils.rs @@ -13,8 +13,8 @@ impl ResponseExt for Response { match self.error_for_status_ref() { Ok(_) => Ok(self), Err(err) => { - let body = self.text()?; - Err(err).context(format!("Body: {:?}", body)) + let body = self.text().context("failed to read response body")?; + Err(err).context(format!("Body: {body:?}")) } } }