From 1c65cd115e0a6940c350441fe71f2fef4e8e12e6 Mon Sep 17 00:00:00 2001 From: Valentin Tolmer Date: Wed, 31 Jan 2024 00:44:17 +0100 Subject: [PATCH] server: Fix panic due to database collation When the database's collation is not "C", the DB order is not the same as the Rust order. As such, asserting that the elements are in increasing order fails. However, since both queries get the order from the database, they should be in the same order. With too many users, the query had a giant filter `IN (u1, u2, u3, ...)`. In PostgreSQL, we can pass the users as an array instead, but that doesn't work with SQLite. Instead, we repeat the filter from the previous query to get the same users/groups, as a subquery. --- .../src/domain/sql_group_backend_handler.rs | 48 +++++++++--------- server/src/domain/sql_user_backend_handler.rs | 49 ++++++++++--------- 2 files changed, 49 insertions(+), 48 deletions(-) diff --git a/server/src/domain/sql_group_backend_handler.rs b/server/src/domain/sql_group_backend_handler.rs index 8facaae..a0da301 100644 --- a/server/src/domain/sql_group_backend_handler.rs +++ b/server/src/domain/sql_group_backend_handler.rs @@ -79,25 +79,24 @@ fn get_group_filter_expr(filter: GroupRequestFilter) -> Cond { impl GroupListerBackendHandler for SqlBackendHandler { #[instrument(skip(self), level = "debug", ret, err)] async fn list_groups(&self, filters: Option) -> Result> { + let filters = filters + .map(|f| { + GroupColumn::GroupId + .in_subquery( + model::Group::find() + .find_also_linked(model::memberships::GroupToUser) + .select_only() + .column(GroupColumn::GroupId) + .filter(get_group_filter_expr(f)) + .into_query(), + ) + .into_condition() + }) + .unwrap_or_else(|| SimpleExpr::Value(true.into()).into_condition()); let results = model::Group::find() .order_by_asc(GroupColumn::GroupId) .find_with_related(model::Membership) - .filter( - filters - .map(|f| { - GroupColumn::GroupId - .in_subquery( - model::Group::find() - .find_also_linked(model::memberships::GroupToUser) - .select_only() - .column(GroupColumn::GroupId) - .filter(get_group_filter_expr(f)) - .into_query(), - ) - .into_condition() - }) - .unwrap_or_else(|| SimpleExpr::Value(true.into()).into_condition()), - ) + .filter(filters.clone()) .all(&self.sql_pool) .await?; let mut groups: Vec<_> = results @@ -110,9 +109,16 @@ impl GroupListerBackendHandler for SqlBackendHandler { } }) .collect(); - let group_ids = groups.iter().map(|u| &u.id); let attributes = model::GroupAttributes::find() - .filter(model::GroupAttributesColumn::GroupId.is_in(group_ids)) + .filter( + model::GroupAttributesColumn::GroupId.in_subquery( + model::Group::find() + .filter(filters) + .select_only() + .column(model::groups::Column::GroupId) + .into_query(), + ), + ) .order_by_asc(model::GroupAttributesColumn::GroupId) .order_by_asc(model::GroupAttributesColumn::AttributeName) .all(&self.sql_pool) @@ -120,12 +126,6 @@ impl GroupListerBackendHandler for SqlBackendHandler { let mut attributes_iter = attributes.into_iter().peekable(); use itertools::Itertools; // For take_while_ref for group in groups.iter_mut() { - assert!(attributes_iter - .peek() - .map(|u| u.group_id >= group.id) - .unwrap_or(true), - "Attributes are not sorted, groups are not sorted, or previous group didn't consume all the attributes"); - group.attributes = attributes_iter .take_while_ref(|u| u.group_id == group.id) .map(AttributeValue::from) diff --git a/server/src/domain/sql_user_backend_handler.rs b/server/src/domain/sql_user_backend_handler.rs index d52418a..ef53271 100644 --- a/server/src/domain/sql_user_backend_handler.rs +++ b/server/src/domain/sql_user_backend_handler.rs @@ -104,23 +104,22 @@ impl UserListerBackendHandler for SqlBackendHandler { // To simplify the query, we always fetch groups. TODO: cleanup. _get_groups: bool, ) -> Result> { + let filters = filters + .map(|f| { + UserColumn::UserId + .in_subquery( + model::User::find() + .find_also_linked(model::memberships::UserToGroup) + .select_only() + .column(UserColumn::UserId) + .filter(get_user_filter_expr(f)) + .into_query(), + ) + .into_condition() + }) + .unwrap_or_else(|| SimpleExpr::Value(true.into()).into_condition()); let mut users: Vec<_> = model::User::find() - .filter( - filters - .map(|f| { - UserColumn::UserId - .in_subquery( - model::User::find() - .find_also_linked(model::memberships::UserToGroup) - .select_only() - .column(UserColumn::UserId) - .filter(get_user_filter_expr(f)) - .into_query(), - ) - .into_condition() - }) - .unwrap_or_else(|| SimpleExpr::Value(true.into()).into_condition()), - ) + .filter(filters.clone()) .order_by_asc(UserColumn::UserId) .find_with_linked(model::memberships::UserToGroup) .order_by_asc(SimpleExpr::Column( @@ -134,10 +133,18 @@ impl UserListerBackendHandler for SqlBackendHandler { groups: Some(groups.into_iter().map(Into::::into).collect()), }) .collect(); + // At this point, the users don't have attributes, we need to populate it with another query. - let user_ids = users.iter().map(|u| &u.user.user_id); let attributes = model::UserAttributes::find() - .filter(model::UserAttributesColumn::UserId.is_in(user_ids)) + .filter( + model::UserAttributesColumn::UserId.in_subquery( + model::User::find() + .filter(filters) + .select_only() + .column(model::users::Column::UserId) + .into_query(), + ), + ) .order_by_asc(model::UserAttributesColumn::UserId) .order_by_asc(model::UserAttributesColumn::AttributeName) .all(&self.sql_pool) @@ -145,12 +152,6 @@ impl UserListerBackendHandler for SqlBackendHandler { let mut attributes_iter = attributes.into_iter().peekable(); use itertools::Itertools; // For take_while_ref for user in users.iter_mut() { - assert!(attributes_iter - .peek() - .map(|u| u.user_id >= user.user.user_id) - .unwrap_or(true), - "Attributes are not sorted, users are not sorted, or previous user didn't consume all the attributes"); - user.user.attributes = attributes_iter .take_while_ref(|u| u.user_id == user.user.user_id) .map(AttributeValue::from)