Cargo owner username disambiguation - #14213
Conversation
This comment has been minimized.
This comment has been minimized.
454d638 to
a420795
Compare
This comment has been minimized.
This comment has been minimized.
f43ce65 to
64efb97
Compare
542309c to
6811381
Compare
|
@Turbo87 thanks for the review! I've addressed your comments. the pr is ready for re-review. |
a228f14 to
8a44eae
Compare
This comment has been minimized.
This comment has been minimized.
|
@carols10cents would also love your review on this when you have some time |
185aa74 to
1f3ff8a
Compare
This comment has been minimized.
This comment has been minimized.
1f3ff8a to
4c528c3
Compare
There was a problem hiding this comment.
Took a quick look and saw one little thing, and I agree with the comments @Turbo87 made.
This comment has been minimized.
This comment has been minimized.
3e8fc63 to
b09e6a3
Compare
This comment has been minimized.
This comment has been minimized.
|
@Turbo87 @carols10cents ready for re-review. thanks! |
|
@Turbo87 @carols10cents still looking for a review on this one when you have some time :) |
This comment has been minimized.
This comment has been minimized.
ca80a41 to
c73265e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c73265e to
270d425
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| AND crate_owners.owner_id = crate_owners_with_login.owner_id | ||
| AND crate_owners.owner_kind = crate_owners_with_login.owner_kind | ||
| AND lower(crate_owners_with_login.login) = lower($2);"#, | ||
| AND canon_username(crate_owners_with_login.login) = canon_username($2);"#, |
There was a problem hiding this comment.
I just noticed that the change to this query, because crate_owners_with_login is selecting both teams.login and users.username as crate_owners_with_login.login, makes it so that we're comparing team names using the canon_username function (which also normalizes hyphen to underscore), whereas before we were only comparing team names using lower.
I don't think we want to change anything about how we handle team names right now. That's under GitHub's control (and we don't have plans to change the way teams work yet).
Looking at the production database, I don't see any team names that differ only by hyphen/underscore today, but I do see some team names with hyphens and some with underscores, so GitHub does allow it. While I don't think it would be a good idea for anyone to do this, I just confirmed that it's possible to create two teams in GitHub, one named crates-io and one named crates_io. I think the change to this query would mean you couldn't manage them independently. If they both existed in crates.io, then you ran cargo owner --add github:rust-lang:crates-io, both the crates-io and the crates_io team would get added, if my reasoning is correct.
Given that we now have this query in owner_remove_with_username and a slightly different query in owner_remove_with_gh_login, maybe we should split owner_remove_with_username into owner_remove_with_username and owner_remove_with_team_name?
| ) -> QueryResult<OauthGithub> { | ||
| oauth_github::table | ||
| .filter(canon_username(oauth_github::login).eq(canon_username(login))) | ||
| .filter(oauth_github::account_id.ne(-1)) |
There was a problem hiding this comment.
The .filter(oauth_github::account_id.ne(-1)) isn't necessary-- users that have gh_id set to -1 in the users table just don't have any records in the oauth_github table. The -1 values were always sort of a hack and I'm looking forward to getting rid of it with this transition, not continue propagating it :)
| gh_login: &str, | ||
| gh_id: i32, | ||
| ) -> QueryResult<i32> { | ||
| let user_id = NewUser::builder() |
| .insert(conn) | ||
| .await?; | ||
|
|
||
| NewOauthGithub::builder() |
There was a problem hiding this comment.
... OauthGithubBuilder::for_user here?
These test builders might not have existed when you started this PR, but they should make other changes smaller, like the User.gh_login change you mentioned we should do in another comment :)
| @@ -0,0 +1 @@ | |||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS index_oauth_github_login ON oauth_github (lower(login)); | |||
There was a problem hiding this comment.
Based on a resolved comment thread on this PR that I'm having trouble linking to and the current state of the query in owner_remove_with_gh_login that has AND canon_username(crate_owners_with_gh_login.login) = canon_username($2), I expected this index to be ON oauth_github (canon_username(login)) rather than lower? Maybe the change got lost in a rebase?
There was a problem hiding this comment.
sorry, I wasn't done with refactorings in this branch yet 🙈
I think we should actually do the opposite and keep GitHub account comparisons as only lower(), since hyphen and underscore are not equivalent on the GitHub side, even if we plan on treating them that way for the crates.io usernames.
There was a problem hiding this comment.
oh whoops! sorry-- i'll hold off with the rest of my review til you say you're done :)
sounds good with the github normalization!
| @@ -3293,9 +3293,14 @@ export interface operations { | |||
| * | |||
| * For users, use just the username (e.g., `"octocat"`). | |||
There was a problem hiding this comment.
Tiny nit that I think will make this documentation clearer: The note this PR adds about disambiguation of usernames should be added up here with users rather than after the note about teams, and the "just" in "use just the username" should be removed because that's not necessarily true now.
| @@ -3358,9 +3363,14 @@ export interface operations { | |||
| * | |||
| * For users, use just the username (e.g., `"octocat"`). | |||
There was a problem hiding this comment.
Same here with moving the disambiguation note up here and removing "just"
| @@ -192,7 +195,10 @@ pub struct ChangeOwnersRequest { | |||
| /// | |||
| /// For users, use just the username (e.g., `"octocat"`). | |||
There was a problem hiding this comment.
Same here with moving the disambiguation note up here and removing "just"
| let login_test = |owner: &Owner| -> bool { | ||
| match parsed_login { | ||
| Login::GitHubTeam(_) => { | ||
| canon_username(owner.username()) == canon_username(login) |
There was a problem hiding this comment.
Similarly to my comment on owner_remove_with_username, I think this is changing the way we handle team names in a way we don't want to change in this PR. Team names should still only use lower.
| .select(User::as_select()) | ||
| .first(conn) | ||
| .await | ||
| } |
There was a problem hiding this comment.
Note for myself: this is where i stopped reviewing and saved the review
The helper mirrors the PostgreSQL function so in-memory owner matching uses the same case folding and `-` to `_` normalization as database queries.
The add-owner endpoint accepts `crates.io:username` arguments and resolves them using crates.io username canonicalization. Owner invitations now retain the selected username so responses identify the requested crates.io account without changing legacy unprefixed behavior.
The expression index supports queries using `lower(oauth_github.login)`. Concurrent creation and removal avoid blocking writes and therefore run outside a transaction.
`OauthGithub::find_by_login()` selects the matching account with the highest account ID when historical rows differ only by case. Joined `User` queries also expose the OAuth login required by owner resolution.
The add-owner endpoint accepts `github:username` arguments and resolves them through linked OAuth accounts. Shared owner-argument validation distinguishes GitHub users from teams while both prefixed removal forms remain unsupported.
`Crate` provides separate removal queries for canonical crates.io usernames and linked GitHub logins. Owner routes select the query for the parsed user or team argument.
The remove-owner endpoint accepts `crates.io:username` arguments and removes the current owner resolved through crates.io username canonicalization. GitHub-prefixed removal arguments remain rejected until their corresponding removal support is enabled.
The remove-owner endpoint accepts `github:username` arguments and removes the current owner resolved through the GitHub login namespace. The owner request documentation now describes both service prefixes for additions and removals.
Unprefixed additions resolve canonical crates.io usernames and require the linked GitHub login to identify the same name. Ambiguous accounts receive explicit `crates.io:` and `github:` commands for selecting the intended user.
Unprefixed removals compare the crate's current owners by canonical crates.io username and linked GitHub login. A sole match is removed, while distinct owners with the same supplied name produce an ambiguity error.
270d425 to
6836aea
Compare
|
☔ The latest upstream changes (possibly #14577) made this pull request unmergeable. Please resolve the merge conflicts. |
4 participants
fixes: #13769