-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(themes): Create APIs for managing themes #6658
Conversation
crates/router/src/core/user/theme.rs
Outdated
let file = theme_utils::retrieve_file_from_theme_bucket( | ||
&state, | ||
&theme_utils::get_theme_file_key(&db_theme.theme_id), | ||
) | ||
.await?; | ||
|
||
let parsed_data = | ||
serde_json::from_slice(&file).change_context(UserErrors::InternalServerError)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't / Shouldn't the uploaded data be the same as request.theme_data
? Do we necessarily need to retrieve the file and deserialize it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uploaded data should be same (in theory). But I don't want to completely trust that. I retrieved the data again so that caller can make sure that data is correctly set in S3 and get_theme
API wouldn't respond with different data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
) -> UserResponse<()> { | ||
let db_theme = state | ||
.global_store | ||
.find_theme_by_lineage(request.lineage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find_by_theme_id could be a better choice here.
|
||
let new_theme = ThemeNew::new( | ||
Uuid::new_v4().to_string(), | ||
request.theme_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sanitize the string. Shouldnt be empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will do when user APIs for themes are introduced.
theme_utils::upload_file_to_theme_bucket( | ||
&state, | ||
&theme_utils::get_theme_file_key(&db_theme.theme_id), | ||
request | ||
.theme_data | ||
.encode_to_vec() | ||
.change_context(UserErrors::InternalServerError)?, | ||
) | ||
.await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will you handle if upload fails but DB inserts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caller can hit update API to change only the theme, which will upload the theme in S3.
.await | ||
.to_not_found_response(UserErrors::ThemeNotFound)?; | ||
|
||
// TODO (#6717): Delete theme folder from the theme storage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we cannot do delete, can we rename the directory in s3 and prefix/suffix it with some deleted identifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will check this.
pub async fn validate_lineage(state: &SessionState, lineage: &ThemeLineage) -> UserResult<()> { | ||
match lineage { | ||
ThemeLineage::Organization { tenant_id, org_id } => { | ||
validate_tenant(state, tenant_id)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be chained using async_map
and stuff.
} | ||
|
||
pub fn get_theme_file_key(theme_id: &str) -> PathBuf { | ||
get_specific_file_key(theme_id, "theme.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are used only one time. Will move them once we are using it one more time.
Type of Change
Description
This PR adds APIs to manage themes.
These are the following APIs
Additional Changes
todo!()
Motivation and Context
Closes #6553
How did you test it?
Responses
Checklist
cargo +nightly fmt --all
cargo clippy