chore: minor refine on RoleMgr (#17683)

* chore: minor refine on RoleMgr

* refactor: rename RoleApi::get_ownerships to list_ownerships

Author: drmingdrmer

src/query/management/src/role/role_api.rs

@@ -32,7 +32,7 @@ pub trait RoleApi: Sync + Send {
 
     async fn get_raw_meta_roles(&self) -> Result<ListKVReply>;
 
-    async fn get_ownerships(&self) -> Result<Vec<SeqV<OwnershipInfo>>>;
+    async fn list_ownerships(&self) -> Result<Vec<SeqV<OwnershipInfo>>>;
 
     /// General role update.
     ///

src/query/management/src/role/role_mgr.rs

@@ -57,6 +57,7 @@ static TXN_MAX_RETRY_TIMES: u32 = 60;
 
 static BUILTIN_ROLE_ACCOUNT_ADMIN: &str = "account_admin";
 
+#[derive(Clone)]
 pub struct RoleMgr {
     kv_api: Arc<dyn kvapi::KVApi<Error = MetaError> + Send + Sync>,
     tenant: Tenant,
@@ -210,7 +211,7 @@ impl RoleApi for RoleMgr {
 
     #[async_backtrace::framed]
     #[fastrace::trace]
-    async fn get_ownerships(&self) -> Result<Vec<SeqV<OwnershipInfo>>, ErrorCode> {
+    async fn list_ownerships(&self) -> Result<Vec<SeqV<OwnershipInfo>>, ErrorCode> {
         let object_owner_prefix = self.ownership_object_prefix();
         let values = self
             .kv_api
@@ -228,7 +229,7 @@ impl RoleApi for RoleMgr {
                 // After rollback the old version, deserialize will return Err Ownership can not be none.
                 // But get ownerships should try to ensure success because in this version.
                 Err(err) => error!(
-                    "deserialize key {} Got err {} while (get_ownerships)",
+                    "deserialize key {} Got err {} while (list_ownerships)",
                     &key, err
                 ),
             }
@@ -269,11 +270,11 @@ impl RoleApi for RoleMgr {
 
     /// Only drop role will call transfer.
     ///
-    /// If a role is dropped, but the owner object is exists,
+    /// If a role is dropped, but the owner object is existing,
     ///
     /// The owner role need to update to account_admin.
     ///
-    /// get_ownerships use prefix_list_kv that will generate once meta call
+    /// list_ownerships use prefix_list_kv that will generate once meta call
     ///
     /// According to Txn reduce meta call. If role own n objects, will generate once meta call.
     #[async_backtrace::framed]
@@ -287,7 +288,7 @@ impl RoleApi for RoleMgr {
             trials.next().unwrap().map_err(AppError::from)?.await;
             let mut if_then = vec![];
             let mut condition = vec![];
-            let seq_owns = self.get_ownerships().await.map_err(|e| {
+            let seq_owns = self.list_ownerships().await.map_err(|e| {
                 e.add_message_back("(while in transfer_ownership_to_admin get ownerships).")
             })?;
             let mut need_transfer = false;

src/query/service/src/interpreters/access/privilege_access.rs

@@ -764,7 +764,7 @@ impl AccessChecker for PrivilegeAccess {
                         let user_api = UserApiProvider::instance();
                         let ownerships = user_api
                             .role_api(&tenant)
-                            .get_ownerships()
+                            .list_ownerships()
                             .await?;
                         let roles = self.ctx.get_all_effective_roles().await?;
                         let roles_name: Vec<String> = roles.iter().map(|role| role.name.to_string()).collect();
@@ -782,7 +782,7 @@ impl AccessChecker for PrivilegeAccess {
                         let user_api = UserApiProvider::instance();
                         let ownerships = user_api
                             .role_api(&tenant)
-                            .get_ownerships()
+                            .list_ownerships()
                             .await?;
                         let roles = self.ctx.get_all_effective_roles().await?;
                         let roles_name: Vec<String> = roles.iter().map(|role| role.name.to_string()).collect();
@@ -937,7 +937,7 @@ impl AccessChecker for PrivilegeAccess {
                 let user_api = UserApiProvider::instance();
                 let ownerships = user_api
                     .role_api(&tenant)
-                    .get_ownerships()
+                    .list_ownerships()
                     .await?;
                 let roles = self.ctx.get_all_effective_roles().await?;
                 let roles_name: Vec<String> = roles.iter().map(|role| role.name.to_string()).collect();

src/query/service/src/interpreters/interpreter_create_warehouses.rs

@@ -83,8 +83,8 @@ impl Interpreter for CreateWarehouseInterpreter {
                 .await?;
 
             if let WarehouseInfo::SystemManaged(sw) = warehouse {
-                let role_api = UserApiProvider::instance().role_api(&tenant);
                 if let Some(current_role) = self.ctx.get_current_role() {
+                    let role_api = UserApiProvider::instance().role_api(&tenant);
                     role_api
                         .grant_ownership(
                             &OwnershipObject::Warehouse { id: sw.role_id },
@@ -114,8 +114,8 @@ impl Interpreter for CreateWarehouseInterpreter {
             .await?;
 
         if let WarehouseInfo::SystemManaged(sw) = warehouse {
-            let role_api = UserApiProvider::instance().role_api(&tenant);
             if let Some(current_role) = self.ctx.get_current_role() {
+                let role_api = UserApiProvider::instance().role_api(&tenant);
                 role_api
                     .grant_ownership(
                         &OwnershipObject::Warehouse { id: sw.role_id },

src/query/service/src/interpreters/interpreter_database_create.rs

@@ -75,8 +75,8 @@ impl Interpreter for CreateDatabaseInterpreter {
 
         // Grant ownership as the current role. The above create_db_req.meta.owner could be removed in
         // the future.
-        let role_api = UserApiProvider::instance().role_api(&tenant);
         if let Some(current_role) = self.ctx.get_current_role() {
+            let role_api = UserApiProvider::instance().role_api(&tenant);
             role_api
                 .grant_ownership(
                     &OwnershipObject::Database {

src/query/service/src/interpreters/interpreter_drop_warehouses.rs

@@ -72,8 +72,8 @@ impl Interpreter for DropWarehouseInterpreter {
         );
 
         if let WarehouseInfo::SystemManaged(sw) = warehouse {
-            let role_api = UserApiProvider::instance().role_api(&tenant);
             if let Some(current_role) = self.ctx.get_current_role() {
+                let role_api = UserApiProvider::instance().role_api(&tenant);
                 role_api
                     .grant_ownership(
                         &OwnershipObject::Warehouse { id: sw.role_id },

src/query/service/src/interpreters/interpreter_user_stage_create.rs

@@ -121,8 +121,8 @@ impl Interpreter for CreateUserStageInterpreter {
 
         // Grant ownership as the current role
         let tenant = self.ctx.get_tenant();
-        let role_api = UserApiProvider::instance().role_api(&tenant);
         if let Some(current_role) = self.ctx.get_current_role() {
+            let role_api = UserApiProvider::instance().role_api(&tenant);
             role_api
                 .grant_ownership(
                     &OwnershipObject::Stage {

src/query/service/src/sessions/session_privilege_mgr.rs

@@ -397,7 +397,7 @@ impl SessionPrivilegeManager for SessionPrivilegeManagerImpl<'_> {
                 let user_api = UserApiProvider::instance();
                 let ownerships = user_api
                     .role_api(&self.session_ctx.get_current_tenant())
-                    .get_ownerships()
+                    .list_ownerships()
                     .await?;
                 let mut ownership_objects = vec![];
                 for ownership in ownerships {

src/query/service/src/table_functions/show_grants/show_grants_table.rs

@@ -495,7 +495,7 @@ async fn show_account_grants(
         // 2. not expand roles
         // So no need to get ownerships.
         if !roles.is_empty() {
-            let ownerships = user_api.role_api(&tenant).get_ownerships().await?;
+            let ownerships = user_api.role_api(&tenant).list_ownerships().await?;
             for ownership in ownerships {
                 if roles.contains(&ownership.data.role) {
                     match ownership.data.object {
@@ -807,7 +807,7 @@ async fn show_object_grant(
         }
     }
 
-    let ownerships = user_api.role_api(&tenant).get_ownerships().await?;
+    let ownerships = user_api.role_api(&tenant).list_ownerships().await?;
     for ownership in ownerships {
         if ownership.data.object == owner_object {
             privileges.push("OWNERSHIP".to_string());

src/query/storages/system/src/streams_table.rs

@@ -157,7 +157,7 @@ impl<const T: bool> AsyncSystemTable for StreamsTable<T> {
                 .collect::<Vec<_>>();
 
             let ownership = if T {
-                user_api.get_ownerships(&tenant).await.unwrap_or_default()
+                user_api.list_ownerships(&tenant).await.unwrap_or_default()
             } else {
                 HashMap::new()
             };

src/query/storages/system/src/tables_table.rs

@@ -589,7 +589,7 @@ where TablesTable<WITH_HISTORY, WITHOUT_VIEW>: HistoryAware
                 dbs.clear();
 
                 let ownership = if get_ownership && default_catalog {
-                    user_api.get_ownerships(&tenant).await.unwrap_or_default()
+                    user_api.list_ownerships(&tenant).await.unwrap_or_default()
                 } else {
                     HashMap::new()
                 };

src/query/users/src/role_mgr.rs

@@ -84,13 +84,13 @@ impl UserApiProvider {
     }
 
     #[async_backtrace::framed]
-    pub async fn get_ownerships(
+    pub async fn list_ownerships(
         &self,
         tenant: &Tenant,
     ) -> Result<HashMap<OwnershipObject, String>> {
         let seq_owns = self
             .role_api(tenant)
-            .get_ownerships()
+            .list_ownerships()
             .await
             .map_err(|e| e.add_message_back("(while get ownerships)."))?;
 
@@ -172,7 +172,7 @@ impl UserApiProvider {
         if let Some(owner) = ownership {
             // if object has ownership, but the owner role is not exists, set owner role to ACCOUNT_ADMIN,
             // only account_admin can access this object.
-            // Note: get_ownerships no need to do this check.
+            // Note: list_ownerships no need to do this check.
             // Because this can cause system.table queries to slow down
             // The intention is that the account admin can grant ownership.
             // So system.tables will display dropped role. It's by design.

src/query/users/src/user_api.rs

@@ -28,7 +28,6 @@ use databend_common_management::PasswordPolicyMgr;
 use databend_common_management::ProcedureMgr;
 use databend_common_management::QuotaApi;
 use databend_common_management::QuotaMgr;
-use databend_common_management::RoleApi;
 use databend_common_management::RoleMgr;
 use databend_common_management::SettingMgr;
 use databend_common_management::StageApi;
@@ -45,6 +44,7 @@ use databend_common_meta_store::MetaStore;
 use databend_common_meta_store::MetaStoreProvider;
 use databend_common_meta_types::MatchSeq;
 use databend_common_meta_types::MetaError;
+use log::debug;
 
 use crate::builtin::BuiltIn;
 use crate::BUILTIN_ROLE_PUBLIC;
@@ -65,6 +65,7 @@ impl UserApiProvider {
     ) -> Result<()> {
         GlobalInstance::set(Self::try_create(conf, builtin, tenant).await?);
         let user_mgr = UserApiProvider::instance();
+
         if let Some(q) = quota {
             let i = user_mgr.tenant_quota_api(tenant);
             let res = i.get_quota(MatchSeq::GE(0)).await?;
@@ -125,13 +126,14 @@ impl UserApiProvider {
         Arc::new(user_mgr)
     }
 
-    pub fn role_api(&self, tenant: &Tenant) -> Arc<impl RoleApi> {
+    pub fn role_api(&self, tenant: &Tenant) -> RoleMgr {
         let role_mgr = RoleMgr::create(
             self.client.clone(),
             tenant,
             GlobalConfig::instance().query.upgrade_to_pb,
         );
-        Arc::new(role_mgr)
+        debug!("RoleMgr created");
+        role_mgr
     }
 
     pub fn stage_api(&self, tenant: &Tenant) -> Arc<dyn StageApi> {