Add new endpoint for delete vault#104
Conversation
WalkthroughA new HTTP DELETE endpoint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Storage
Client->>Server: DELETE /vault/:pluginId/:publicKeyECDSA
Server->>Server: Validate pluginId and publicKeyECDSA
alt Invalid parameters
Server-->>Client: 400 Bad Request
else Valid parameters
Server->>Storage: Delete vault backup file
alt Deletion fails
Server-->>Client: 500 Internal Server Error
else Deletion succeeds
Server-->>Client: 200 OK (no content)
end
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Pull Request Overview
Adds a new HTTP DELETE endpoint to remove an existing vault backup file.
- Registers
DELETE /:pluginId/:publicKeyECDSAroute inStartServer - Implements
DeleteVaulthandler with parameter validation and file deletion logic - Returns appropriate HTTP status codes on success or failure
Comments suppressed due to low confidence (2)
api/server.go:306
- [nitpick] Instead of returning an empty 400 response, use
c.JSONwith an error message for consistency with other validation errors.
return c.NoContent(http.StatusBadRequest)
api/server.go:300
- No tests were added for the new
DeleteVaulthandler—consider adding unit and integration tests to cover successful deletion and various error paths.
func (s *Server) DeleteVault(c echo.Context) error {
![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
🧹 Nitpick comments (1)
api/server.go (1)
300-318: Consider adding error logging and improving error handling.The implementation is consistent with other vault methods, but could benefit from the following improvements:
- Missing error logging: Unlike other methods in this file, deletion failures aren't logged, which could make debugging difficult.
- Error message exposure: The error from
DeleteFileis directly returned to the client, potentially exposing internal implementation details.Apply this diff to improve error handling:
fileName := vcommon.GetVaultBackupFilename(publicKeyECDSA, pluginId) if err := s.vaultStorage.DeleteFile(fileName); err != nil { + s.logger.WithError(err).Error("fail to delete vault file") - return c.JSON(http.StatusInternalServerError, NewErrorResponse(err.Error())) + return c.JSON(http.StatusInternalServerError, NewErrorResponse("fail to delete vault")) }Also, consider whether authorization checks are needed to ensure the caller has permission to delete this specific vault.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/server.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/server.go (1)
api/plugin.go (1)
NewErrorResponse(32-36)
🔇 Additional comments (1)
api/server.go (1)
115-115: LGTM! Route registration follows established patterns.The DELETE endpoint is properly registered in the vault group with a clear comment and consistent URL pattern matching other vault operations.
Summary by CodeRabbit