Skip to content

dashboard changes #1362

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/handlers/http/modal/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,20 @@ impl Server {
.authorize(Action::ListDashboard),
),
)
.service(
web::resource("/list_tags").route(
web::get()
.to(dashboards::list_tags)
.authorize(Action::ListDashboard),
),
)
.service(
web::resource("/list_by_tag/{tag}").route(
web::get()
.to(dashboards::list_dashboards_by_tag)
.authorize(Action::ListDashboard),
),
)
.service(
web::scope("/{dashboard_id}")
.service(
Expand Down
116 changes: 96 additions & 20 deletions src/handlers/http/users/dashboards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*
*/

use std::collections::HashMap;

use crate::{
handlers::http::rbac::RBACError,
storage::ObjectStorageError,
Expand Down Expand Up @@ -68,37 +70,88 @@ pub async fn create_dashboard(
pub async fn update_dashboard(
req: HttpRequest,
dashboard_id: Path<String>,
Json(mut dashboard): Json<Dashboard>,
dashboard: Option<Json<Dashboard>>,
) -> Result<impl Responder, DashboardError> {
let user_id = get_hash(&get_user_from_request(&req)?);
let dashboard_id = validate_dashboard_id(dashboard_id.into_inner())?;
let mut existing_dashboard = DASHBOARDS
.get_dashboard_by_user(dashboard_id, &user_id)
.await
.ok_or(DashboardError::Metadata(
"Dashboard does not exist or user is not authorized",
))?;

// Validate all tiles have valid IDs
if let Some(tiles) = &dashboard.tiles {
if tiles.iter().any(|tile| tile.tile_id.is_nil()) {
return Err(DashboardError::Metadata("Tile ID must be provided"));
}
let query_map = web::Query::<HashMap<String, String>>::from_query(req.query_string())
.map_err(|_| DashboardError::InvalidQueryParameter)?;

// Validate: either query params OR body, not both
let has_query_params = !query_map.is_empty();
let has_body_update = dashboard
.as_ref()
.is_some_and(|d| d.title != existing_dashboard.title || d.tiles.is_some());

if has_query_params && has_body_update {
return Err(DashboardError::Metadata(
"Cannot use both query parameters and request body for updates",
));
}

// Check if tile_id are unique
if let Some(tiles) = &dashboard.tiles {
let unique_tiles: Vec<_> = tiles
.iter()
.map(|tile| tile.tile_id)
.collect::<std::collections::HashSet<_>>()
.into_iter()
.collect();

if unique_tiles.len() != tiles.len() {
return Err(DashboardError::Metadata("Tile IDs must be unique"));
let mut final_dashboard = if has_query_params {
// Apply partial updates from query parameters
if let Some(is_favorite) = query_map.get("isFavorite") {
existing_dashboard.is_favorite = Some(is_favorite == "true");
}
}
if let Some(tags) = query_map.get("tags") {
let parsed_tags: Vec<String> = tags
.split(',')
.map(|s| s.trim())
.filter(|s| !s.is_empty())
.map(|s| s.to_string())
.collect();
existing_dashboard.tags = if parsed_tags.is_empty() {
None
} else {
Some(parsed_tags)
};
}
if let Some(rename_to) = query_map.get("renameTo") {
let trimmed = rename_to.trim();
if trimmed.is_empty() {
return Err(DashboardError::Metadata("Rename to cannot be empty"));
}
existing_dashboard.title = trimmed.to_string();
}
existing_dashboard
} else {
let dashboard = dashboard
.ok_or(DashboardError::Metadata("Request body is required"))?
.into_inner();
if let Some(tiles) = &dashboard.tiles {
if tiles.iter().any(|tile| tile.tile_id.is_nil()) {
return Err(DashboardError::Metadata("Tile ID must be provided"));
}

// Check if tile_id are unique
let unique_tiles: Vec<_> = tiles
.iter()
.map(|tile| tile.tile_id)
.collect::<std::collections::HashSet<_>>()
.into_iter()
.collect();

if unique_tiles.len() != tiles.len() {
return Err(DashboardError::Metadata("Tile IDs must be unique"));
}
}

dashboard
};

DASHBOARDS
.update(&user_id, dashboard_id, &mut dashboard)
.update(&user_id, dashboard_id, &mut final_dashboard)
.await?;

Ok((web::Json(dashboard), StatusCode::OK))
Ok((web::Json(final_dashboard), StatusCode::OK))
}

pub async fn delete_dashboard(
Expand Down Expand Up @@ -145,6 +198,26 @@ pub async fn add_tile(
Ok((web::Json(dashboard), StatusCode::OK))
}

pub async fn list_tags() -> Result<impl Responder, DashboardError> {
let tags = DASHBOARDS.list_tags().await;
Ok((web::Json(tags), StatusCode::OK))
}

pub async fn list_dashboards_by_tag(tag: Path<String>) -> Result<impl Responder, DashboardError> {
let tag = tag.into_inner();
if tag.is_empty() {
return Err(DashboardError::Metadata("Tag cannot be empty"));
}

let dashboards = DASHBOARDS.list_dashboards_by_tag(&tag).await;
let dashboard_summaries = dashboards
.iter()
.map(|dashboard| dashboard.to_summary())
.collect::<Vec<_>>();

Ok((web::Json(dashboard_summaries), StatusCode::OK))
}

#[derive(Debug, thiserror::Error)]
pub enum DashboardError {
#[error("Failed to connect to storage: {0}")]
Expand All @@ -159,6 +232,8 @@ pub enum DashboardError {
Custom(String),
#[error("Dashboard does not exist or is not accessible")]
Unauthorized,
#[error("Invalid query parameter")]
InvalidQueryParameter,
}

impl actix_web::ResponseError for DashboardError {
Expand All @@ -170,6 +245,7 @@ impl actix_web::ResponseError for DashboardError {
Self::UserDoesNotExist(_) => StatusCode::NOT_FOUND,
Self::Custom(_) => StatusCode::INTERNAL_SERVER_ERROR,
Self::Unauthorized => StatusCode::UNAUTHORIZED,
Self::InvalidQueryParameter => StatusCode::BAD_REQUEST,
}
}

Expand Down
85 changes: 80 additions & 5 deletions src/users/dashboards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,16 @@ pub struct Tile {
pub other_fields: Option<serde_json::Map<String, Value>>,
}
#[derive(Debug, Serialize, Deserialize, Default, Clone)]
#[serde(rename_all = "camelCase")]
pub struct Dashboard {
pub version: Option<String>,
pub title: String,
pub author: Option<String>,
pub dashboard_id: Option<Ulid>,
pub created: Option<DateTime<Utc>>,
pub modified: Option<DateTime<Utc>>,
pub tags: Option<Vec<String>>,
pub is_favorite: Option<bool>, // whether the dashboard is marked as favorite, default is false
dashboard_type: Option<DashboardType>,
pub tiles: Option<Vec<Tile>>,
}
Expand All @@ -77,6 +81,9 @@ impl Dashboard {
if self.tiles.is_none() {
self.tiles = Some(Vec::new());
}

// if is_favorite is None, set it to false, else set it to the current value
self.is_favorite = self.is_favorite.or(Some(false));
}

/// create a summary of the dashboard
Expand All @@ -96,6 +103,13 @@ impl Dashboard {
);
}

if let Some(created) = &self.created {
map.insert(
"created".to_string(),
serde_json::Value::String(created.to_string()),
);
}

if let Some(modified) = &self.modified {
map.insert(
"modified".to_string(),
Expand All @@ -105,11 +119,27 @@ impl Dashboard {

if let Some(dashboard_id) = &self.dashboard_id {
map.insert(
"dashboard_id".to_string(),
"dashboardId".to_string(),
serde_json::Value::String(dashboard_id.to_string()),
);
}

if let Some(tags) = &self.tags {
map.insert(
"tags".to_string(),
serde_json::Value::Array(
tags.iter()
.map(|tag| serde_json::Value::String(tag.clone()))
.collect(),
),
);
}

map.insert(
"isFavorite".to_string(),
serde_json::Value::Bool(self.is_favorite.unwrap_or(false)),
);

map
}
}
Expand Down Expand Up @@ -175,6 +205,16 @@ impl Dashboards {
let dashboard_id = dashboard
.dashboard_id
.ok_or(DashboardError::Metadata("Dashboard ID must be provided"))?;

// ensure the dashboard has unique title
let dashboards = self.0.read().await;
let has_duplicate = dashboards
.iter()
.any(|d| d.title == dashboard.title && d.dashboard_id != dashboard.dashboard_id);

if has_duplicate {
return Err(DashboardError::Metadata("Dashboard title must be unique"));
}
Comment on lines +209 to +217
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Potential race condition in title uniqueness validation.

The uniqueness check has a race condition where concurrent operations could both pass validation and create duplicate titles. However, this may be acceptable given the typical usage patterns of dashboard creation.

The validation logic correctly excludes the same dashboard when checking for duplicates during updates.

Consider if stricter concurrency control is needed:


🏁 Script executed:

#!/bin/bash
# Description: Check if there are other places where concurrent dashboard operations might conflict
# Expected: Find patterns of concurrent dashboard access that might need synchronization

rg -A 5 -B 5 "dashboards.*write.*await|write.*await.*dashboards" --type rust

Length of output: 707


Hold write lock during title‐uniqueness check and insertion

Currently, in src/users/dashboards.rs (around lines 209–217), the code does:

// ensure the dashboard has unique title
let dashboards = self.0.read().await;
let has_duplicate = dashboards
    .iter()
    .any(|d| d.title == dashboard.title && d.dashboard_id != dashboard.dashboard_id);

if has_duplicate {
    return Err(DashboardError::Metadata("Dashboard title must be unique"));
}

// …later…
let mut dashboards = self.0.write().await;
dashboards.retain(|d| d.dashboard_id != dashboard.dashboard_id);
dashboards.push(dashboard.clone());

Between the read lock and the write lock, another concurrent create/update can slip in, leading to duplicate titles. To eliminate this window:

  • Acquire the write lock first, then do both the duplicate check and the insertion under that guard.
  • Example diff:
- // ensure the dashboard has unique title
- let dashboards = self.0.read().await;
- let has_duplicate = dashboards
-     .iter()
-     .any(|d| d.title == dashboard.title && d.dashboard_id != dashboard.dashboard_id);
+ // atomically check and insert under write lock
+ let mut dashboards = self.0.write().await;
+ let has_duplicate = dashboards
+     .iter()
+     .any(|d| d.title == dashboard.title && d.dashboard_id != dashboard.dashboard_id);

  if has_duplicate {
      return Err(DashboardError::Metadata("Dashboard title must be unique"));
  }

- // …later under a separate write lock…
- let mut dashboards = self.0.write().await;
- dashboards.retain(|d| d.dashboard_id != dashboard.dashboard_id);
- dashboards.push(dashboard.clone());
+ // now that we've validated under the same lock:
+ dashboards.retain(|d| d.dashboard_id != dashboard.dashboard_id);
+ dashboards.push(dashboard.clone());

Apply this pattern to both the create and update code paths so that checking for duplicates and modifying the collection is a single atomic operation.

🤖 Prompt for AI Agents
In src/users/dashboards.rs around lines 209 to 217, the uniqueness check for
dashboard titles is done under a read lock, but the insertion happens later
under a write lock, allowing a race condition. To fix this, acquire the write
lock before checking for duplicates and perform both the duplicate check and the
insertion while holding the write lock, ensuring atomicity and preventing
concurrent modifications from causing duplicate titles.

let path = dashboard_path(user_id, &format!("{dashboard_id}.json"));

let store = PARSEABLE.storage.get_object_store();
Expand All @@ -194,6 +234,7 @@ impl Dashboards {
user_id: &str,
dashboard: &mut Dashboard,
) -> Result<(), DashboardError> {
dashboard.created = Some(Utc::now());
dashboard.set_metadata(user_id, None);

self.save_dashboard(user_id, dashboard).await?;
Expand All @@ -211,10 +252,12 @@ impl Dashboards {
dashboard_id: Ulid,
dashboard: &mut Dashboard,
) -> Result<(), DashboardError> {
self.ensure_dashboard_ownership(dashboard_id, user_id)
let existing_dashboard = self
.ensure_dashboard_ownership(dashboard_id, user_id)
.await?;

dashboard.set_metadata(user_id, Some(dashboard_id));
dashboard.created = existing_dashboard.created;
self.save_dashboard(user_id, dashboard).await?;

let mut dashboards = self.0.write().await;
Expand Down Expand Up @@ -288,6 +331,35 @@ impl Dashboards {
self.0.read().await.clone()
}

/// List tags from all dashboards
/// This function returns a list of unique tags from all dashboards
pub async fn list_tags(&self) -> Vec<String> {
let dashboards = self.0.read().await;
let mut tags = dashboards
.iter()
.filter_map(|d| d.tags.as_ref())
.flat_map(|t| t.iter().cloned())
.collect::<Vec<String>>();
tags.sort();
tags.dedup();
tags
}

/// List dashboards by tag
/// This function returns a list of dashboards that have the specified tag
pub async fn list_dashboards_by_tag(&self, tag: &str) -> Vec<Dashboard> {
let dashboards = self.0.read().await;
dashboards
.iter()
.filter(|d| {
d.tags
.as_ref()
.is_some_and(|tags| tags.contains(&tag.to_string()))
})
.cloned()
.collect()
}

/// Ensure the user is the owner of the dashboard
/// This function is called when updating or deleting a dashboard
/// check if the user is the owner of the dashboard
Expand All @@ -296,10 +368,13 @@ impl Dashboards {
&self,
dashboard_id: Ulid,
user_id: &str,
) -> Result<(), DashboardError> {
) -> Result<Dashboard, DashboardError> {
self.get_dashboard_by_user(dashboard_id, user_id)
.await
.ok_or_else(|| DashboardError::Unauthorized)
.map(|_| ())
.ok_or_else(|| {
DashboardError::Metadata(
"Dashboard does not exist or you do not have permission to access it",
)
})
}
}
Loading