feat: add server-side sorting and search for admin organizations endpoint #367
Conversation
…oint - Add sort_by (name, created_at, spend_limit, current_usage) query param - Add sort_order (asc, desc) query param - Add search query param (ILIKE on name and id) - Dynamic SQL with NULLS LAST for nullable columns - Return total count in same query for efficiency
PierreLeGuen
had a problem deploying
to
Cloud API test env
GitHub Actions
Failure
Summary of ChangesHello @PierreLeGuen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the administrative interface for managing organizations by integrating robust server-side sorting and searching capabilities. It allows administrators to efficiently filter and order organization data based on various criteria such as name, creation date, spend limits, and current usage, thereby improving data discoverability and management. The changes span across the API, service, and database layers, ensuring a cohesive and performant implementation, complemented by extensive end-to-end tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Security Review - Critical Issue Found1. LIKE Injection Vulnerability (SECURITY - BLOCKING)Location: The search pattern construction allows LIKE injection attacks: let pattern = format!("%{}%", query); // ❌ VulnerableAttack scenario: A malicious search query like Fix required: // Escape LIKE special characters: %, _, let escaped_query = query
.replace('\\', "\\\\")
.replace('%', "\\%")
.replace('_', "\\_");
let pattern = format!("%{}%", escaped_query);
// Update SQL to include ESCAPE clause:
"WHERE o.is_active = true AND (o.name ILIKE $3 ESCAPE '\\' OR o.id::text ILIKE $3 ESCAPE '\\')",2. Missing Input Validation (SECURITY - HIGH)Location: No length limit on search parameter could cause DoS via memory exhaustion or database performance degradation. Fix required: if let Some(ref search) = params.search {
if search.len() > 255 {
return Err(ErrorResponse::validation_error("Search query too long (max 255 characters)"));
}
}3. Minor: Inefficient unwrap_or_default() Calls (PERFORMANCE - LOW)Location: Lines 369, 376, 382 Call ✅ Positive Notes
🚫 Required Before Merge
Status: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces server-side sorting and searching for the admin organizations endpoint, which is a great feature enhancement. The changes are well-structured across the API, service, and repository layers. The addition of a comprehensive E2E test suite is particularly commendable.
My review focuses on improving the robustness of the new tests and addressing a minor performance regression in the database query execution. Specifically, I've suggested:
- Making the sorting tests more reliable by isolating test data to prevent flakiness.
- Enhancing a test for sorting by usage to ensure it properly validates the functionality.
- Reintroducing concurrent query execution for fetching data and total count to optimize performance.
- A minor refactoring for better code clarity in the repository layer.
All original comments were retained as they are valid and do not contradict or fall under the specific modification criteria of the provided rules. Overall, this is a solid contribution. Addressing these points will further improve the quality and reliability of the new functionality.
| async fn test_admin_list_organizations_sort_by_current_usage() { | ||
| let (server, _guard) = setup_test_server().await; | ||
|
|
||
| let _org1 = setup_org_with_credits(&server, 1000000).await; | ||
| let _org2 = setup_org_with_credits(&server, 2000000).await; | ||
|
|
||
| let response = server | ||
| .get("/v1/admin/organizations?sort_by=current_usage&sort_order=asc") | ||
| .add_header("Authorization", format!("Bearer {}", get_session_id())) | ||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||
| .await; | ||
|
|
||
| assert_eq!(response.status_code(), 200); | ||
| let list_response: ListOrganizationsAdminResponse = | ||
| serde_json::from_str(&response.text()).expect("Failed to parse response"); | ||
|
|
||
| assert!(list_response.organizations.len() >= 2); | ||
| } |
There was a problem hiding this comment.
This test is not currently verifying the sorting logic for current_usage. It only asserts that at least two organizations are returned, which doesn't confirm that they are sorted correctly. To make this test effective, it should:
- Create organizations and then generate some API usage for them to ensure
current_usageis non-zero and varies between them. - Fetch the list sorted by
current_usage. - Assert that the returned organizations are correctly sorted based on their usage.
As it stands, this test provides a false sense of security about the sorting functionality.
| async fn test_admin_list_organizations_sort_by_name_asc() { | ||
| let (server, _guard) = setup_test_server().await; | ||
|
|
||
| let org_names = vec!["zebra-org", "alpha-org", "delta-org"]; | ||
| for name in &org_names { | ||
| let request = api::models::CreateOrganizationRequest { | ||
| name: format!("test-{}-{}", name, uuid::Uuid::new_v4()), | ||
| description: Some(format!("Test organization {}", name)), | ||
| }; | ||
| let response = server | ||
| .post("/v1/organizations") | ||
| .add_header("Authorization", format!("Bearer {}", get_session_id())) | ||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||
| .json(&serde_json::json!(request)) | ||
| .await; | ||
| assert_eq!(response.status_code(), 200); | ||
| } | ||
|
|
||
| let response = server | ||
| .get("/v1/admin/organizations?sort_by=name&sort_order=asc") | ||
| .add_header("Authorization", format!("Bearer {}", get_session_id())) | ||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||
| .await; | ||
|
|
||
| assert_eq!(response.status_code(), 200); | ||
| let list_response: ListOrganizationsAdminResponse = | ||
| serde_json::from_str(&response.text()).expect("Failed to parse response"); | ||
|
|
||
| let names: Vec<String> = list_response | ||
| .organizations | ||
| .iter() | ||
| .map(|o| o.name.clone()) | ||
| .collect(); | ||
|
|
||
| let mut sorted_names = names.clone(); | ||
| sorted_names.sort(); | ||
| assert_eq!( | ||
| names, sorted_names, | ||
| "Organizations should be sorted by name ascending" | ||
| ); | ||
| } |
There was a problem hiding this comment.
This test may be flaky because it asserts the sorting order on the entire list of organizations returned by the API. If other organizations exist in the test database (e.g., from other tests running in parallel or from setup), this assertion can fail. To make the test more robust, you should isolate the test data. A good approach is to create organizations with a unique name prefix and then use the search query parameter to fetch only those specific organizations before asserting the sort order. The test_admin_list_organizations_search_and_sort test already uses this reliable pattern.
| async fn test_admin_list_organizations_sort_by_name_desc() { | ||
| let (server, _guard) = setup_test_server().await; | ||
|
|
||
| for i in 0..3 { | ||
| let request = api::models::CreateOrganizationRequest { | ||
| name: format!("sort-test-{}-{}", i, uuid::Uuid::new_v4()), | ||
| description: Some(format!("Test organization {}", i)), | ||
| }; | ||
| let response = server | ||
| .post("/v1/organizations") | ||
| .add_header("Authorization", format!("Bearer {}", get_session_id())) | ||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||
| .json(&serde_json::json!(request)) | ||
| .await; | ||
| assert_eq!(response.status_code(), 200); | ||
| } | ||
|
|
||
| let response = server | ||
| .get("/v1/admin/organizations?sort_by=name&sort_order=desc") | ||
| .add_header("Authorization", format!("Bearer {}", get_session_id())) | ||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||
| .await; | ||
|
|
||
| assert_eq!(response.status_code(), 200); | ||
| let list_response: ListOrganizationsAdminResponse = | ||
| serde_json::from_str(&response.text()).expect("Failed to parse response"); | ||
|
|
||
| let names: Vec<String> = list_response | ||
| .organizations | ||
| .iter() | ||
| .map(|o| o.name.clone()) | ||
| .collect(); | ||
|
|
||
| let mut sorted_names = names.clone(); | ||
| sorted_names.sort_by(|a, b| b.cmp(a)); | ||
| assert_eq!( | ||
| names, sorted_names, | ||
| "Organizations should be sorted by name descending" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Similar to the ascending sort test, this test can be flaky. It assumes the returned list contains only the organizations created within this test, which may not be true in a shared test environment. To ensure reliability, please isolate the test data by using a unique prefix for organization names and then filtering the results using the search parameter before checking the descending sort order.
| async fn test_admin_list_organizations_sort_by_created_at() { | ||
| let (server, _guard) = setup_test_server().await; | ||
|
|
||
| let mut org_ids = Vec::new(); | ||
| for _i in 0..3 { | ||
| let org = create_org(&server).await; | ||
| org_ids.push(org.id.clone()); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(10)).await; | ||
| } | ||
|
|
||
| let response = server | ||
| .get("/v1/admin/organizations?sort_by=created_at&sort_order=desc") | ||
| .add_header("Authorization", format!("Bearer {}", get_session_id())) | ||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||
| .await; | ||
|
|
||
| assert_eq!(response.status_code(), 200); | ||
| let list_response: ListOrganizationsAdminResponse = | ||
| serde_json::from_str(&response.text()).expect("Failed to parse response"); | ||
|
|
||
| let timestamps: Vec<chrono::DateTime<chrono::Utc>> = list_response | ||
| .organizations | ||
| .iter() | ||
| .map(|o| o.created_at) | ||
| .collect(); | ||
|
|
||
| for i in 0..timestamps.len().saturating_sub(1) { | ||
| assert!( | ||
| timestamps[i] >= timestamps[i + 1], | ||
| "Timestamps should be in descending order" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test is susceptible to flakiness because it fetches and checks the sort order of all organizations, which might include data from other tests. Although you are collecting org_ids, you are not using them to filter the results. To make this test reliable, you should filter the returned organizations to include only the ones created in this test before asserting the created_at sort order. A better approach would be to use a unique prefix and the search parameter to isolate test data from the API side.
| async fn test_admin_list_organizations_default_sort() { | ||
| let (server, _guard) = setup_test_server().await; | ||
|
|
||
| create_org(&server).await; | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(10)).await; | ||
| create_org(&server).await; | ||
|
|
||
| let response = server | ||
| .get("/v1/admin/organizations") | ||
| .add_header("Authorization", format!("Bearer {}", get_session_id())) | ||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||
| .await; | ||
|
|
||
| assert_eq!(response.status_code(), 200); | ||
| let list_response: ListOrganizationsAdminResponse = | ||
| serde_json::from_str(&response.text()).expect("Failed to parse response"); | ||
|
|
||
| let timestamps: Vec<chrono::DateTime<chrono::Utc>> = list_response | ||
| .organizations | ||
| .iter() | ||
| .map(|o| o.created_at) | ||
| .collect(); | ||
|
|
||
| for i in 0..timestamps.len().saturating_sub(1) { | ||
| assert!( | ||
| timestamps[i] >= timestamps[i + 1], | ||
| "Default sort should be by created_at descending" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test for default sorting can be flaky as it relies on the entire list of organizations returned by the API. If other organizations are present in the database, the assertion for created_at descending order might fail. To make this test robust, please isolate the data by creating organizations with a unique prefix and using the search parameter to fetch only those organizations for the assertion.
| let order_column = match order_by.unwrap_or_default() { | ||
| AdminOrganizationOrderBy::CreatedAt => "o.created_at", | ||
| AdminOrganizationOrderBy::Name => "o.name", | ||
| AdminOrganizationOrderBy::SpendLimit => "olh.spend_limit", | ||
| AdminOrganizationOrderBy::CurrentUsage => "ob.total_spent", | ||
| }; | ||
|
|
||
| let order_dir = match order_direction.unwrap_or_default() { | ||
| AdminOrganizationOrderDirection::Asc => "ASC", | ||
| AdminOrganizationOrderDirection::Desc => "DESC", | ||
| }; | ||
|
|
||
| // NULLS LAST for columns that can be null | ||
| let nulls_clause = match order_by.unwrap_or_default() { | ||
| AdminOrganizationOrderBy::SpendLimit | AdminOrganizationOrderBy::CurrentUsage => { | ||
| " NULLS LAST" | ||
| } | ||
| _ => "", | ||
| }; |
There was a problem hiding this comment.
To improve readability and avoid redundant calls, you can call order_by.unwrap_or_default() once and store its result in a variable. This makes the code slightly cleaner and more efficient.
| let order_column = match order_by.unwrap_or_default() { | |
| AdminOrganizationOrderBy::CreatedAt => "o.created_at", | |
| AdminOrganizationOrderBy::Name => "o.name", | |
| AdminOrganizationOrderBy::SpendLimit => "olh.spend_limit", | |
| AdminOrganizationOrderBy::CurrentUsage => "ob.total_spent", | |
| }; | |
| let order_dir = match order_direction.unwrap_or_default() { | |
| AdminOrganizationOrderDirection::Asc => "ASC", | |
| AdminOrganizationOrderDirection::Desc => "DESC", | |
| }; | |
| // NULLS LAST for columns that can be null | |
| let nulls_clause = match order_by.unwrap_or_default() { | |
| AdminOrganizationOrderBy::SpendLimit | AdminOrganizationOrderBy::CurrentUsage => { | |
| " NULLS LAST" | |
| } | |
| _ => "", | |
| }; | |
| let final_order_by = order_by.unwrap_or_default(); | |
| let order_column = match final_order_by { | |
| AdminOrganizationOrderBy::CreatedAt => "o.created_at", | |
| AdminOrganizationOrderBy::Name => "o.name", | |
| AdminOrganizationOrderBy::SpendLimit => "olh.spend_limit", | |
| AdminOrganizationOrderBy::CurrentUsage => "ob.total_spent", | |
| }; | |
| let order_dir = match order_direction.unwrap_or_default() { | |
| AdminOrganizationOrderDirection::Asc => "ASC", | |
| AdminOrganizationOrderDirection::Desc => "DESC", | |
| }; | |
| // NULLS LAST for columns that can be null | |
| let nulls_clause = match final_order_by { | |
| AdminOrganizationOrderBy::SpendLimit | AdminOrganizationOrderBy::CurrentUsage => { | |
| " NULLS LAST" | |
| } | |
| _ => "", | |
| }; |
| let (rows, count_row) = if let Some(ref pattern) = search_pattern { | ||
| let rows = client.query(&query, &[&limit, &offset, pattern]).await?; | ||
| let count_row = client.query_one(&count_query, &[pattern]).await?; | ||
| (rows, count_row) | ||
| } else { | ||
| let rows = client.query(&query, &[&limit, &offset]).await?; | ||
| let count_row = client.query_one(&count_query, &[]).await?; | ||
| (rows, count_row) | ||
| }; |
There was a problem hiding this comment.
The main query and the count query are executed sequentially. This is a performance regression, as the previous implementation executed them concurrently. You can use tokio::join! to run these independent database queries in parallel, which will improve the endpoint's response time.
| let (rows, count_row) = if let Some(ref pattern) = search_pattern { | |
| let rows = client.query(&query, &[&limit, &offset, pattern]).await?; | |
| let count_row = client.query_one(&count_query, &[pattern]).await?; | |
| (rows, count_row) | |
| } else { | |
| let rows = client.query(&query, &[&limit, &offset]).await?; | |
| let count_row = client.query_one(&count_query, &[]).await?; | |
| (rows, count_row) | |
| }; | |
| let (rows, count_row) = if let Some(ref pattern) = search_pattern { | |
| let rows_fut = client.query(&query, &[&limit, &offset, pattern]); | |
| let count_fut = client.query_one(&count_query, &[pattern]); | |
| let (rows_result, count_row_result) = tokio::join!(rows_fut, count_fut); | |
| (rows_result?, count_row_result?) | |
| } else { | |
| let rows_fut = client.query(&query, &[&limit, &offset]); | |
| let count_fut = client.query_one(&count_query, &[]); | |
| let (rows_result, count_row_result) = tokio::join!(rows_fut, count_fut); | |
| (rows_result?, count_row_result?) | |
| }; |
Summary
sort_byquery param:name,created_at,spend_limit,current_usagesort_orderquery param:asc,descsearchquery param (ILIKE on name and org ID)NULLS LASTfor nullable columnsTest plan