Alias to an existing peer_id or an existing alias should fail #6589
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the alias management system within the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (1)
hoprd/rest-api/src/lib.rs (1)
Line range hint
449-453: Map error variants to appropriate HTTP status codes.Currently, all API errors return a 500 Internal Server Error status code. For better HTTP semantics, consider mapping error variants to appropriate status codes. The new
AliasOrPeerIdAliasAlreadyExistserror should return 409 Conflict as specified in the PR objectives.Here's a suggested implementation:
impl IntoResponse for ApiError { fn into_response(self) -> Response { - (StatusCode::INTERNAL_SERVER_ERROR, self).into_response() + let status = match self.status.as_str() { + "ALIAS_OR_PEER_ID_ALIAS_ALREADY_EXISTS" => StatusCode::CONFLICT, + "INVALID_INPUT" => StatusCode::BAD_REQUEST, + "UNAUTHORIZED" => StatusCode::UNAUTHORIZED, + "NOT_FOUND" => StatusCode::NOT_FOUND, + _ => StatusCode::INTERNAL_SERVER_ERROR, + }; + (status, self).into_response() } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
hoprd/db/api/src/aliases.rs(2 hunks)hoprd/db/api/src/errors.rs(1 hunks)hoprd/rest-api/src/alias.rs(2 hunks)hoprd/rest-api/src/lib.rs(1 hunks)tests/test_integration.py(1 hunks)
🧰 Additional context used
🪛 Ruff
tests/test_integration.py
88-88: Line too long (124 > 120)
(E501)
🔇 Additional comments (3)
hoprd/db/api/src/errors.rs (1)
9-10: LGTM! The new error variant is well-placed and supports the uniqueness constraints.
The AliasOrPeerIdAlreadyExists variant with its static error message is appropriately placed in the database layer and aligns well with the PR's objective of preventing duplicate aliases.
hoprd/rest-api/src/alias.rs (1)
87-87: LGTM: Error description accurately reflects both conflict cases.
The updated error description properly documents both conflict scenarios: existing alias and already aliased peer_id, aligning with the bi-directional mapping requirements.
Let's verify the error description matches the implementation:
✅ Verification successful
Let me gather more context about the error handling implementation to ensure full verification.
Let me try a different pattern to get the set_alias implementation.
Let me try one final search pattern to locate the implementation.
Based on the evidence gathered from the shell scripts, I can now provide the final verification:
✓ Error description matches implementation
The error description accurately reflects the implementation:
- The error variant
AliasOrPeerIdAliasAlreadyExistsin the codebase confirms handling of both cases - The error is properly mapped to HTTP 409 (Conflict) status code
- The error propagation chain from DB layer (
DbError::AliasOrPeerIdAlreadyExists) to API response is consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for corresponding error enum variant and handler
rg -A 2 "AliasOrPeerIdAlias|AliasOrPeerId.*Exists" --type rust
Length of output: 988
Script:
#!/bin/bash
# Check the error enum definition and its mapping
rg -A 5 "enum ApiErrorStatus" --type rust
# Check the actual error handling in the set_alias function
ast-grep --pattern 'async fn set_alias($$$) {
$$$
}'
Length of output: 575
Script:
#!/bin/bash
# Search for the set_alias function implementation with context
rg -A 20 "pub async fn set_alias" --type rust
Length of output: 47
Script:
#!/bin/bash
# Search for the handler function with more context and different pattern
rg -A 20 "fn set_alias.*->.*Response" --type rust
# Also check the API route definition
rg "route.*alias" --type rust
Length of output: 529
hoprd/db/api/src/aliases.rs (1)
95-109: Consider adding tests for edge cases.
While the current test coverage is comprehensive, consider adding tests for these scenarios:
- Very long aliases (to verify any length constraints)
- Special characters in aliases (to verify character set handling)
- Concurrent alias setting (to verify race condition handling)
Let's check if there are any length or character set constraints in the schema:
a62ea5e to
dfb3987
Compare
dfb3987 to
ffb42cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
hoprd/db/api/src/aliases.rs(3 hunks)hoprd/db/api/src/errors.rs(1 hunks)hoprd/rest-api/src/alias.rs(2 hunks)hoprd/rest-api/src/lib.rs(1 hunks)tests/test_integration.py(1 hunks)
🔇 Additional comments (5)
hoprd/db/api/src/errors.rs (2)
9-10: LGTM! The error variant aligns with PR objectives.
The new error variant correctly handles the case where an alias or peer ID already exists, supporting the PR's goal of preventing duplicate entries and ensuring unique mappings.
9-10: 🧹 Nitpick (assertive)
Consider enhancing error details for debugging.
The current implementation from the previous review comment is still valid and would improve debugging capabilities.
hoprd/rest-api/src/alias.rs (1)
87-87: LGTM: Documentation accurately reflects error conditions.
The updated error description properly covers both conflict scenarios: existing alias and already aliased peer_id. This aligns well with the PR objectives for preventing duplicate entries.
tests/test_integration.py (2)
100-103: LGTM! Well-structured test for automatic self-aliasing.
The test is concise, focused, and effectively verifies that each node automatically contains an alias "me" that maps to its own peer ID.
108-108: LGTM! Improved test name.
The renamed function better describes its purpose of testing alias removal functionality.
Reintroduce backwards compatible and POST compliant behavior on alias entry:
Notes
Refixes #6206