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

100-104: Consider removing #[repr(u8)] from SessionCapability enum

The #[repr(u8)] attribute on the SessionCapability enum may not be necessary since the enum variants are already unit-like and serialization is handled by Serde. Keeping #[repr(u8)] could lead to unintended behaviors during serialization or when adding new variants in the future. Removing it reduces potential issues and aligns with Rust's best practices for enums.

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

Line range hint 1-424: Consider adding response examples to the OpenAPI documentation.

While the schema definitions have been added, consider enhancing the documentation by adding example responses for the session-related endpoints. This would improve the swagger-ui experience.

Example enhancement for SessionClientResponse:

 #[derive(Debug, Clone, Serialize, utoipa::ToSchema)]
+#[schema(example = json!({
+    "id": "12345",
+    "status": "connected",
+    "target": {
+        "address": "127.0.0.1:8000",
+        "protocol": "tcp"
+    }
+}))]
 pub struct SessionClientResponse {
     // existing fields...
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Verify DELETE operation conformance in OpenAPI spec
# Expected: DELETE operations should not have requestBody

# Search for DELETE operation definitions in the OpenAPI spec
ast-grep --pattern 'delete($$$).route("/session/$$$", $$$)'

# Search for any DELETE operations that might have request bodies
rg -A 5 'delete\(' hoprd/rest-api/src/session.rs

#!/bin/bash
# Let's try a different approach to find DELETE operations in the codebase

# Search for DELETE operations in the REST API implementation
rg -i "delete.*session" hoprd/rest-api/src/lib.rs

# Look for route handlers in session.rs
rg "fn.*delete" hoprd/rest-api/src/session.rs

# Check the actual implementation of session-related operations
cat hoprd/rest-api/src/session.rs

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

314-333: Add documentation for the RoutingOptions enum.

While the implementation is correct, adding documentation would improve API clarity:

• Purpose of each variant
• When to use IntermediatePath vs Hops
• Impact of the explicit-path feature flag

 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, utoipa::ToSchema)]
+/// Options for configuring the routing behavior of a session.
 pub enum RoutingOptions {
     #[cfg(feature = "explicit-path")]
     #[schema(value_type = Vec<String>)]
+    /// Route through specific intermediate peers in the given order.
     IntermediatePath(#[serde_as(as = "Vec<DisplayFromStr>")] Vec<PeerId>),
+    /// Route through the specified number of random intermediate hops.
     Hops(usize),
 }

---

693-712: Add documentation for the IpProtocol enum.

The enum implementation is correct, but adding documentation would improve API clarity:

• Purpose of the enum
• When to use each protocol variant
• Impact on session behavior

 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, strum::Display, strum::EnumString, utoipa::ToSchema)]
 #[strum(serialize_all = "lowercase", ascii_case_insensitive)]
 #[serde(rename_all = "lowercase")]
+/// Supported IP protocols for session communication.
 pub enum IpProtocol {
     #[allow(clippy::upper_case_acronyms)]
+    /// Transmission Control Protocol - reliable, ordered delivery
     TCP,
     #[allow(clippy::upper_case_acronyms)]
+    /// User Datagram Protocol - unreliable, unordered delivery
     UDP,
 }

---

917-917: Enhance test coverage for routing options.

The tests use Default::default() for IntermediatePath, which might not adequately test all routing scenarios. Consider adding test cases for:

• Different path lengths
• Various hop counts
• Invalid path configurations

Also applies to: 939-939, 982-982

Configuration used: .coderabbit.yaml
Review profile: CHILL

Makefile (1)

105-105: LGTM! Improved process termination signal.

The change from default kill to kill -s INT is a better practice as it sends an interrupt signal (SIGINT) instead of termination signal (SIGTERM), allowing the process to handle the shutdown more gracefully. This is particularly appropriate for server processes like anvil.

tests/utils.py (1)

55-55: Improve code readability by breaking long lines

The change to use get() with a default value is a good defensive programming practice. However, the lines exceed the maximum length of 120 characters.

Consider breaking the lines for better readability:

-        oc for oc in all_channels.all if oc.get('source_address', None) == src.address and oc.get('destination_address', None) == dest.address
+        oc for oc in all_channels.all
+        if (oc.get('source_address', None) == src.address and
+            oc.get('destination_address', None) == dest.address)

Also applies to: 64-64

🧰 Tools

🪛 Ruff (0.8.2)

55-55: Line too long (142 > 120)

(E501)

tests/hopr.py (1)

347-348: Improve code style in membership test

The membership test can be written more idiomatically.

-                if not param in getattr(response, status)[0]:
+                if param not in getattr(response, status)[0]:

🧰 Tools

🪛 Ruff (0.8.2)

347-347: Test for membership should be not in

Convert to not in

(E713)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

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

119-143: Document TCP-specific assumptions in capabilities.

Based on past learnings, the current implementation assumes TCP-like behavior for sessions. Consider adding documentation to clarify which capabilities are applicable to different session types (TCP vs UDP).

Add documentation like this:

 #[repr(u8)]
 #[derive(
     Debug, Clone, strum::EnumIter, strum::Display, strum::EnumString, Serialize, Deserialize, utoipa::ToSchema,
 )]
+/// Session capabilities that can be enabled for a connection.
+/// Note: Some capabilities are primarily designed for TCP-like sessions
+/// and may not be applicable to all session types.
 pub enum SessionCapability {

---

402-420: Optimize capabilities conversion and defaults.

The current implementation could be more efficient by avoiding the intermediate Vec allocation.

Consider this optimization:

-            capabilities: self
-                .capabilities
-                .map(|vs| {
-                    vs.into_iter()
-                        .map(|v| {
-                            let cap: hopr_lib::SessionCapability = v.into();
-                            cap
-                        })
-                        .collect::<Vec<_>>()
-                })
-                .unwrap_or_else(|| match target_protocol {
-                    IpProtocol::TCP => {
-                        vec![
-                            hopr_lib::SessionCapability::Retransmission,
-                            hopr_lib::SessionCapability::Segmentation,
-                        ]
-                    }
-                    _ => vec![], // no default capabilities for UDP, etc.
-                }),
+            capabilities: self.capabilities
+                .map(|vs| vs.into_iter().map(Into::into).collect())
+                .unwrap_or_else(|| match target_protocol {
+                    IpProtocol::TCP => vec![
+                        hopr_lib::SessionCapability::Retransmission,
+                        hopr_lib::SessionCapability::Segmentation,
+                    ],
+                    _ => Vec::new(),
+                }),

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6393
File: hoprd/rest-api/src/session.rs:91-91
Timestamp: 2024-11-12T10:56:01.095Z
Learning: The current implementation in `hoprd/rest-api/src/session.rs` assumes all sessions are TCP-like due to the missing implementation of UDP behavior. The commented-out code suggests an intention to support different session types based on `capabilities`.

tests/conftest.py (1)

427-428: LGTM! Good addition of timeout protection.

Adding timeout protection to the node connectivity check is a good improvement that prevents tests from hanging indefinitely if nodes fail to connect.

Consider these improvements:

1. Extract the timeout value (60) to a constant at the top of the file for better maintainability
2. Add more detailed error logging when timeout occurs, for example:

-    nodes_connectivity = await asyncio.wait_for(asyncio.gather(*tasks), timeout)
+    try:
+        nodes_connectivity = await asyncio.wait_for(asyncio.gather(*tasks), timeout)
+    except asyncio.TimeoutError:
+        logging.error(f"Node connectivity check timed out after {timeout} seconds")
+        raise

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro