Output routing options in the Session listing #6745
Conversation
When Session gets created and listed (via create/list REST API) endpoints, the `path` now gets returned as well reflecting on the routing options for that Session. Also, new return code has been added when the listening host for a new session is already occupied.
📝 WalkthroughWalkthroughThe pull request introduces changes to session management and error handling across multiple files in the Hoprd project. The modifications primarily focus on improving the structure of session-related data by introducing a new Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)hoprd/rest-api/src/session.rs (5)
The new
The addition of the Also applies to: 452-452
The error handling has been enhanced to consistently return Also applies to: 557-557, 622-622
The implementation consistently uses the new Also applies to: 648-663, 713-718
Let's ensure the new structure is used consistently throughout the codebase. ✅ Verification successfulBased on the search results, I can now generate the final response: StoredSessionEntry is consistently used for session storage across the codebase The verification shows that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining tuple-based session storage and verify StoredSessionEntry usage
# Check for any remaining tuple storage patterns
rg -A 2 "HashMap<ListenerId,\s*\("
# Check for consistent usage of StoredSessionEntry
rg "StoredSessionEntry"
# Check for any missed path fields in session responses
ast-grep --pattern 'struct $name {
$$$
path: $type,
$$$
}'
Length of output: 639 Script: #!/bin/bash
# Let's verify more thoroughly with different approaches
# Check for any HashMap usage with session-related types
rg -A 3 "HashMap.*Session"
# Look for all struct definitions with 'Session' in their name
ast-grep --pattern 'struct $name {
$$$
}'
# Search for any session-related response types
rg -A 5 "struct.*Response"
# Check for any session creation or storage patterns
rg -A 3 "new_session|create_session|store_session"
Length of output: 66072 Script: #!/bin/bash
# Let's focus on session-related code in the hoprd codebase
# Search for session-related struct definitions and implementations
rg -A 5 "struct.*Session|impl.*Session"
# Look for session creation and storage patterns
rg -A 5 "new_session|create_session|add_session|register_session"
# Check for session registry/storage usage
rg -A 5 "SessionRegistry|session_registry|sessions"
# Look for any session target or path usage
rg -A 5 "SessionTargetSpec|RoutingOptions"
Length of output: 83264 Script: #!/bin/bash
# Let's focus specifically on the session storage and routing options
# Check for StoredSessionEntry usage and implementation
rg -A 5 "StoredSessionEntry"
# Look for session target and routing options usage
rg -A 5 "SessionTargetSpec|RoutingOptions" hoprd/
# Check for session storage/registry patterns in hoprd
rg -A 5 "HashMap.*Session|session_registry|sessions" hoprd/
# Look for any modifications to session storage
rg -A 5 "insert|remove|add.*session" hoprd/
Length of output: 30592 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: 0
🧹 Outside diff range and nitpick comments (1)
hoprd/rest-api/src/session.rs (1)
552-559: Consider reducing code duplication in session storage.The session storage logic is duplicated between TCP and UDP handlers. Consider extracting the common storage logic into a helper function.
+ fn store_session( + open_listeners: &mut tokio::sync::RwLockWriteGuard<'_, HashMap<ListenerId, StoredSessionEntry>>, + listener_id: ListenerId, + target: SessionTargetSpec, + path: RoutingOptions, + jh: hopr_async_runtime::prelude::JoinHandle<()>, + ) { + open_listeners.insert( + listener_id, + StoredSessionEntry { + target, + path, + jh, + }, + ); + }Also applies to: 592-607
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
hoprd/rest-api/Cargo.toml(1 hunks)hoprd/rest-api/src/session.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hoprd/rest-api/Cargo.toml
🔇 Additional comments (4)
hoprd/rest-api/src/session.rs (4)
120-129: LGTM! Well-structured session storage.
The new StoredSessionEntry struct provides a clear and well-documented structure for session data, improving code readability and maintainability over the previous tuple-based storage.
481-481: LGTM! Improved error handling for address conflicts.
The consistent use of ListenHostAlreadyUsed error status across TCP and UDP bindings provides clearer feedback when attempting to bind to an already occupied address.
Also applies to: 501-501, 566-566
387-387: LGTM! Response structure properly includes path information.
The SessionClientResponse has been correctly updated to include path information, and this is consistently implemented in both creation and listing endpoints.
Also applies to: 396-396, 618-618, 657-662
735-739: LGTM! Proper async resource cleanup.
The join handle is correctly cancelled during session cleanup, ensuring proper resource management and preventing potential leaks.
|
Needs #6737 to be merged first |
# Conflicts: # hoprd/rest-api/src/session.rs
When Session gets created and listed (via create/list REST API) endpoints, the
pathnow gets returned as well reflecting on the routing options for that Session.Also, new return code has been added when the listening host for a new session is already occupied.
Closes #6693
Closes #6694