From d912ce069cf2996e2b78618a574a89d82ded205e Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 11 Nov 2024 15:04:20 +0000 Subject: [PATCH] zulip: reduce code duplication --- src/schema.rs | 209 +++++++++++++++++++++------------------------- src/static_api.rs | 16 ++-- src/validate.rs | 90 +++++++++++++++++--- 3 files changed, 180 insertions(+), 135 deletions(-) diff --git a/src/schema.rs b/src/schema.rs index 1870005e3..0f4890528 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -370,6 +370,49 @@ impl Team { Ok(lists) } + fn expand_zulip_membership( + &self, + data: &Data, + common: &RawZulipCommon, + on_exclude_not_included: impl Fn(&str) -> Error, + ) -> Result, Error> { + let mut members = if common.include_team_members { + self.members(data)? + } else { + HashSet::new() + }; + for person in &common.extra_people { + members.insert(person.as_str()); + } + for team in &common.extra_teams { + let team = data + .team(team) + .ok_or_else(|| format_err!("team {} is missing", team))?; + members.extend(team.members(data)?); + } + for excluded in &common.excluded_people { + if !members.remove(excluded.as_str()) { + return Err(on_exclude_not_included(excluded)); + } + } + + let mut final_members = Vec::new(); + for member in members.iter() { + let member = data + .person(member) + .ok_or_else(|| format_err!("{} does not have a person configuration", member))?; + let member = match (member.github.clone(), member.zulip_id) { + (github, Some(zulip_id)) => ZulipMember::MemberWithId { github, zulip_id }, + (github, _) => ZulipMember::MemberWithoutId { github }, + }; + final_members.push(member); + } + for &extra in &common.extra_zulip_ids { + final_members.push(ZulipMember::JustId(extra)); + } + Ok(final_members) + } + pub(crate) fn raw_zulip_groups(&self) -> &[RawZulipGroup] { &self.zulip_groups } @@ -379,46 +422,17 @@ impl Team { let zulip_groups = &self.zulip_groups; for raw_group in zulip_groups { - let mut group = ZulipGroup { - name: raw_group.name.clone(), - includes_team_members: raw_group.include_team_members, - members: Vec::new(), - }; - - let mut members = if raw_group.include_team_members { - self.members(data)? - } else { - HashSet::new() - }; - for person in &raw_group.extra_people { - members.insert(person.as_str()); - } - for team in &raw_group.extra_teams { - let team = data - .team(team) - .ok_or_else(|| format_err!("team {} is missing", team))?; - members.extend(team.members(data)?); - } - for excluded in &raw_group.excluded_people { - if !members.remove(excluded.as_str()) { - bail!("'{excluded}' was specifically excluded from the Zulip group '{}' but they were already not included", raw_group.name); - } - } - - for member in members.iter() { - let member = data.person(member).ok_or_else(|| { - format_err!("{} does not have a person configuration", member) - })?; - let member = match (member.github.clone(), member.zulip_id) { - (github, Some(zulip_id)) => ZulipGroupMember::MemberWithId { github, zulip_id }, - (github, _) => ZulipGroupMember::MemberWithoutId { github }, - }; - group.members.push(member); - } - for &extra in &raw_group.extra_zulip_ids { - group.members.push(ZulipGroupMember::JustId(extra)); - } - groups.push(group); + groups.push(ZulipGroup(ZulipCommon { + name: raw_group.common.name.clone(), + includes_team_members: raw_group.common.include_team_members, + members: self.expand_zulip_membership( + data, + &raw_group.common, + |excluded| { + format_err!("'{excluded}' was specifically excluded from the Zulip group '{}' but they were already not included", raw_group.common.name) + }, + )?, + })); } Ok(groups) } @@ -432,47 +446,17 @@ impl Team { let zulip_streams = self.raw_zulip_streams(); for raw_stream in zulip_streams { - let mut stream = ZulipStream { - name: raw_stream.name.clone(), - members: Vec::new(), - }; - - let mut members = if raw_stream.include_team_members { - self.members(data)? - } else { - HashSet::new() - }; - for person in &raw_stream.extra_people { - members.insert(person.as_str()); - } - for team in &raw_stream.extra_teams { - let team = data - .team(team) - .ok_or_else(|| format_err!("team {} is missing", team))?; - members.extend(team.members(data)?); - } - for excluded in &raw_stream.excluded_people { - if !members.remove(excluded.as_str()) { - bail!("'{excluded}' was specifically excluded from the Zulip stream '{}' but they were already not included", raw_stream.name); - } - } - - for member in members.iter() { - let member = data.person(member).ok_or_else(|| { - format_err!("{} does not have a person configuration", member) - })?; - let member = match (member.github.clone(), member.zulip_id) { - (github, Some(zulip_id)) => { - ZulipStreamMember::MemberWithId { github, zulip_id } - } - (github, _) => ZulipStreamMember::MemberWithoutId { github }, - }; - stream.members.push(member); - } - for &extra in &raw_stream.extra_zulip_ids { - stream.members.push(ZulipStreamMember::JustId(extra)); - } - streams.push(stream); + streams.push(ZulipStream(ZulipCommon { + name: raw_stream.common.name.clone(), + includes_team_members: raw_stream.common.include_team_members, + members: self.expand_zulip_membership( + data, + &raw_stream.common, + |excluded| { + format_err!("'{excluded}' was specifically excluded from the Zulip stream '{}' but they were already not included", raw_stream.common.name) + }, + )?, + })); } Ok(streams) } @@ -733,7 +717,7 @@ pub(crate) struct TeamList { #[derive(serde_derive::Deserialize, Debug)] #[serde(rename_all = "kebab-case", deny_unknown_fields)] -pub(crate) struct RawZulipGroup { +pub(crate) struct RawZulipCommon { pub(crate) name: String, #[serde(default = "default_true")] pub(crate) include_team_members: bool, @@ -747,20 +731,18 @@ pub(crate) struct RawZulipGroup { pub(crate) excluded_people: Vec, } +#[derive(serde_derive::Deserialize, Debug)] +#[serde(rename_all = "kebab-case", deny_unknown_fields)] +pub(crate) struct RawZulipGroup { + #[serde(flatten)] + pub(crate) common: RawZulipCommon, +} + #[derive(serde_derive::Deserialize, Debug)] #[serde(rename_all = "kebab-case", deny_unknown_fields)] pub(crate) struct RawZulipStream { - pub(crate) name: String, - #[serde(default = "default_true")] - pub(crate) include_team_members: bool, - #[serde(default)] - pub(crate) extra_people: Vec, - #[serde(default)] - pub(crate) extra_zulip_ids: Vec, - #[serde(default)] - pub(crate) extra_teams: Vec, - #[serde(default)] - pub(crate) excluded_people: Vec, + #[serde(flatten)] + pub(crate) common: RawZulipCommon, } #[derive(Debug)] @@ -780,52 +762,49 @@ impl List { } #[derive(Debug)] -pub(crate) struct ZulipGroup { +pub(crate) struct ZulipCommon { name: String, includes_team_members: bool, - members: Vec, + members: Vec, } -impl ZulipGroup { +impl ZulipCommon { pub(crate) fn name(&self) -> &str { &self.name } - /// Whether the group includes the members of the team its associated + /// Whether the group/stream includes the members of the associated team? pub(crate) fn includes_team_members(&self) -> bool { self.includes_team_members } - pub(crate) fn members(&self) -> &[ZulipGroupMember] { + pub(crate) fn members(&self) -> &[ZulipMember] { &self.members } } -#[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] -pub(crate) enum ZulipGroupMember { - MemberWithId { github: String, zulip_id: u64 }, - JustId(u64), - MemberWithoutId { github: String }, -} - #[derive(Debug)] -pub(crate) struct ZulipStream { - name: String, - members: Vec, -} +pub(crate) struct ZulipGroup(ZulipCommon); -impl ZulipStream { - pub(crate) fn name(&self) -> &str { - &self.name +impl std::ops::Deref for ZulipGroup { + type Target = ZulipCommon; + fn deref(&self) -> &Self::Target { + &self.0 } +} - pub(crate) fn members(&self) -> &[ZulipStreamMember] { - &self.members +#[derive(Debug)] +pub(crate) struct ZulipStream(ZulipCommon); + +impl std::ops::Deref for ZulipStream { + type Target = ZulipCommon; + fn deref(&self) -> &Self::Target { + &self.0 } } #[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] -pub(crate) enum ZulipStreamMember { +pub(crate) enum ZulipMember { MemberWithId { github: String, zulip_id: u64 }, JustId(u64), MemberWithoutId { github: String }, diff --git a/src/static_api.rs b/src/static_api.rs index 82efcd500..69c51b6f7 100644 --- a/src/static_api.rs +++ b/src/static_api.rs @@ -1,7 +1,5 @@ use crate::data::Data; -use crate::schema::{ - Bot, Email, Permissions, RepoPermission, TeamKind, ZulipGroupMember, ZulipStreamMember, -}; +use crate::schema::{Bot, Email, Permissions, RepoPermission, TeamKind, ZulipMember}; use anyhow::{ensure, Context as _, Error}; use indexmap::IndexMap; use log::info; @@ -279,13 +277,13 @@ impl<'a> Generator<'a> { members: members .into_iter() .filter_map(|m| match m { - ZulipGroupMember::MemberWithId { zulip_id, .. } => { + ZulipMember::MemberWithId { zulip_id, .. } => { Some(v1::ZulipGroupMember::Id(zulip_id)) } - ZulipGroupMember::JustId(zulip_id) => { + ZulipMember::JustId(zulip_id) => { Some(v1::ZulipGroupMember::Id(zulip_id)) } - ZulipGroupMember::MemberWithoutId { .. } => None, + ZulipMember::MemberWithoutId { .. } => None, }) .collect(), }, @@ -310,13 +308,13 @@ impl<'a> Generator<'a> { members: members .into_iter() .filter_map(|m| match m { - ZulipStreamMember::MemberWithId { zulip_id, .. } => { + ZulipMember::MemberWithId { zulip_id, .. } => { Some(v1::ZulipStreamMember::Id(zulip_id)) } - ZulipStreamMember::JustId(zulip_id) => { + ZulipMember::JustId(zulip_id) => { Some(v1::ZulipStreamMember::Id(zulip_id)) } - ZulipStreamMember::MemberWithoutId { .. } => None, + ZulipMember::MemberWithoutId { .. } => None, }) .collect(), }, diff --git a/src/validate.rs b/src/validate.rs index 86196d43b..0b938a844 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -1,6 +1,6 @@ use crate::data::Data; use crate::github::GitHubApi; -use crate::schema::{Bot, Email, Permissions, Team, TeamKind, TeamPeople, ZulipGroupMember}; +use crate::schema::{Bot, Email, Permissions, Team, TeamKind, TeamPeople, ZulipMember}; use crate::zulip::ZulipApi; use anyhow::{bail, Error}; use log::{error, warn}; @@ -45,6 +45,9 @@ static CHECKS: &[Check)>] = checks![ validate_unique_zulip_groups, validate_zulip_group_ids, validate_zulip_group_extra_people, + validate_unique_zulip_streams, + validate_zulip_stream_ids, + validate_zulip_stream_extra_people, validate_repos, validate_branch_protections, validate_member_roles, @@ -333,9 +336,10 @@ fn validate_inactive_members(data: &Data, errors: &mut Vec) { .values() .flat_map(|z| z.members()) .filter_map(|m| match m { - ZulipGroupMember::MemberWithId { github, .. } - | ZulipGroupMember::MemberWithoutId { github } => Some(github.as_str()), - ZulipGroupMember::JustId(_) => None, + ZulipMember::MemberWithId { github, .. } | ZulipMember::MemberWithoutId { github } => { + Some(github.as_str()) + } + ZulipMember::JustId(_) => None, }) .collect::>(); wrapper( @@ -686,15 +690,13 @@ fn validate_zulip_users(data: &Data, zulip: &ZulipApi, errors: &mut Vec) .members() .iter() .filter_map(|m| match m { - ZulipGroupMember::MemberWithId { github, zulip_id } - if !by_id.contains(zulip_id) => - { + ZulipMember::MemberWithId { github, zulip_id } if !by_id.contains(zulip_id) => { Some(github.clone()) } - ZulipGroupMember::JustId(zulip_id) if !by_id.contains(zulip_id) => { + ZulipMember::JustId(zulip_id) if !by_id.contains(zulip_id) => { Some(format!("ID: {zulip_id}")) } - ZulipGroupMember::MemberWithoutId { github } => Some(github.clone()), + ZulipMember::MemberWithoutId { github } => Some(github.clone()), _ => None, }) .collect::>(); @@ -733,6 +735,30 @@ fn validate_zulip_group_ids(data: &Data, errors: &mut Vec) { }); } +/// Ensure every member of a team that has a Zulip stream either has a Zulip id +fn validate_zulip_stream_ids(data: &Data, errors: &mut Vec) { + wrapper(data.teams(), errors, |team, errors| { + let streams = team.zulip_streams(data)?; + // Returns if stream is empty or all the groups don't include the team members + if streams.is_empty() || streams.iter().all(|s| !s.includes_team_members()) { + return Ok(()); + } + wrapper(team.members(data)?.iter(), errors, |member, _| { + if let Some(member) = data.person(member) { + if member.zulip_id().is_none() { + bail!( + "person `{}` in '{}' is a member of a Zulip stream but has no Zulip id", + member.github(), + team.name() + ); + } + } + Ok(()) + }); + Ok(()) + }); +} + /// Ensure there is at most one definition for any given Zulip group fn validate_unique_zulip_groups(data: &Data, errors: &mut Vec) { let mut groups = HashMap::new(); @@ -756,16 +782,58 @@ fn validate_unique_zulip_groups(data: &Data, errors: &mut Vec) { }); } +/// Ensure there is at most one definition for any given Zulip group +fn validate_unique_zulip_streams(data: &Data, errors: &mut Vec) { + let mut streams = HashMap::new(); + wrapper(data.teams(), errors, |team, errors| { + wrapper( + team.zulip_streams(data).iter().flatten(), + errors, + |stream, _| { + if let Some(other_team) = streams.insert(stream.name().to_owned(), team.name()) { + bail!( + "the Zulip stream `{}` is defined in both `{}` and `{}` team definitions", + stream.name(), + team.name(), + other_team + ); + } + Ok(()) + }, + ); + Ok(()) + }); +} + /// Ensure members of extra-people in a Zulip user group are real people fn validate_zulip_group_extra_people(data: &Data, errors: &mut Vec) { wrapper(data.teams(), errors, |team, errors| { wrapper(team.raw_zulip_groups().iter(), errors, |group, _| { - for person in &group.extra_people { + for person in &group.common.extra_people { if data.person(person).is_none() { bail!( "person `{}` does not exist (in Zulip group `{}`)", person, - group.name + group.common.name + ); + } + } + Ok(()) + }); + Ok(()) + }); +} + +/// Ensure members of extra-people in a Zulip user group are real people +fn validate_zulip_stream_extra_people(data: &Data, errors: &mut Vec) { + wrapper(data.teams(), errors, |team, errors| { + wrapper(team.raw_zulip_streams().iter(), errors, |stream, _| { + for person in &stream.common.extra_people { + if data.person(person).is_none() { + bail!( + "person `{}` does not exist (in Zulip stream `{}`)", + person, + stream.common.name ); } }