-
Notifications
You must be signed in to change notification settings - Fork 54
syn2mas: Support migrating external IDs as upstream OAuth2 providers #3917
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
Conversation
Deploying matrix-authentication-service-docs with
|
Latest commit: |
6b02497
|
Status: | ✅ Deploy successful! |
Preview URL: | https://6f583dfb.matrix-authentication-service-docs.pages.dev |
Branch Preview URL: | https://rei-syn2mas-7-external-ids.matrix-authentication-service-docs.pages.dev |
704ddb7
to
6b02497
Compare
#[allow(clippy::missing_panics_doc)] // not a real panic | ||
#[tracing::instrument(skip_all, level = Level::DEBUG)] | ||
pub async fn write_users(&mut self, users: Vec<MasNewUser>) -> Result<(), Error> { | ||
pub fn write_users(&mut self, users: Vec<MasNewUser>) -> BoxFuture<'_, Result<(), Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning with boxing here, and why not -> impl Future
?
let mut link_ids: Vec<Uuid> = Vec::with_capacity(links.len()); | ||
let mut user_ids: Vec<Uuid> = Vec::with_capacity(links.len()); | ||
let mut upstream_provider_ids: Vec<Uuid> = Vec::with_capacity(links.len()); | ||
let mut subjects: Vec<String> = Vec::with_capacity(links.len()); | ||
let mut created_ats: Vec<DateTime<Utc>> = Vec::with_capacity(links.len()); | ||
|
||
for MasNewUpstreamOauthLink { | ||
link_id, | ||
user_id, | ||
upstream_provider_id, | ||
subject, | ||
created_at, | ||
} in links | ||
{ | ||
link_ids.push(link_id); | ||
user_ids.push(user_id); | ||
upstream_provider_ids.push(upstream_provider_id); | ||
subjects.push(subject); | ||
created_ats.push(created_at); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itertools::multiunzip
is a thing, not sure it would make that better though so 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mrmm, it's hard to find a tool that works here other than doing it the boring way. I don't see that much point in destructuring to a tuple. Maybe it's a little better, but not by much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
posterity note: the crates soa_derive
and columnar
look like they could be promising for this sort of thing, but it doesn't seem worth adding a new dep (+ proc_macro + compile time) for this when we only have a handful of these to do
type WriteBufferFlusher<'conn, T> = | ||
for<'a> fn(&'a mut MasWriter<'conn>, Vec<T>) -> BoxFuture<'a, Result<(), Error>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Riiight, all the box future make sense now :)
rng: &mut impl RngCore, | ||
) -> Result<UsersMigrated, Error> { | ||
let mut write_buffer = MasUserWriteBuffer::new(mas); | ||
let mut user_buffer = MasWriteBuffer::new(MasWriter::write_users); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how I feel about passing function pointers instead of using traits… I guess that works, not sure how it would look like with traits
Commit-by-commit suggested