refactor(tenant): use tenant id type #6643
Conversation
apoorvdixit88
added
A-core
apoorvdixit88
requested review from
jagan-jaya,
jarnura,
SanchithHegde,
dracarys18,
racnan and
ThisIsMani
apoorvdixit88
changed the title
crates/router/src/core/user.rs
Outdated
| @@ -1105,7 +1105,8 @@ pub async fn create_internal_user( | |||
| } | |||
| })?; | |||
|
|
|||
| let default_tenant_id = common_utils::consts::DEFAULT_TENANT.to_string(); | |||
| let default_tenant_id = | |||
| common_utils::id_type::TenantId::new_unchecked(common_utils::consts::DEFAULT_TENANT); | |||
There was a problem hiding this comment.
Unchecked we can use for internal purpose like for the const we will be declaring. With this we don't need to handle the errors. But its not required we can change.
crates/router/src/db/events.rs
Outdated
| @@ -732,7 +732,10 @@ mod tests { | |||
| )) | |||
| .await; | |||
| let state = &Arc::new(app_state) | |||
| .get_session_state("public", || {}) | |||
| .get_session_state( | |||
| &common_utils::id_type::TenantId::wrap("public".to_string()).unwrap(), | |||
There was a problem hiding this comment.
why is const not used here ?
There was a problem hiding this comment.
Its test, i guess we can hard code the value, as it was before.
crates/router/src/services/api.rs
Outdated
| let request_tenant_id = common_utils::id_type::TenantId::wrap( | ||
| incoming_request_header | ||
| .get(TENANT_HEADER) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .ok_or_else(|| errors::ApiErrorResponse::MissingTenantId.switch())? | ||
| .to_string(), | ||
| ) | ||
| .change_context( | ||
| errors::ApiErrorResponse::InvalidRequestData { | ||
| message: format!("`{}` header is invalid", headers::X_TENANT_ID), | ||
| } |
There was a problem hiding this comment.
can this be simplified? string -> cow -> id_type
|
|
||
| impl TenantId { | ||
| /// Get tenant id from String | ||
| pub fn wrap(tenant_id: String) -> CustomResult<Self, ValidationError> { |
There was a problem hiding this comment.
Nit: Can we use a better name which indicates what it's being constructed from, say try_from_string() maybe?
There was a problem hiding this comment.
Yes we can, for org id we follow the wrap(), i just thought to keep things consistent. Shall i change it there as well?
| } | ||
|
|
||
| /// Create a tenant ID without check | ||
| pub fn new_unchecked(tenant_id: &str) -> Self { |
There was a problem hiding this comment.
Do we necessarily need the unchecked, or can we avoid it?
My concern is that keeping a public unchecked constructor allows the possibility of invalid data entering our system.
There was a problem hiding this comment.
We don't necessary need unchecked, its just that it can be used for internal use cases. We don't need to handle the errors for the constants we will be defining.
But yes it can add up to possibility of invalid data, i will try to remove this.
apoorvdixit88
dismissed stale reviews from racnan and jagan-jaya
via
bcfcec4
Type of Change
Description
Use tenant_id type for better type safety
Additional Changes
Motivation and Context
Closes #6642
How did you test it?
It refactoring PR, its compiling and checks are passing
Tested flows like signup/signin with tenancy featue flag enabled and disabled in local
Checklist
cargo +nightly fmt --allcargo clippy