hoprd/rest-api/src/messages.rs (1)

Line range hint 174-211: Consider enhancing error handling for path resolution.

While the overall validation is good, the error handling for path resolution could be more specific. When path resolution fails, the error message "Failed to fulfill path" doesn't provide enough context about which peer in the path failed to resolve.

Consider modifying the error handling to include the problematic peer ID:

- .map_err(|e: ApiErrorStatus| {
-     (
-         StatusCode::UNPROCESSABLE_ENTITY,
-         ApiErrorStatus::UnknownFailure(format!("Failed to fulfill path: {e}")),
-     )
-         .into_response()
- })?
+ .map_err(|e: ApiErrorStatus| {
+     (
+         StatusCode::UNPROCESSABLE_ENTITY,
+         ApiErrorStatus::UnknownFailure(format!(
+             "Failed to resolve peer in path: {}",
+             e
+         )),
+     )
+         .into_response()
+ })?

hoprd/rest-api/src/session.rs (3)

114-116: Add documentation for the path field

Consider adding documentation comments (///) to the path field in SessionWebsocketClientQueryRequest to explain its purpose and how it overrides the hops parameter when the explicit-path feature is enabled.

---

135-150: Avoid variable shadowing of path_options

The variable path_options is initially assigned at line 135 and then potentially shadowed at line 138 within the #[cfg(feature = "explicit-path")] block. To improve code clarity and prevent confusion, consider avoiding variable shadowing by modifying the code.

Apply this diff to avoid variable shadowing:

 let mut path_options = RoutingOptions::Hops((self.hops as u32).try_into()?);

 #[cfg(feature = "explicit-path")]
 if let Some(path) = self.path {
     // Explicit `path` will override `hops`
-    let path_options = RoutingOptions::IntermediatePath(
+    path_options = RoutingOptions::IntermediatePath(
         path.split(',')
             .map(PeerId::from_str)
             .collect::<Result<Vec<PeerId>, _>>()
             .map_err(|e| HoprLibError::GeneralError(format!("invalid peer id on path: {e}")))?
             .try_into()?,
     );
 }

---

314-318: Review the necessity of runtime checks for explicit-path feature

The runtime check for RoutingOptions::IntermediatePath within the #[cfg(not(feature = "explicit-path"))] block may be redundant because IntermediatePath should not be constructed when the explicit-path feature is disabled. Consider verifying whether this check is necessary.

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Search for cfg attributes related to explicit-path feature
rg '#\[cfg.*explicit-path' 

# Search for feature-gated code blocks
rg 'cfg_if.*explicit-path'

#!/bin/bash
# Let's examine the actual code changes in the files where explicit-path is used
rg -B 2 -A 2 '#\[cfg.*explicit-path' hoprd/rest-api/src/messages.rs hoprd/rest-api/src/session.rs

# Check if there are any tests or examples using this feature
fd -e rs | xargs rg 'explicit-path'

# Look for any documentation or comments about this feature
rg -i 'explicit.?path' -g '*.{md,rs,toml}'

#!/bin/bash
# Check for other endpoints that might need the explicit-path feature flag
echo "Checking for other routing-related endpoints..."
rg -l "RoutingOptions::" --type rust

# Check for potential API clients
echo "Checking for API clients..."
fd -e ts -e js -e py -e rs "api" -x rg -l "websocket|messages"

#!/bin/bash
# Let's check the usage of explicit path routing feature flag across the codebase
echo "Checking feature flag usage..."
rg -A 5 "explicit-path" --type rust

# Let's examine the routing-related code in the identified files
echo "Checking routing implementations..."
for file in "transport/session/src/initiation.rs" "transport/session/src/manager.rs" "transport/api/src/lib.rs" "hoprd/rest-api/src/messages.rs" "hoprd/rest-api/src/session.rs"; do
    echo "=== $file ==="
    rg -A 10 "Route|route" "$file"
done

# Let's check the websocket test implementation
echo "=== Checking websocket test implementation ==="
cat "tests/test_websocket_api.py"

hoprd/rest-api/src/session.rs (2)

135-150: Consider adding input validation for path

While the implementation is solid, consider adding validation for:

1. Empty path string
2. Maximum number of allowed hops to prevent excessive routing chains

 #[cfg(feature = "explicit-path")]
 let path_options = if let Some(path) = self.path {
+    // Validate path is not empty
+    if path.trim().is_empty() {
+        return Err(HoprLibError::GeneralError("path cannot be empty".into()));
+    }
+    
+    let peers: Vec<PeerId> = path.split(',')
+        .map(PeerId::from_str)
+        .collect::<Result<Vec<PeerId>, _>>()
+        .map_err(|e| HoprLibError::GeneralError(format!("invalid peer id on path: {e}")))?;
+    
+    // Validate maximum hops
+    const MAX_HOPS: usize = 10; // Adjust this value based on your requirements
+    if peers.len() > MAX_HOPS {
+        return Err(HoprLibError::GeneralError(format!("path exceeds maximum allowed hops: {}", MAX_HOPS)));
+    }
+    
     RoutingOptions::IntermediatePath(
-        path.split(',')
-            .map(PeerId::from_str)
-            .collect::<Result<Vec<PeerId>, _>>()
-            .map_err(|e| HoprLibError::GeneralError(format!("invalid peer id on path: {e}")))?
+        peers
             .try_into()?,
     )

---

Feature flag implementation needs test coverage

The explicit-path feature flag is properly defined in Cargo.toml and consistently used across the codebase with proper cfg attributes. However, there are no specific test cases covering the explicit path routing functionality. Current tests in session.rs and messages.rs only cover basic functionality.

Required actions:

• Add test cases in hoprd/rest-api/src/session.rs to verify the behavior when explicit-path feature is enabled
• Add test cases in hoprd/rest-api/src/messages.rs to verify message routing with explicit paths
• Consider adding integration tests to verify the end-to-end routing behavior with explicit paths

🔗 Analysis chain

Line range hint 1-1: Verify feature flag consistency and test coverage

Let's verify the consistent usage of the explicit-path feature flag and ensure proper test coverage.

---

Let me gather more information about the test coverage specifically for our codebase, excluding vendor files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for consistent feature flag usage
echo "Checking feature flag usage..."
rg --type rust "explicit-path|explicit_path" -C 2

# Check for related test coverage
echo "Checking test coverage..."
rg --type rust "test.*explicit.*path|test.*path.*routing" -C 2

Length of output: 5384

---

Script:

#!/bin/bash
# Check for test modules and test cases in our codebase
echo "Checking test modules in hoprd..."
fd -e rs test hoprd/

echo -e "\nChecking test cases for routing and paths..."
rg --type rust "mod\s+tests|#\[test\]" -C 2 hoprd/rest-api/src/session.rs hoprd/rest-api/src/messages.rs

echo -e "\nChecking feature flag definitions..."
fd Cargo.toml -x cat {} | grep -A 5 -B 5 "explicit-path"

Length of output: 2320

Configuration used: .coderabbit.yaml
Review profile: CHILL

hoprd/rest-api/src/session.rs (1)

138-150: Consider using multiple path parameters instead of comma-separated values

While the implementation works, using comma-separated values for peer IDs in URLs might cause issues with some clients. Based on previous discussions, consider using multiple path parameters (e.g., path=peer1&path=peer2) which is more URL-compliant.

-            path.split(',')
+            path.split('&')
+                .filter_map(|param| param.strip_prefix("path="))

Configuration used: .coderabbit.yaml
Review profile: CHILL