Skip to content

Handle team update scenario where GitHub team has been recreated #11653

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
34 changes: 27 additions & 7 deletions crates/crates_io_database/src/models/team.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,33 @@ impl NewTeam<'_> {
pub async fn create_or_update(&self, conn: &mut AsyncPgConnection) -> QueryResult<Team> {
use diesel::insert_into;

insert_into(teams::table)
.values(self)
.on_conflict(teams::github_id)
.do_update()
.set(self)
.get_result(conn)
.await
// First try to update an existing record with matching org_id and login
let update_result = diesel::update(
teams::table.filter(
teams::org_id
.eq(self.org_id)
.and(teams::login.eq(self.login)),
),
)
.set(self)
.returning(Team::as_returning())
.get_result(conn)
.await;

match update_result {
Ok(team) => Ok(team),
Err(diesel::result::Error::NotFound) => {
// No existing record found, try to insert
insert_into(teams::table)
.values(self)
.on_conflict(teams::github_id)
.do_update()
.set(self)
.get_result(conn)
.await
}
Err(e) => Err(e),
}
}
}

Expand Down
55 changes: 55 additions & 0 deletions src/tests/team.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,61 @@ async fn add_renamed_team() -> anyhow::Result<()> {
Ok(())
}

/// Test adding a team that has the same name as an existing team, but a different ID
/// This would happen if a team was deleted in GitHub and then recreated with the same name.
#[tokio::test(flavor = "multi_thread")]
async fn add_team_with_same_name_different_id() -> anyhow::Result<()> {
let (app, anon) = TestApp::init().empty().await;
let mut conn = app.db_conn().await;
let user = app.db_new_user("user-all-teams").await;
let token = user.db_new_token("arbitrary token name").await;
let owner_id = user.as_model().id;

CrateBuilder::new("foo_same_name_different_id", owner_id)
.expect_build(&mut conn)
.await;

let original_team = NewTeam::builder()
.login("github:test-org:core")
.org_id(1000)
.github_id(2001)
.build()
.create_or_update(&mut conn)
.await?;

let new_team = NewTeam::builder()
// same team name
.login(&original_team.login)
// same org ID
.org_id(original_team.org_id)
// different team ID
.github_id(original_team.github_id + 1)
.build()
.create_or_update(&mut conn)
.await?;
Comment on lines +141 to +158
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried about these manual database mutations in the test, since they don't quite guarantee that the same logic is applied as it would through the API. I think the issue might be that this is mixing unit testing of the create_or_update() fn with integration testing of the API endpoint behavior.

I would suggest putting the unit test(s) into the database crate via mod tests and then adjusting the test here to only use the API endpoints. It might also make sense to use a custom GitHub API mock for this test so that we can better control the returned IDs.

We should probably also explicitly test the case where the org IDs do not match to avoid accidentally enabling resurrection attacks.


assert_eq!(
crate::schema::teams::table
.count()
.get_result::<i64>(&mut conn)
.await?,
1
);
token
.add_named_owner("foo_same_name_different_id", "github:test-org:core")
.await
.good();
let json = anon
.crate_owner_teams("foo_same_name_different_id")
.await
.good();

assert_eq!(json.teams.len(), 1);
assert_eq!(json.teams[0].login, "github:test-org:core");
assert_eq!(json.teams[0].id, new_team.id);
Ok(())
}

/// Test adding team names with mixed case, when on the team
#[tokio::test(flavor = "multi_thread")]
async fn add_team_mixed_case() -> anyhow::Result<()> {
Expand Down