From dcb7c5ef98dbfac53097c7cc96fce48b8f3ee7ec Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 20 Jan 2025 16:44:13 -0700 Subject: [PATCH] feat: Enhance permission group handling and streamline SQL queries - Expanded the `allow_columns_to_appear_in_same_group_by_clause!` macro in `models.rs` to include additional columns for datasets and users, improving query flexibility. - Refactored the `list_permission_groups` function to include dataset count and assigned status, enhancing the information returned for each permission group. - Updated SQL queries in `list_permission_groups` to utilize left joins for better data retrieval and to ensure accurate permission checks. - Removed redundant column allowances in various files, streamlining the codebase and improving maintainability. --- api/src/database/models.rs | 18 ++++- .../users/assets/list_permission_groups.rs | 67 ++++++++++++++----- api/src/routes/ws/datasets/list_datasets.rs | 15 ----- .../ws/permissions/list_permission_groups.rs | 7 -- api/src/routes/ws/permissions/list_teams.rs | 2 +- api/src/routes/ws/permissions/list_users.rs | 10 +-- .../ws/permissions/permissions_utils.rs | 8 +-- 7 files changed, 72 insertions(+), 55 deletions(-) diff --git a/api/src/database/models.rs b/api/src/database/models.rs index 09f216462..a67464ac3 100644 --- a/api/src/database/models.rs +++ b/api/src/database/models.rs @@ -15,7 +15,23 @@ allow_columns_to_appear_in_same_group_by_clause!( teams::name, permission_groups_to_identities::permission_group_id, teams_to_users::user_id, - teams_to_users::role + teams_to_users::role, + permission_groups::id, + permission_groups::name, + permission_groups_to_identities::identity_id, + permission_groups_to_identities::identity_type, + users::id, + users::name, + users::email, + users_to_organizations::role, + datasets::id, + datasets::name, + datasets::created_at, + datasets::updated_at, + datasets::enabled, + datasets::imported, + data_sources::id, + data_sources::name, ); #[derive(Queryable, Insertable, Identifiable, Associations, Debug)] diff --git a/api/src/routes/rest/routes/users/assets/list_permission_groups.rs b/api/src/routes/rest/routes/users/assets/list_permission_groups.rs index cd3415072..bc3e5b77e 100644 --- a/api/src/routes/rest/routes/users/assets/list_permission_groups.rs +++ b/api/src/routes/rest/routes/users/assets/list_permission_groups.rs @@ -7,9 +7,12 @@ use diesel_async::RunQueryDsl; use serde::Serialize; use uuid::Uuid; +use crate::database::enums::IdentityType; use crate::database::lib::get_pg_pool; -use crate::database::models::{PermissionGroup, User}; -use crate::database::schema::permission_groups; +use crate::database::models::User; +use crate::database::schema::{ + dataset_permissions, permission_groups, permission_groups_to_identities, +}; use crate::routes::rest::ApiResponse; use crate::utils::security::checks::is_user_workspace_admin_or_data_admin; use crate::utils::user::user_info::get_user_organization_id; @@ -18,9 +21,8 @@ use crate::utils::user::user_info::get_user_organization_id; pub struct PermissionGroupInfo { pub id: Uuid, pub name: String, - pub organization_id: Uuid, - pub created_at: DateTime, - pub updated_at: DateTime, + pub dataset_count: i64, + pub assigned: bool, } pub async fn list_permission_groups( @@ -45,24 +47,59 @@ async fn list_permission_groups_handler(user: User) -> Result = permission_groups::table + let groups = permission_groups::table + .left_join( + permission_groups_to_identities::table.on( + permission_groups_to_identities::permission_group_id + .eq(permission_groups::id) + .and(permission_groups_to_identities::deleted_at.is_null()) + .and(permission_groups_to_identities::identity_id.eq(user.id)) + .and(permission_groups_to_identities::identity_type.eq(IdentityType::User)), + ), + ) + .left_join( + dataset_permissions::table.on(dataset_permissions::permission_id + .eq(permission_groups::id) + .and(dataset_permissions::permission_type.eq("permission_group")) + .and(dataset_permissions::deleted_at.is_null()) + .and(dataset_permissions::organization_id.eq(organization_id))), + ) + .select(( + permission_groups::id, + permission_groups::name, + diesel::dsl::sql::( + "COALESCE(count(dataset_permissions.id), 0)", + ), + diesel::dsl::sql::( + "permission_groups_to_identities.identity_id IS NOT NULL", + ), + )) + .group_by(( + permission_groups::id, + permission_groups::name, + dataset_permissions::id, + permission_groups_to_identities::identity_id, + )) .filter(permission_groups::organization_id.eq(organization_id)) .filter(permission_groups::deleted_at.is_null()) .order_by(permission_groups::created_at.desc()) - .load(&mut *conn) + .load::<(Uuid, String, i64, bool)>(&mut *conn) .await?; Ok(groups .into_iter() - .map(|group| PermissionGroupInfo { - id: group.id, - name: group.name, - organization_id: group.organization_id, - created_at: group.created_at, - updated_at: group.updated_at, - }) + .map( + |(id, name, dataset_count, assigned)| PermissionGroupInfo { + id, + name, + dataset_count, + assigned, + }, + ) .collect()) } diff --git a/api/src/routes/ws/datasets/list_datasets.rs b/api/src/routes/ws/datasets/list_datasets.rs index 48e292e26..e807303bc 100644 --- a/api/src/routes/ws/datasets/list_datasets.rs +++ b/api/src/routes/ws/datasets/list_datasets.rs @@ -1,7 +1,6 @@ use anyhow::{anyhow, Result}; use chrono::{DateTime, Utc}; use diesel::{ - allow_columns_to_appear_in_same_group_by_clause, dsl::sql, sql_types::{Nullable, Timestamptz}, BoolExpressionMethods, ExpressionMethods, JoinOnDsl, NullableExpressionMethods, QueryDsl, @@ -78,20 +77,6 @@ pub struct ListDatasetObject { pub belongs_to: Option, } -allow_columns_to_appear_in_same_group_by_clause!( - datasets::id, - datasets::name, - datasets::created_at, - datasets::updated_at, - datasets::enabled, - datasets::imported, - users::id, - users::name, - users::email, - data_sources::id, - data_sources::name, -); - pub async fn list_datasets(user: &User, req: ListDatasetsRequest) -> Result<()> { let list_dashboards_res = match list_datasets_handler( &user.id, diff --git a/api/src/routes/ws/permissions/list_permission_groups.rs b/api/src/routes/ws/permissions/list_permission_groups.rs index 3f3051bba..2ae80222e 100644 --- a/api/src/routes/ws/permissions/list_permission_groups.rs +++ b/api/src/routes/ws/permissions/list_permission_groups.rs @@ -1,6 +1,5 @@ use anyhow::{anyhow, Result}; use diesel::{ - allow_columns_to_appear_in_same_group_by_clause, dsl::sql, sql_types::{Array, BigInt, Text, Uuid as DieselUuid}, BoolExpressionMethods, ExpressionMethods, JoinOnDsl, NullableExpressionMethods, QueryDsl, @@ -65,12 +64,6 @@ pub struct PermissionGroupInfo { pub belongs_to: Option, } -allow_columns_to_appear_in_same_group_by_clause!( - permission_groups::id, - permission_groups::name, - permission_groups_to_identities::identity_id, -); - pub async fn list_permission_groups(user: &User, req: ListPermissionGroupsRequest) -> Result<()> { let permission_groups = match list_permission_groups_handler(user, req.page, req.page_size, req.filters).await { diff --git a/api/src/routes/ws/permissions/list_teams.rs b/api/src/routes/ws/permissions/list_teams.rs index 1b868a053..cef46051b 100644 --- a/api/src/routes/ws/permissions/list_teams.rs +++ b/api/src/routes/ws/permissions/list_teams.rs @@ -1,6 +1,6 @@ use anyhow::{anyhow, Result}; use diesel::{ - allow_columns_to_appear_in_same_group_by_clause, dsl::sql, sql_types::BigInt, + dsl::sql, sql_types::BigInt, BoolExpressionMethods, ExpressionMethods, JoinOnDsl, NullableExpressionMethods, QueryDsl, }; use diesel_async::RunQueryDsl; diff --git a/api/src/routes/ws/permissions/list_users.rs b/api/src/routes/ws/permissions/list_users.rs index fad9cd9d9..ac2f702a4 100644 --- a/api/src/routes/ws/permissions/list_users.rs +++ b/api/src/routes/ws/permissions/list_users.rs @@ -1,6 +1,6 @@ use anyhow::{anyhow, Result}; use diesel::{ - alias, allow_columns_to_appear_in_same_group_by_clause, + alias, dsl::sql, sql_types::{Array, BigInt, Text, Uuid as DieselUuid}, BoolExpressionMethods, ExpressionMethods, JoinOnDsl, NullableExpressionMethods, QueryDsl, @@ -160,14 +160,6 @@ async fn list_all_users( let team_pgi = alias!(permission_groups_to_identities as team_pgi); - allow_columns_to_appear_in_same_group_by_clause!( - users::id, - users::name, - users::email, - users_to_organizations::role, - permission_groups_to_identities::permission_group_id - ); - let user_permissions: Vec<(Uuid, Option, String, i64, i64, UserOrganizationRole)> = match users::table .left_join(teams_to_users::table.on(users::id.eq(teams_to_users::user_id).and(teams_to_users::deleted_at.is_null()))) .inner_join(users_to_organizations::table.on(users::id.eq(users_to_organizations::user_id).and(users_to_organizations::deleted_at.is_null()))) diff --git a/api/src/routes/ws/permissions/permissions_utils.rs b/api/src/routes/ws/permissions/permissions_utils.rs index 58a6a82bb..f521f898e 100644 --- a/api/src/routes/ws/permissions/permissions_utils.rs +++ b/api/src/routes/ws/permissions/permissions_utils.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use anyhow::{anyhow, Result}; use diesel::{ - alias, allow_columns_to_appear_in_same_group_by_clause, + alias, deserialize::QueryableByName, dsl::{count, not, sql}, insert_into, @@ -523,12 +523,6 @@ async fn get_user_queries_last_30_days(user_id: Arc) -> Result { async fn get_user_permission_groups(user_id: Arc) -> Result> { let mut conn = get_pg_pool().get().await?; - allow_columns_to_appear_in_same_group_by_clause!( - permission_groups::id, - permission_groups::name, - permission_groups_to_identities::identity_type, - ); - let permission_groups = match permission_groups::table .inner_join( permission_groups_to_identities::table