Skip to content

Commit

Permalink
chore: refine variable name
Browse files Browse the repository at this point in the history
  • Loading branch information
@drmingdrmer
drmingdrmer committed Feb 27, 2024
1 parent a8c4132 commit e74f3bb
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 82 deletions.
2 changes: 1 addition & 1 deletion src/meta/app/src/principal/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub use user_defined_function::UserDefinedFunction;
pub use user_grant::GrantEntry;
pub use user_grant::GrantObject;
pub use user_grant::OwnershipObject;
pub use user_grant::RoleGrantee;
pub use user_grant::TenantOwnershipObject;
pub use user_grant::UserGrantSet;
pub use user_identity::UserIdentity;
pub use user_info::UserInfo;
Expand Down
156 changes: 84 additions & 72 deletions src/meta/app/src/principal/user_grant.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,30 +118,21 @@ impl OwnershipObject {
})
}
"table-by-id" => {
unimplemented!("No db_id encoded, can not rebuild key");
#[allow(unreachable_code)]
{
let table_id = p.next_u64()?;
Ok(OwnershipObject::Table {
catalog_name: Self::DEFAULT_CATALOG.to_string(),
db_id: 0,
table_id,
})
}
let table_id = p.next_u64()?;
Ok(OwnershipObject::Table {
catalog_name: Self::DEFAULT_CATALOG.to_string(),
db_id: 0,
table_id,
})
}
"table-by-catalog-id" => {
unimplemented!("No db_id encoded, can not rebuild key");
#[allow(unreachable_code)]
{
let catalog_name = p.next_str()?;
let db_id = p.next_u64()?;
let table_id = p.next_u64()?;
Ok(OwnershipObject::Table {
catalog_name,
db_id,
table_id,
})
}
let catalog_name = p.next_str()?;
let table_id = p.next_u64()?;
Ok(OwnershipObject::Table {
catalog_name,
db_id: 0, // string key does not contain db_id
table_id,
})
}
"stage-by-name" => {
let name = p.next_str()?;
Expand All @@ -162,7 +153,7 @@ impl OwnershipObject {
}
}

/// The meta-service key of subject to grant a role to.
/// The meta-service key of object whose ownership to grant.
///
/// It could be a tenant's database, a tenant's table etc.
/// It is in form of `__fd_object_owners/<tenant>/<object>`.
Expand All @@ -174,31 +165,32 @@ impl OwnershipObject {
/// - `stage-by-name/<stage_name>`
/// - `udf-by-name/<udf_name>`
#[derive(Clone, Debug, Eq, PartialEq, Hash)]
pub struct RoleGrantee {
pub struct TenantOwnershipObject {
tenant: Tenant,
subject: OwnershipObject,
object: OwnershipObject,
}

impl RoleGrantee {
impl TenantOwnershipObject {
pub fn new(tenant: Tenant, subject: OwnershipObject) -> Self {
// TODO: remove it when we support multiple catalogs
// Legacy issue: Assert compatibility: No other catalog should be used.
match &subject {
OwnershipObject::Database { catalog_name, .. } => {
assert_eq!(catalog_name, OwnershipObject::DEFAULT_CATALOG);
}
OwnershipObject::Table { catalog_name, .. } => {
assert_eq!(catalog_name, OwnershipObject::DEFAULT_CATALOG);
}
OwnershipObject::Stage { .. } => {}
OwnershipObject::UDF { .. } => {}
}
// Assertion is disabled.
// Instead, accessing field `object` is disallowed.
// match &subject {
// OwnershipObject::Database { catalog_name, .. } => {
// assert_eq!(catalog_name, OwnershipObject::DEFAULT_CATALOG);
// }
// OwnershipObject::Table { catalog_name, .. } => {
// assert_eq!(catalog_name, OwnershipObject::DEFAULT_CATALOG);
// }
// OwnershipObject::Stage { .. } => {}
// OwnershipObject::UDF { .. } => {}
// }

Self::new_unchecked(tenant, subject)
}

pub(crate) fn new_unchecked(tenant: Tenant, subject: OwnershipObject) -> Self {
RoleGrantee { tenant, subject }
pub(crate) fn new_unchecked(tenant: Tenant, object: OwnershipObject) -> Self {
TenantOwnershipObject { tenant, object }
}

/// Return a encoded key prefix for listing keys belongs to the tenant.
Expand All @@ -210,13 +202,17 @@ impl RoleGrantee {
.push_str(&self.tenant.tenant)
.done()
}

pub fn tenant(&self) -> &Tenant {
&self.tenant
}

pub fn subject(&self) -> &OwnershipObject {
&self.subject
}
// Because the encoded string key does not contain all of the field:
// Catalog and db_id is missing, so we can not rebuild the complete key from string key.
// Thus it is not allow to access this field.
// pub fn subject(&self) -> &OwnershipObject {
// &self.object
// }
}

#[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Eq, PartialEq, Hash)]
Expand Down Expand Up @@ -491,12 +487,12 @@ mod kvapi_key_impl {
use databend_common_meta_kvapi::kvapi;
use databend_common_meta_kvapi::kvapi::KeyError;

use crate::principal::user_grant::RoleGrantee;
use crate::principal::user_grant::TenantOwnershipObject;
use crate::principal::OwnershipInfo;
use crate::principal::OwnershipObject;
use crate::tenant::Tenant;

impl kvapi::Key for RoleGrantee {
impl kvapi::Key for TenantOwnershipObject {
const PREFIX: &'static str = "__fd_object_owners";
type ValueType = OwnershipInfo;

Expand All @@ -506,7 +502,7 @@ mod kvapi_key_impl {

fn to_string_key(&self) -> String {
let b = kvapi::KeyBuilder::new_prefixed(Self::PREFIX).push_str(&self.tenant.tenant);
self.subject.build_key(b).done()
self.object.build_key(b).done()
}

fn from_str_key(s: &str) -> Result<Self, KeyError> {
Expand All @@ -516,9 +512,9 @@ mod kvapi_key_impl {
let subject = OwnershipObject::parse_key(&mut p)?;
p.done()?;

Ok(RoleGrantee {
Ok(TenantOwnershipObject {
tenant: Tenant::new(tenant),
subject,
object: subject,
})
}
}
Expand All @@ -534,107 +530,123 @@ mod kvapi_key_impl {
mod tests {
use databend_common_meta_kvapi::kvapi::Key;

use crate::principal::user_grant::RoleGrantee;
use crate::principal::user_grant::TenantOwnershipObject;
use crate::principal::OwnershipObject;
use crate::tenant::Tenant;

#[test]
fn test_role_grantee_as_kvapi_key() {
// db with default catalog
{
let role_grantee =
RoleGrantee::new_unchecked(Tenant::new("test"), OwnershipObject::Database {
let role_grantee = TenantOwnershipObject::new_unchecked(
Tenant::new("test"),
OwnershipObject::Database {
catalog_name: "default".to_string(),
db_id: 1,
});
},
);

let key = role_grantee.to_string_key();
assert_eq!("__fd_object_owners/test/database-by-id/1", key);

let parsed = RoleGrantee::from_str_key(&key).unwrap();
let parsed = TenantOwnershipObject::from_str_key(&key).unwrap();
assert_eq!(role_grantee, parsed);
}

// db with catalog
{
let role_grantee =
RoleGrantee::new_unchecked(Tenant::new("test"), OwnershipObject::Database {
let role_grantee = TenantOwnershipObject::new_unchecked(
Tenant::new("test"),
OwnershipObject::Database {
catalog_name: "cata/foo".to_string(),
db_id: 1,
});
},
);

let key = role_grantee.to_string_key();
assert_eq!(
"__fd_object_owners/test/database-by-catalog-id/cata%2ffoo/1",
key
);

let parsed = RoleGrantee::from_str_key(&key).unwrap();
let parsed = TenantOwnershipObject::from_str_key(&key).unwrap();
assert_eq!(role_grantee, parsed);
}

// table with default catalog
{
let role_grantee =
RoleGrantee::new_unchecked(Tenant::new("test"), OwnershipObject::Table {
let obj =
TenantOwnershipObject::new_unchecked(Tenant::new("test"), OwnershipObject::Table {
catalog_name: "default".to_string(),
db_id: 1,
table_id: 2,
});

let key = role_grantee.to_string_key();
let key = obj.to_string_key();
assert_eq!("__fd_object_owners/test/table-by-id/2", key);

// TODO: Cant not parse key, db-id is not encoded
// let parsed = RoleGrantee::from_str_key(&key).unwrap();
// assert_eq!(role_grantee, parsed);
let parsed = TenantOwnershipObject::from_str_key(&key).unwrap();
assert_eq!(
TenantOwnershipObject::new_unchecked(Tenant::new("test"), OwnershipObject::Table {
catalog_name: "default".to_string(),
db_id: 0, // db_id is not encoded into key
table_id: 2,
}),
parsed
);
}

// table with catalog
{
let role_grantee =
RoleGrantee::new_unchecked(Tenant::new("test"), OwnershipObject::Table {
let obj =
TenantOwnershipObject::new_unchecked(Tenant::new("test"), OwnershipObject::Table {
catalog_name: "cata/foo".to_string(),
db_id: 1,
table_id: 2,
});

let key = role_grantee.to_string_key();
let key = obj.to_string_key();
assert_eq!(
"__fd_object_owners/test/table-by-catalog-id/cata%2ffoo/2",
key
);

// TODO: Cant not parse key, db-id is not encoded
// let parsed = RoleGrantee::from_str_key(&key).unwrap();
// assert_eq!(role_grantee, parsed);
let parsed = TenantOwnershipObject::from_str_key(&key).unwrap();
assert_eq!(
TenantOwnershipObject::new_unchecked(Tenant::new("test"), OwnershipObject::Table {
catalog_name: "cata/foo".to_string(),
db_id: 0, // db_id is not encoded into key
table_id: 2,
}),
parsed
);
}

// stage
{
let role_grantee =
RoleGrantee::new_unchecked(Tenant::new("test"), OwnershipObject::Stage {
TenantOwnershipObject::new_unchecked(Tenant::new("test"), OwnershipObject::Stage {
name: "foo".to_string(),
});

let key = role_grantee.to_string_key();
assert_eq!("__fd_object_owners/test/stage-by-name/foo", key);

let parsed = RoleGrantee::from_str_key(&key).unwrap();
let parsed = TenantOwnershipObject::from_str_key(&key).unwrap();
assert_eq!(role_grantee, parsed);
}

// udf
{
let role_grantee =
RoleGrantee::new_unchecked(Tenant::new("test"), OwnershipObject::UDF {
TenantOwnershipObject::new_unchecked(Tenant::new("test"), OwnershipObject::UDF {
name: "foo".to_string(),
});

let key = role_grantee.to_string_key();
assert_eq!("__fd_object_owners/test/udf-by-name/foo", key);

let parsed = RoleGrantee::from_str_key(&key).unwrap();
let parsed = TenantOwnershipObject::from_str_key(&key).unwrap();
assert_eq!(role_grantee, parsed);
}
}
Expand Down
18 changes: 9 additions & 9 deletions src/query/management/src/role/role_mgr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ use databend_common_meta_app::app_error::TxnRetryMaxTimes;
use databend_common_meta_app::principal::GrantObject;
use databend_common_meta_app::principal::OwnershipInfo;
use databend_common_meta_app::principal::OwnershipObject;
use databend_common_meta_app::principal::RoleGrantee;
use databend_common_meta_app::principal::RoleInfo;
use databend_common_meta_app::principal::TenantOwnershipObject;
use databend_common_meta_app::principal::UserPrivilegeType;
use databend_common_meta_app::tenant::Tenant;
use databend_common_meta_kvapi::kvapi;
Expand Down Expand Up @@ -107,19 +107,19 @@ impl RoleMgr {
}

/// Build meta-service for a role grantee, which is a tenant's database, table, stage, udf, etc.
fn grantee_key(&self, object: &OwnershipObject) -> String {
let grantee = RoleGrantee::new(Tenant::new(self.tenant.as_str()), object.clone());
fn ownership_object_key(&self, object: &OwnershipObject) -> String {
let grantee = TenantOwnershipObject::new(Tenant::new(self.tenant.as_str()), object.clone());
grantee.to_string_key()
}

/// Build meta-service for a listing keys belongs to the tenant.
///
/// In form of `__fd_object_owners/<tenant>/`.
fn grantee_prefix(&self) -> String {
fn ownership_object_prefix(&self) -> String {
let dummy = OwnershipObject::UDF {
name: "dummy".to_string(),
};
let grantee = RoleGrantee::new(Tenant::new(self.tenant.as_str()), dummy);
let grantee = TenantOwnershipObject::new(Tenant::new(self.tenant.as_str()), dummy);
grantee.tenant_prefix()
}

Expand Down Expand Up @@ -206,7 +206,7 @@ impl RoleApi for RoleMgr {
#[async_backtrace::framed]
#[minitrace::trace]
async fn get_ownerships(&self) -> Result<Vec<SeqV<OwnershipInfo>>, ErrorCode> {
let object_owner_prefix = self.grantee_prefix();
let object_owner_prefix = self.ownership_object_prefix();
let kv_api = self.kv_api.clone();
let values = kv_api.prefix_list_kv(object_owner_prefix.as_str()).await?;

Expand Down Expand Up @@ -267,7 +267,7 @@ impl RoleApi for RoleMgr {
let old_role = self.get_ownership(object).await?.map(|o| o.role);
let grant_object = convert_to_grant_obj(object);

let owner_key = self.grantee_key(object);
let owner_key = self.ownership_object_key(object);

let owner_value = serialize_struct(
&OwnershipInfo {
Expand Down Expand Up @@ -347,7 +347,7 @@ impl RoleApi for RoleMgr {
&self,
object: &OwnershipObject,
) -> databend_common_exception::Result<Option<OwnershipInfo>> {
let key = self.grantee_key(object);
let key = self.ownership_object_key(object);

let res = self.kv_api.get_kv(&key).await?;
let seq_value = match res {
Expand Down Expand Up @@ -376,7 +376,7 @@ impl RoleApi for RoleMgr {
) -> databend_common_exception::Result<()> {
let role = self.get_ownership(object).await?.map(|o| o.role);

let owner_key = self.grantee_key(object);
let owner_key = self.ownership_object_key(object);

let mut if_then = vec![txn_op_del(&owner_key)];
let mut condition = vec![];
Expand Down

0 comments on commit e74f3bb

Please sign in to comment.