Switch session create to check oauth_github first, then get user#13856
Switch session create to check oauth_github first, then get user#13856carols10cents wants to merge 2 commits into
oauth_github first, then get user#13856Conversation
We have `new_user`, which is a crates.io `NewUser`. I'm about to add usage of crates.io `User`s, which I think will be confusing unless this varibale name changes.
| .set(( | ||
| users::name.eq(gh_user.name.as_ref()), | ||
| users::gh_login.eq(&gh_user.login), | ||
| // users::username.eq(&gh_user.login), |
There was a problem hiding this comment.
This will need to be uncommented if #13781 gets in first
| .build(); | ||
|
|
||
| let user_id = | ||
| create_or_update_user(&new_user, gh_user.email.as_deref(), emails, conn) |
There was a problem hiding this comment.
create_or_update_user really should only be creating, i think. And it starts its own transaction, but i don't think diesel/postgres cares about nested transactions.
There was a problem hiding this comment.
but i don't think diesel/postgres cares about nested transactions
it does:
If there is already an open transaction, savepoints will be used instead.
(from https://docs.diesel.rs/1.4.x/diesel/connection/trait.Connection.html#method.transaction)
carols10cents
changed the title
oauth_github first, then get useroauth_github first, then get user
| Err(error) if is_read_only_error(&error) => { | ||
| // If we're in read only mode, we can't update their details | ||
| // just look for an existing user | ||
| find_user_by_gh_id(conn, gh_user.id).await?.ok_or(error) | ||
| } |
There was a problem hiding this comment.
this change has a hidden regression that becomes visible with #13862.
previously the nested transactions caused the "read-only error -> use read-only query instead" case to work, but now the transaction is poisoned and won't work anymore for the read-only query either.
and in case you think I'm clever: nope, this was found by Claude 😅
|
☔ The latest upstream changes (possibly #13781) made this pull request unmergeable. Please resolve the merge conflicts. |
3 participants
This is what I was talking about in this comment, that we can't use nice upserts but need to do multiple update/select/inserts in a transaction instead, so that we switch to using
oauth_githubas the source of truth of whether we're logging in an existing user or a new user.I've extracted this from #13832 and I think making this change first is needed before we can stop using
users.{gh_id, gh_avatar, gh_encrypted_token}.