refactor(api_admin)!: use query instead of body #69
Conversation
WalkthroughThe changes involve a significant shift in how account-related data is processed within the API. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Database
Client->>API: POST /create-account?name=example.com&token=abc123
API->>Database: Create account with name example.com
Database-->>API: Account created
API-->>Client: 201 Created
sequenceDiagram
participant Client
participant API
participant Database
Client->>API: POST /remove-account?name=example.com&token=abc123
API->>Database: Remove account with name example.com
Database-->>API: Account removed
API-->>Client: 200 OK
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
crates/api_admin/src/entities/create_remove_account.rs (2)
4-5: LGTM: Struct changes align with query parameter usage.The modifications to the struct are consistent with the PR objective:
- Changing
#[derive(ToSchema)]to#[derive(IntoParams)]supports the use of query parameters.- Renaming the struct to
CreateRemoveAccountQueryclearly indicates its purpose.These changes enhance code clarity and maintain the expected functionality.
Consider using separate structs for creating and removing accounts (e.g.,
CreateAccountQueryandRemoveAccountQuery) to improve code clarity and adhere to the Single Responsibility Principle. This change would make the code more maintainable and easier to understand.
Line range hint
1-15: Summary: Successful refactoring to use query parametersThe changes in this file effectively implement the PR objective of using query parameters instead of a JSON body for account-related operations. Key points:
- The import and derive attribute changes support the use of
IntoParamsfor query parameter handling.- The struct renaming enhances code clarity.
These modifications improve the API's adherence to RESTful practices and make the code more maintainable. The transition appears smooth and well-executed.
Consider splitting the
CreateRemoveAccountQueryinto separate structs for creation and removal operations in the future. This would further improve code organization and make it easier to add operation-specific fields if needed.docs/src/admins/create-account.md (1)
24-24: LGTM! Consider simplifying the command.The updated curl command correctly reflects the change from using a JSON body to query parameters, which aligns with the PR objective. This change makes the API usage more straightforward and consistent with RESTful practices.
Consider simplifying the command by removing the unnecessary
echocalls:NAME="example.com" curl -X POST "http://localhost:$HATSU_LISTEN_PORT/api/v0/admin/create-account?name=$NAME&token=$HATSU_ACCESS_TOKEN"This change makes the command slightly more readable while maintaining the same functionality.
crates/api_admin/src/routes/create_account.rs (1)
30-30: LGTM: Function updated to use query parameters consistently.The changes in the function signature and implementation correctly reflect the shift from JSON payload to query parameters. The user URL generation and account creation logic have been updated accordingly.
Consider updating the error message on line 40 to reference the query parameter instead of an account:
- format!("The account already exists: {}", account.name), + format!("An account with the name '{}' already exists", query.name),This change would make the error message more consistent with the new query parameter approach.
Also applies to: 33-33, 44-44
crates/api_admin/src/routes/remove_account.rs (2)
28-28: LGTM: Function updated consistently to use query parameters.The changes in the function signature and body correctly implement the shift from JSON payload to query parameters. All references to
payload.namehave been appropriately replaced withquery.name, and error messages have been updated accordingly.Consider adding input validation for the
query.nameparameter to ensure it meets any required format or length constraints before processing the request. This can help prevent potential issues with invalid input early in the request handling process.Example:
if !is_valid_account_name(&query.name) { return Err(AppError::new( format!("Invalid account name: {}", query.name), None, Some(StatusCode::BAD_REQUEST), )); }Also applies to: 31-31, 51-51, 53-53, 59-59
Line range hint
1-64: Implement actual account removal functionality.While the changes to use query parameters instead of JSON payload have been implemented correctly, the core functionality of account removal is still not implemented. The function continues to return a "not implemented" response.
Consider the following steps to complete the implementation:
- Implement the actual account removal logic, replacing the TODO comment.
- Update the response to reflect the success or failure of the account removal process.
- Add appropriate logging for the account removal action.
- Consider implementing a confirmation step or a "dry run" option to prevent accidental account removals.
- Ensure that all associated data (posts, followers, etc.) are handled appropriately during account removal.
Example skeleton for account removal:
// Inside the Some(account) branch let result = remove_account_data(&data.conn, &account).await?; if result.is_success() { Ok(( StatusCode::OK, Json(CreateRemoveAccountResult { name: query.name.clone(), message: format!("Successfully removed account: {}", query.name), }), )) } else { Err(AppError::new( format!("Failed to remove account: {}", query.name), None, Some(StatusCode::INTERNAL_SERVER_ERROR), )) }Don't forget to remove the TODO comment once the implementation is complete.
Justfile (1)
75-75: Approve change with considerationsThe modification aligns with the PR objective of using query parameters instead of the request body. However, there are some important considerations:
Using POST with query parameters is unconventional. Consider changing the HTTP method to GET for consistency with RESTful practices, especially since the data is now in the URL.
Passing the access token as a query parameter may pose a security risk. Query parameters are often logged in server logs and can appear in browser history. Consider using a header (e.g.,
Authorization) for the token instead.This change might affect API compatibility. Ensure that all clients are updated to use the new parameter passing method.
To address these concerns, consider the following changes:
- curl -X POST "http://localhost:${HATSU_LISTEN_PORT}/api/v0/admin/{{method}}-account?name={{name}}&token=${HATSU_ACCESS_TOKEN}" + curl -X GET "http://localhost:${HATSU_LISTEN_PORT}/api/v0/admin/{{method}}-account?name={{name}}" \ + -H "Authorization: Bearer ${HATSU_ACCESS_TOKEN}"This suggestion uses GET (which is more appropriate for query parameters), moves the token to the Authorization header, and keeps the account name as a query parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- Justfile (1 hunks)
- crates/api_admin/src/entities/create_remove_account.rs (1 hunks)
- crates/api_admin/src/entities/mod.rs (1 hunks)
- crates/api_admin/src/routes/create_account.rs (4 hunks)
- crates/api_admin/src/routes/mod.rs (1 hunks)
- crates/api_admin/src/routes/remove_account.rs (4 hunks)
- docs/src/admins/create-account.md (1 hunks)
🔇 Additional comments (11)
crates/api_admin/src/entities/mod.rs (1)
5-5: Approve renaming toCreateRemoveAccountQuerywith follow-up actions.The renaming from
CreateRemoveAccounttoCreateRemoveAccountQueryaligns well with the PR objective of using queries instead of body parameters. This change enhances clarity and consistency in the API design.However, this modification has some important implications:
- It's a breaking change that will affect any code directly using
CreateRemoveAccount.- The new name is consistent with
BlockUrlQuery, suggesting a pattern for naming query structs.To ensure all references are updated and to identify any potential issues, please run the following verification script:
This script will help identify any areas that might need attention due to this renaming.
crates/api_admin/src/entities/create_remove_account.rs (1)
2-2: LGTM: Import change aligns with query parameter usage.The change from
ToSchematoIntoParamsis consistent with the PR objective of using query parameters instead of a JSON body. This modification supports the new approach for handling account-related data in the API.crates/api_admin/src/routes/create_account.rs (3)
4-4: LGTM: Import changes align with new query parameter approach.The changes in the imports correctly reflect the shift from using a JSON payload to query parameters for input handling.
11-11: LGTM: Entity import updated to reflect query parameter usage.The change from
CreateRemoveAccounttoCreateRemoveAccountQueryaccurately represents the new approach of using query parameters for input.
20-20: LGTM: API documentation updated to reflect query parameter usage.The
paramsattribute in the API documentation has been correctly updated to useCreateRemoveAccountQuery, ensuring consistency with the new input method.crates/api_admin/src/routes/mod.rs (3)
Line range hint
47-73: Changes are consistent with existing auth and routing logic.The removal of
CreateRemoveAccountand the corresponding OpenAPI schema changes are consistent with the existing authentication middleware and route definitions:
- The auth middleware still correctly checks for a token in query parameters, which aligns with the new approach of using query parameters for account operations.
- The route definitions remain unchanged, indicating that the API structure is maintained while only the internal handling of account operations has been modified.
- The security scheme definition is appropriately left unchanged, as the authentication method remains the same.
These observations confirm that the changes have been implemented consistently throughout the file.
Line range hint
25-29: OpenAPI schema components updated to reflect API changes.The removal of
CreateRemoveAccountfrom the OpenAPI schema components is consistent with the changes in the import statement and the PR objective. This update ensures that the API documentation accurately reflects the new approach of using query parameters for account operations.Let's verify that the routes for create_account and remove_account are still present and haven't been accidentally removed:
#!/bin/bash # Description: Verify that create_account and remove_account routes are still present # Test: Search for create_account and remove_account route definitions. Expect: Both routes should be present. rg --type rust "routes!\(create_account::create_account\)" crates/api_admin/src/routes/mod.rs rg --type rust "routes!\(remove_account::remove_account\)" crates/api_admin/src/routes/mod.rs
16-16: Removal ofCreateRemoveAccountimport aligns with PR objective.The removal of
CreateRemoveAccountfrom the imports is consistent with the PR objective of using query parameters instead of request body for account operations. This change suggests that the structure is no longer needed for processing account creation and removal requests.Let's verify that
CreateRemoveAccountis not used elsewhere in the codebase:✅ Verification successful
CreateRemoveAccounthas been successfully removed from the codebase.The
CreateRemoveAccountentity is no longer present or used elsewhere, confirming that the removal aligns with the PR objective of handling account operations via query parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of CreateRemoveAccount in the codebase # Test: Search for CreateRemoveAccount usage. Expect: No results. rg --type rust "CreateRemoveAccount"Length of output: 305
crates/api_admin/src/routes/remove_account.rs (3)
2-2: LGTM: Import changes align with new query parameter approach.The changes in the imports correctly reflect the shift from using JSON payload to query parameters for the
remove_accountfunction.
8-8: LGTM: Entity import updated to reflect query-based approach.The renaming of
CreateRemoveAccounttoCreateRemoveAccountQueryaccurately represents the new query-based structure for account removal requests.
17-17: LGTM: API documentation updated to reflect query parameters.The
paramsattribute in the API documentation has been correctly updated to useCreateRemoveAccountQuery, ensuring consistency with the new query-based approach.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation