Update Axum version in proxy #817
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
paulgb
changed the base branch from
main
to
paul/dis-2702-plane-proxy-refactor-to-latest-hyper
paulgb
force-pushed
the
paul/dis-2737-update-axum-version-in-proxy
branch
from
2db6f7f to
4829b68
Compare
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files in the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used📓 Learnings (1)
🔇 Additional comments (5)
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (11)
dynamic-proxy/tests/common/websocket_echo_server.rs (1)
51-58: Improved error handling, but consider adding loggingThe changes improve the robustness of the
handle_socketfunction by handling errors more gracefully and being more specific about the message types it processes. Breaking the loop instead of panicking is a good approach for a long-running WebSocket connection.However, consider the following suggestions to further improve the implementation:
Add logging for error cases to aid in debugging. For example:
if let Ok(msg) = msg { if let Message::Text(text) = msg { if socket.send(Message::Text(text)).await.is_err() { eprintln!("Failed to send message, closing connection"); break; } } else { eprintln!("Received non-text message, ignoring"); } } else { eprintln!("Error receiving message, closing connection"); break; }Consider handling non-text messages explicitly, either by logging them or by sending an appropriate response to the client.
These additions would improve the observability of the WebSocket server behavior without compromising the robustness gained by the current changes.
plane/plane-tests/tests/common/websocket_echo_server.rs (1)
51-58: Improved error handling, but consider adding debug logging.The changes to the
handle_socketfunction significantly improve error handling by replacing panics with graceful loop breaks. This enhancement aligns well with best practices for robust WebSocket servers.Consider adding debug logging for errors to aid in troubleshooting. Here's a suggested improvement:
async fn handle_socket(mut socket: WebSocket) { while let Some(msg) = socket.recv().await { if let Ok(msg) = msg { if let Message::Text(text) = msg { if socket.send(Message::Text(text)).await.is_err() { + tracing::debug!("Failed to send message, closing connection"); break; } } } else { + tracing::debug!("Failed to receive message, closing connection"); break; } } }This addition will provide valuable context during debugging without affecting the production behavior.
dynamic-proxy/tests/graceful.rs (1)
39-41: LGTM with a minor suggestion.The changes improve the code by ensuring proper ownership of the
clientandurlvariables within the closure. This prevents potential lifetime issues and enhances code clarity.However, I suggest removing the underscore prefixes from
_clientand_urlas these variables are actually used within the closure. The underscore prefix typically indicates intentionally unused variables, which is not the case here.Consider applying this minor change:
- let _client = client.clone(); - let _url = url.clone(); - let response_handle = tokio::spawn(async move { _client.get(&_url).send().await.unwrap() }); + let client_clone = client.clone(); + let url_clone = url.clone(); + let response_handle = tokio::spawn(async move { client_clone.get(&url_clone).send().await.unwrap() });dynamic-proxy/tests/graceful_https.rs (1)
58-58: Add comment explaining the purpose of sleepThe reduction in sleep duration from 200ms to 100ms is a good optimization. However, it would be helpful to add a comment explaining why this sleep is necessary. It appears to be giving time for the shutdown process to start before checking the response.
Consider adding a comment like this:
+ // Give time for the shutdown process to start before checking the response tokio::time::sleep(Duration::from_millis(100)).await;dynamic-proxy/src/graceful_shutdown.rs (1)
Line range hint
36-38: LGTM: New subscribe method enhances shutdown notification capabilitiesThe addition of the
subscribemethod to theGracefulShutdownstruct is a valuable enhancement. It allows external code to receive shutdown notifications, enabling multiple parts of the application to independently listen for shutdown signals. The implementation is correct and straightforward.Consider adding a brief doc comment to explain the purpose and usage of this method. For example:
/// Returns a receiver that can be used to listen for shutdown notifications. /// The receiver will be notified when the `shutdown` method is called. pub fn subscribe(&self) -> watch::Receiver<()> { self.tx.subscribe() }plane/plane-tests/tests/proxy_connection_monitor.rs (1)
63-63: Consider usingassert!for boolean checksThe change from
assert!toassert_eq!doesn't alter the test's behavior, but usingassert!for boolean checks is more idiomatic in Rust. It's also more concise and directly expresses the intent.Consider reverting this change:
- assert_eq!(backend_entry.had_recent_connection, true); + assert!(backend_entry.had_recent_connection);This aligns with Clippy's recommendation and Rust's best practices for boolean assertions.
🧰 Tools
🪛 GitHub Check: clippy
[warning] 63-63: used
assert_eq!with a literal bool
warning: usedassert_eq!with a literal bool
--> plane/plane-tests/tests/proxy_connection_monitor.rs:63:5
|
63 | assert_eq!(backend_entry.had_recent_connection, true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
= note:#[warn(clippy::bool_assert_comparison)]on by default
help: replace it withassert!(..)
|
63 - assert_eq!(backend_entry.had_recent_connection, true);
63 + assert!(backend_entry.had_recent_connection);
|dynamic-proxy/src/proxy.rs (3)
22-29: LGTM! Consider adding a comment explaining the custom Clone implementation.The custom
Cloneimplementation forProxyClientlooks correct. It properly clones theclientfield and copies thetimeoutfield.To improve code clarity, consider adding a brief comment explaining why a custom
Cloneimplementation is necessary. For example:// Custom Clone implementation to ensure proper cloning of the client field impl Clone for ProxyClient { // ... (rest of the implementation) }
61-61: LGTM! Consider adding more context to the error log.The change from
warn!toerror!appropriately reflects the severity of an upstream request failure.For consistency with the error handling below and to provide more context, consider modifying the log message as follows:
tracing::error!(url, "Upstream request timed out");This change would make it clear that the failure is due to a timeout and maintain consistency with the error handling pattern used elsewhere in the method.
119-120: LGTM! Consider unifying log levels for consistency.The changes in the
upstream_requestmethod improve error handling and logging:
- The introduction of the
urlvariable enhances code readability.- The new warning log for request timeouts provides valuable context.
- Including the
urlin the error log aids in debugging.For consistency with the changes made earlier in the file, consider changing the log level for the timeout case from
warn!toerror!:tracing::error!(url, "Upstream request timed out.");This would maintain a consistent approach to logging severe issues throughout the file.
Also applies to: 124-124, 128-128
plane/src/proxy/proxy_server.rs (1)
147-147: Simplified error logging format.The error logging has been simplified, which improves readability. However, consider the trade-offs with structured logging, which can be beneficial for log parsing and filtering in production environments.
If structured logging is important for your logging infrastructure, consider using a format like:
tracing::error!(error = %e, "Error proxying request");This maintains readability while preserving structured logging benefits.
plane/src/controller/forward_auth.rs (1)
71-74: Simplify status code setting inresponse_helperThe
Response::builder().status()method acceptsStatusCodedirectly. You can simplify the code by passingstatuswithout converting it tou16.Apply this diff to simplify the code:
Response::builder() - .status(status.as_u16()) + .status(status) .body(body) .expect("Response is always valid.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
- dynamic-proxy/src/graceful_shutdown.rs (1 hunks)
- dynamic-proxy/src/proxy.rs (4 hunks)
- dynamic-proxy/tests/common/simple_upgrade_service.rs (1 hunks)
- dynamic-proxy/tests/common/websocket_echo_server.rs (1 hunks)
- dynamic-proxy/tests/graceful.rs (1 hunks)
- dynamic-proxy/tests/graceful_https.rs (1 hunks)
- dynamic-proxy/tests/test_http_redirect.rs (1 hunks)
- dynamic-proxy/tests/test_upgrade.rs (1 hunks)
- plane/Cargo.toml (3 hunks)
- plane/plane-tests/Cargo.toml (1 hunks)
- plane/plane-tests/tests/backend_actions.rs (1 hunks)
- plane/plane-tests/tests/common/localhost_resolver.rs (1 hunks)
- plane/plane-tests/tests/common/mod.rs (1 hunks)
- plane/plane-tests/tests/common/test_env.rs (2 hunks)
- plane/plane-tests/tests/common/websocket_echo_server.rs (1 hunks)
- plane/plane-tests/tests/forward_auth.rs (1 hunks)
- plane/plane-tests/tests/proxy_connection_monitor.rs (1 hunks)
- plane/src/client/sse.rs (5 hunks)
- plane/src/controller/connect.rs (1 hunks)
- plane/src/controller/error.rs (1 hunks)
- plane/src/controller/forward_auth.rs (4 hunks)
- plane/src/controller/mod.rs (4 hunks)
- plane/src/proxy/mod.rs (1 hunks)
- plane/src/proxy/proxy_server.rs (2 hunks)
- plane/src/proxy/request.rs (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- plane/plane-tests/tests/common/localhost_resolver.rs
- plane/plane-tests/tests/common/mod.rs
- plane/src/controller/connect.rs
🧰 Additional context used
📓 Learnings (2)
dynamic-proxy/tests/graceful.rs (1)
Learnt from: paulgb PR: jamsocket/plane#814 File: dynamic-proxy/tests/graceful.rs:46-60 Timestamp: 2024-09-24T19:32:37.996Z Learning: In the `test_graceful_shutdown()` test in `dynamic-proxy/tests/graceful.rs`, if the server has not shut down, the response will be returned immediately, failing the test, so adding a timeout is unnecessary.dynamic-proxy/tests/graceful_https.rs (1)
Learnt from: paulgb PR: jamsocket/plane#814 File: dynamic-proxy/tests/graceful.rs:46-60 Timestamp: 2024-09-24T19:32:37.996Z Learning: In the `test_graceful_shutdown()` test in `dynamic-proxy/tests/graceful.rs`, if the server has not shut down, the response will be returned immediately, failing the test, so adding a timeout is unnecessary.
🪛 GitHub Check: clippy
dynamic-proxy/tests/test_http_redirect.rs
[warning] 33-33: returning the result of a
letbinding from a block
warning: returning the result of aletbinding from a block
--> dynamic-proxy/tests/test_http_redirect.rs:33:5
|
31 | let response = get_client().get(url).send().await.unwrap();
| ----------------------------------------------------------- unnecessaryletbinding
32 |
33 | response
| ^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
= note:#[warn(clippy::let_and_return)]on by default
help: return the expression directly
|
31 ~
32 |
33 ~ get_client().get(url).send().await.unwrap()
|plane/plane-tests/tests/proxy_connection_monitor.rs
[warning] 63-63: used
assert_eq!with a literal bool
warning: usedassert_eq!with a literal bool
--> plane/plane-tests/tests/proxy_connection_monitor.rs:63:5
|
63 | assert_eq!(backend_entry.had_recent_connection, true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
= note:#[warn(clippy::bool_assert_comparison)]on by default
help: replace it withassert!(..)
|
63 - assert_eq!(backend_entry.had_recent_connection, true);
63 + assert!(backend_entry.had_recent_connection);
|
🔇 Additional comments (45)
plane/plane-tests/Cargo.toml (4)
23-23: New feature "net" added totokiodependency - verify usage and necessityThe
tokiodependency now includes the "net" feature in addition to the existing features. This suggests that the project now requires Tokio's networking capabilities.Please ensure that:
- The "net" feature is actually needed and used in the project.
- This addition is related to the updates in
hyperandreqwest, which might require this feature for async networking.You can use the following script to find all usages of Tokio's networking features in the project:
#!/bin/bash # Find all Rust files using Tokio's networking features rg --type rust 'tokio::(net|io)::' -A 5Review the output to confirm the necessity of the "net" feature and identify any areas where it's being used.
19-19: Minor version update forreqwest- review changelog for improvementsThe
reqwestdependency has been updated from version 0.11.22 to 0.12.8. This minor version update likely includes new features, performance improvements, or bug fixes.Please:
- Review the
reqwestchangelog for any relevant improvements or changes that could benefit the project.- Ensure that the project's HTTP client usage with
reqweststill functions as expected with this new version.You can use the following script to find all usages of
reqwestin the project:#!/bin/bash # Find all Rust files using reqwest rg --type rust 'use reqwest::|use reqwest\s' -A 5Review the output to identify any potential areas where new features could be leveraged or where testing might be necessary.
16-23: Summary of dependency updates - consider overall project impactThe changes in this file represent a significant update to key dependencies:
hyperhas undergone a major version update (0.14 to 1.4).reqwesthas been updated to a new minor version (0.11 to 0.12).tokiohas added the "net" feature.Given these changes, it's crucial to:
- Review the entire project for any breaking changes due to the
hyperupdate.- Test thoroughly to ensure all networking functionality works as expected with the updated dependencies.
- Consider any performance improvements or new features that could be leveraged from these updates.
To get an overview of the project's dependency usage, you can run:
#!/bin/bash # List all direct dependencies used in Rust files rg --type rust '^\s*use\s+(\w+)' -o -r '$1' | sort | uniqThis will help identify which parts of the project might be affected by these dependency updates.
16-16: Major version update forhyper- verify compatibility and update usageThe
hyperdependency has been updated from version 0.14.27 to 1.4.1. This is a significant update that crosses the 1.0 boundary, which often indicates breaking changes in the API.Please ensure that:
- All usage of the
hypercrate in the project has been updated to be compatible with the new version.- The project's HTTP server implementation using Hyper has been thoroughly tested with this new version.
You can use the following script to find all usages of
hyperin the project:Review the output to identify any potential incompatibilities or required updates.
plane/Cargo.toml (8)
52-52: Verify compatibility with tungstenite 0.24.0The
tungstenitedependency has been updated from version 0.20.1 to 0.24.0. This significant version jump may introduce breaking changes that could affect the codebase. Note that this update aligns with the tokio-tungstenite update observed earlier.Please ensure that:
- All breaking changes have been addressed in the codebase.
- New features or improvements in tungstenite 0.24.0 are leveraged where applicable.
Run the following script to check for potential affected areas:
#!/bin/bash # Description: Search for tungstenite-related code that might be affected by the update # Test: Search for tungstenite imports and usages rg --type rust -i 'use tungstenite::|tungstenite::' # Test: Search for common tungstenite types or methods rg --type rust -i 'WebSocket|Message|Error'Review the tungstenite changelog for detailed information on changes between versions 0.20.1 and 0.24.0.
47-47: Verify compatibility with tower 0.5.1The
towerdependency has been updated from version 0.4.13 to 0.5.1. While this is a minor version update, it may still introduce some changes that could affect the codebase.Please ensure that:
- Any breaking changes or deprecations have been addressed in the codebase.
- New features or improvements in tower 0.5.1 are leveraged where applicable.
Run the following script to check for potential affected areas:
#!/bin/bash # Description: Search for tower-related code that might be affected by the update # Test: Search for tower imports and usages rg --type rust -i 'use tower::|tower::' # Test: Search for common tower types or traits rg --type rust -i 'Service|Layer|ServiceBuilder'Review the tower changelog for detailed information on changes between versions 0.4.13 and 0.5.1.
29-29: Verify the necessity and usage of hyper-utilA new dependency
hyper-utilversion 0.1.9 has been added with features "client", "client-legacy", "http1", and "http2". This addition is likely related to the major version update of hyper.Please ensure that:
- The
hyper-utildependency is necessary and being used effectively in the codebase.- The selected features are all required for the project.
Run the following script to check for the usage of hyper-util:
If the search doesn't yield any results, consider removing this dependency or documenting its intended future use.
46-46: Verify compatibility with tokio-tungstenite 0.24.0The
tokio-tungstenitedependency has been updated from version 0.20.1 to 0.24.0. This significant version jump may introduce breaking changes that could affect the codebase.Please ensure that:
- All breaking changes have been addressed in the codebase.
- New features or improvements in tokio-tungstenite 0.24.0 are leveraged where applicable.
Run the following script to check for potential affected areas:
Review the tokio-tungstenite changelog for detailed information on changes between versions 0.20.1 and 0.24.0.
48-48: Verify compatibility with tower-http 0.6.1The
tower-httpdependency has been updated from version 0.4.4 to 0.6.1. While this is a minor version update, it spans two minor versions and may introduce changes that could affect the codebase.Please ensure that:
- Any breaking changes or deprecations have been addressed in the codebase.
- New features or improvements in tower-http 0.6.1 are leveraged where applicable.
- The "trace" and "cors" features are still correctly implemented.
Run the following script to check for potential affected areas:
Review the tower-http changelog for detailed information on changes between versions 0.4.4 and 0.6.1.
28-28: Verify compatibility with hyper 1.4.1The
hyperdependency has been updated from version 0.14.27 to 1.4.1, which is a major version change. This update is likely to introduce significant breaking changes that will affect the codebase.Please ensure that:
- All breaking changes have been addressed in the codebase.
- New features or improvements in hyper 1.4.1 are leveraged where applicable.
Run the following script to check for potential breaking changes:
Review the hyper changelog for detailed information on changes between versions 0.14.27 and 1.4.1.
17-17: Verify compatibility with axum 0.7.7The
axumdependency has been updated from version 0.6.20 to 0.7.7, which is a major version change. This update may introduce breaking changes that could affect the codebase.Please ensure that:
- All breaking changes have been addressed in the codebase.
- New features or improvements in axum 0.7.7 are leveraged where applicable.
Run the following script to check for potential breaking changes:
Review the axum changelog for detailed information on changes between versions 0.6.20 and 0.7.7.
✅ Verification successful
Attempting to re-verify axum update with corrected scripts
Please run the following scripts to accurately search for
axum-related code that might be affected by the update:Please provide the output of these scripts to continue the verification process.
axum 0.7.7 Update Verified
The
axumdependency has been updated to version0.7.7. Based on the current codebase analysis, no incompatible usages or breaking changes have been identified that would affect the functionality of your project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for axum-related code that might be affected by the update # Test: Search for axum imports and usages rg --type rust -i 'use axum::|axum::' # Test: Search for Router usage (common breaking change in axum updates) rg --type rust -i 'Router::new()' # Test: Search for specific axum types that often change rg --type rust -i 'Json|Extension|TypedHeader'Length of output: 110543
Script:
#!/bin/bash # Description: Corrected script to search for axum-related code that might be affected by the update # Search for axum imports and usages rg --type rust -i 'use axum::' # Search for Router usage (common breaking change in axum updates) rg --type rust -i 'Router::new()' # Search for specific axum types that often change rg --type rust -i 'Json|Extension|TypedHeader'Length of output: 16376
34-34: Verify compatibility with reqwest 0.12.8The
reqwestdependency has been updated from version 0.11.22 to 0.12.8. While this is a minor version update, it may still introduce some changes that could affect the codebase.Please ensure that:
- Any breaking changes or deprecations have been addressed in the codebase.
- New features or improvements in reqwest 0.12.8 are leveraged where applicable.
Run the following script to check for potential affected areas:
Review the reqwest changelog for detailed information on changes between versions 0.11.22 and 0.12.8.
✅ Verification successful
reqwest 0.12.8 Update Verified
The
reqwestdependency has been successfully updated to version0.12.8. Based on the current codebase searches:
- Imports and Usages: Multiple instances of
reqwestimports and usages were identified, includingClient::new(),Client::builder(), and various HTTP methods.- Functionality Check: Common
reqwestmethods such asget_async()andpost_async()are in use and appear compatible with the updated version.- No Deprecated Features: No deprecated or removed features related to
reqwestwere detected in the current codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for reqwest-related code that might be affected by the update # Test: Search for reqwest imports and usages rg --type rust -i 'use reqwest::|reqwest::' # Test: Search for common reqwest types or methods that might have changed rg --type rust -i 'Client::new()|Response::json|RequestBuilder'Length of output: 110543
Script:
#!/bin/bash # Description: Search for reqwest-related code that might be affected by the update # Test: Search for reqwest imports rg --type rust -i '^use reqwest::' # Test: Search for reqwest usages outside of imports rg --type rust -i '\breqwest::\b' # Test: Search for common reqwest types or methods that might have changed rg --type rust -i 'Client::new\(\)|Response::json\(|RequestBuilder'Length of output: 5507
Script:
#!/bin/bash # Description: Comprehensive search for reqwest-related code that might be affected by the update # Test: Search for reqwest imports echo "Searching for reqwest imports..." rg --type rust '^use reqwest::' # Test: Search for reqwest usages outside of imports echo "Searching for reqwest usages outside of imports..." rg --type rust '\breqwest::\b' # Test: Search for common reqwest types or methods that might have changed echo "Searching for common reqwest types or methods that might have changed..." rg --type rust 'Client::new\(\)|Response::json\(|RequestBuilder|get_async\(|post_async\('Length of output: 5864
dynamic-proxy/tests/graceful.rs (1)
39-41: Verify test functionality after changes.While the changes appear to maintain the core logic of the test, it's important to ensure that the test still functions as expected. The retrieved learning suggests that no timeout is necessary in this test, which aligns with the current implementation.
To verify the test functionality, please run the following command:
This command will run the specific test and display any output, allowing you to verify that the test still passes and behaves as expected after the changes.
dynamic-proxy/tests/test_upgrade.rs (3)
Line range hint
1-75: Summary: Test updated to remove CONNECTION header checks while maintaining core functionality.The changes in this file are minimal and focused on removing the
CONNECTIONheader import and related assertions. The core functionality of the upgrade test remains intact, still verifying the status code, UPGRADE header, and communication over the upgraded connection.These changes appear intentional and may be aligned with updates in the underlying libraries or implementation. No new issues or concerns were introduced by these modifications.
To ensure completeness:
- Confirm that the removal of
CONNECTIONheader checks aligns with any changes in theSimpleUpgradeServiceor related components.- Verify that this change is consistent across all relevant tests and implementations in the codebase.
Line range hint
18-75: LGTM: Test still covers core upgrade functionality.The removal of the
CONNECTIONheader assertion doesn't significantly impact the test's effectiveness. The test still verifies:
- The correct status code (SWITCHING_PROTOCOLS)
- The presence and value of the UPGRADE header
- The actual communication over the upgraded connection
However, it's worth confirming if the
CONNECTIONheader check is no longer necessary for the upgrade process.Let's verify if the
CONNECTIONheader is still being set in theSimpleUpgradeService:#!/bin/bash # Description: Check if CONNECTION header is still being set in SimpleUpgradeService # Test: Search for CONNECTION header usage in SimpleUpgradeService rg --type rust 'CONNECTION' common/src/simple_upgrade_service.rs
6-7: LGTM: Import changes align with test modifications.The removal of the
CONNECTIONimport fromhyper::headeris consistent with the subsequent changes in the test. This suggests that theCONNECTIONheader is no longer being explicitly set or checked in the upgrade process.To ensure this change doesn't impact other parts of the codebase, let's verify the usage of
CONNECTIONheader:✅ Verification successful
Verified: Import removal in
test_upgrade.rsdoes not affect other parts of the codebase.The
CONNECTIONheader is still used inplane/src/client/sse.rs, ensuring that its removal from the test filedynamic-proxy/tests/test_upgrade.rsis isolated and does not impact the main functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of CONNECTION header in the codebase # Test: Search for CONNECTION header usage rg --type rust 'CONNECTION'Length of output: 141
Script:
#!/bin/bash # Description: Perform a comprehensive search for any remaining usage of the CONNECTION header in Rust files across the entire codebase. # Search for 'CONNECTION' in all Rust files without specifying paths to avoid missing files. rg --type rust 'CONNECTION'Length of output: 199
dynamic-proxy/tests/test_http_redirect.rs (1)
Line range hint
1-85: LGTM: Tests remain comprehensive and intact.The changes made to this file are minimal and don't affect the overall functionality of the tests. The test cases for various HTTPS redirection scenarios (basic redirect, redirect with slash path, redirect with path, and redirect with query params) remain comprehensive and intact. They continue to assert the expected status codes and location headers, ensuring the correct behavior of the HTTPS redirect service.
🧰 Tools
🪛 GitHub Check: clippy
[warning] 33-33: returning the result of a
letbinding from a block
warning: returning the result of aletbinding from a block
--> dynamic-proxy/tests/test_http_redirect.rs:33:5
|
31 | let response = get_client().get(url).send().await.unwrap();
| ----------------------------------------------------------- unnecessaryletbinding
32 |
33 | response
| ^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
= note:#[warn(clippy::let_and_return)]on by default
help: return the expression directly
|
31 ~
32 |
33 ~ get_client().get(url).send().await.unwrap()
|dynamic-proxy/tests/common/simple_upgrade_service.rs (1)
34-34: Approval: Removal of redundant CONNECTION headerThe removal of the
CONNECTIONheader is appropriate and aligns with HTTP standards. According to RFC 7230 section 6.7, the presence of theUPGRADEheader field implies an implicitCONNECTIONheader field with the value "upgrade". Therefore, explicitly setting theCONNECTIONheader is redundant.This change simplifies the code without affecting the functionality of the upgrade process. The
UPGRADEheader andSWITCHING_PROTOCOLSstatus are still correctly set, which is sufficient for handling the upgrade request.dynamic-proxy/tests/graceful_https.rs (1)
53-53: Verify if sleep before shutdown is necessaryThe reduction in sleep duration from 600ms to 100ms is a good optimization. However, based on a similar test in
dynamic-proxy/tests/graceful.rs, it's worth considering if this sleep is necessary at all. If the server hasn't started up, the subsequent request would fail immediately, making the sleep potentially redundant.To verify if this sleep is necessary, you can try removing it and see if the test still passes consistently. If it does, you can safely remove this line.
dynamic-proxy/src/graceful_shutdown.rs (1)
15-15: LGTM: Deriving Clone enhances struct usabilityThe addition of
#[derive(Clone)]for theGracefulShutdownstruct is a good improvement. This allows for easy duplication of the struct, which can be beneficial in scenarios where multiple copies of the shutdown mechanism are needed. The change is consistent with the file's header comment and doesn't introduce any apparent issues, as the internalwatch::Sender<()>is itselfClone.plane/src/controller/error.rs (1)
3-3: Approve import change and suggest verification.The change from
reqwest::StatusCodetoaxum::http::StatusCodeis appropriate and aligns with the use of theaxumframework in this file. This modification enhances consistency in the codebase.To ensure all
StatusCodeusages have been updated consistently across the codebase, please run the following verification script:This will help identify any missed conversions from
reqwest::StatusCodetoaxum::http::StatusCodein other parts of the codebase.✅ Verification successful
All
StatusCodeimports are correctly updated.The replacement of
reqwest::StatusCodewithaxum::http::StatusCodehas been successfully applied in the main codebase. The remaining instances ofreqwest::StatusCodeare confined to test files, which is appropriate given their role in handling HTTP client operations within tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usages of reqwest::StatusCode # Test: Search for reqwest::StatusCode. Expect: No results rg --type rust 'reqwest::StatusCode' # Test: Search for axum::http::StatusCode. Expect: Results in relevant files rg --type rust 'axum::http::StatusCode'Length of output: 801
plane/plane-tests/tests/proxy_connection_monitor.rs (1)
Line range hint
1-124: Overall test structure and coverage looks goodThe test functions
proxy_marks_backend_as_recently_activeandproxy_marks_websocket_backend_as_activeare well-structured and provide comprehensive coverage of proxy behavior for both HTTP and WebSocket connections. They effectively test the marking of backends as active and recently connected.🧰 Tools
🪛 GitHub Check: clippy
[warning] 63-63: used
assert_eq!with a literal bool
warning: usedassert_eq!with a literal bool
--> plane/plane-tests/tests/proxy_connection_monitor.rs:63:5
|
63 | assert_eq!(backend_entry.had_recent_connection, true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
= note:#[warn(clippy::bool_assert_comparison)]on by default
help: replace it withassert!(..)
|
63 - assert_eq!(backend_entry.had_recent_connection, true);
63 + assert!(backend_entry.had_recent_connection);
|plane/src/proxy/request.rs (1)
50-50: Approve change with suggestions for improvementThe modification to return "/" instead of an empty string when there's no path after the token is a good improvement. It ensures that a valid path is always returned, which can prevent potential issues in downstream components.
However, to maintain the robustness of our test suite and clarity of the code, I suggest the following:
- Add new test cases to cover this modified behavior, especially since some tests were removed (as mentioned in the PR summary).
- Update the function's documentation to reflect this new behavior explicitly.
Here's a suggestion for additional test cases:
#[test] fn test_get_and_maybe_remove_bearer_token_no_path() { let mut parts = uri::Parts::default(); parts.path_and_query = Some(PathAndQuery::from_static("/token")); let token = get_and_maybe_remove_bearer_token(&mut parts); assert_eq!(token, Some(BearerToken::from("token"))); assert_eq!(parts.path_and_query, Some(PathAndQuery::from_static("/"))); }And for the documentation update:
/// Removes a connection string from the URI and returns it. /// If no connection string is found, returns None. /// If no path is provided after the token, "/" is used as the path. pub fn get_and_maybe_remove_bearer_token(parts: &mut uri::Parts) -> Option<BearerToken> { // ... existing implementation ... }To ensure that this change doesn't introduce any regressions, please run the following command:
This will help us identify any other parts of the codebase that might need to be updated to accommodate this change.
✅ Verification successful
<!-- [monologue] [task verify_usage_1/1] Review the usage of `get_and_maybe_remove_bearer_token` after the change. [observations] - The shell script identified only one usage in `plane/src/proxy/proxy_server.rs`. - The usage involves extracting the bearer token and handling the case where it might be `None`. - The change in `request.rs` ensures that a "/" is returned instead of an empty string when there's no path. - The downstream code checks for `Some(bearer_token)` and handles `None` by returning a `BAD_REQUEST`. [analysis] - Since the only usage properly handles the `None` case, the change to return "/" does not introduce any issues. - There are no other usages that could be affected by this change. - The existing test case covers the scenario where there's no path after the token. [actions] - Conclude that the change is verified and does not introduce any issues. -->Change Verified
The function
get_and_maybe_remove_bearer_tokenis only used inproxy_server.rs. The modification to return "/" instead of an empty string does not adversely affect its usage and maintains the correctness of downstream handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any uses of get_and_maybe_remove_bearer_token that might be affected by this change rg --type rust "get_and_maybe_remove_bearer_token" -C 5Length of output: 2228
plane/src/proxy/mod.rs (2)
43-43: Clarification needed on documentation changeThe documentation for the
http_portfield has been modified to remove information about its relationship withhttps_port. While this change might simplify the comment, it could potentially remove valuable context for developers.Could you please clarify the reasoning behind this change? It would be helpful to understand if:
- The removed information was inaccurate or outdated.
- The relationship between
http_portandhttps_portis explained elsewhere in the codebase.- This change is part of a larger documentation update strategy.
Consider whether the removed information provided valuable context for understanding the interplay between
http_portandhttps_port. If it did, we might want to retain a modified version of this information or add a cross-reference to where this relationship is explained in more detail.
Line range hint
1-184: LGTM for the rest of the fileThe changes in this file are limited to the documentation comment we've discussed. The rest of the file remains unchanged and appears to be functioning as intended. No further issues or concerns were identified during the review.
dynamic-proxy/src/proxy.rs (1)
71-71: LGTM! Consistent error logging.The change from
warn!toerror!is consistent with the previous change and appropriately reflects the severity of an upstream request failure.plane/src/proxy/proxy_server.rs (1)
238-238: Improved error handling in user data serialization.The change from
expecttounwrap_or_default()is a significant improvement in error handling. This modification prevents potential panics and makes the code more robust by providing a default value when serialization fails.This change aligns with best practices for production-ready code, enhancing the overall reliability of the application.
plane/plane-tests/tests/common/test_env.rs (4)
108-108: LGTM: Asynchronous TCP listener creationThe change to use
TcpListener::bind(...).awaitis appropriate and aligns with the transition to asynchronous TCP listener creation usingtokio::net::TcpListener. This enhancement improves the overall asynchronous capabilities of the code.
166-166: LGTM: Consistent asynchronous TCP listener creationThe change to use
TcpListener::bind(...).awaitin thecontroller_with_forward_authmethod is consistent with the previous modification in thecontrollermethod. This maintains uniformity across the codebase for TCP listener creation and enhances the overall asynchronous capabilities.
Line range hint
279-283: LGTM: Consistent use of tokio::net::TcpListenerThe
dnsmethod correctly usestokio::net::TcpListener::bindfor creating the listener, which is consistent with the changes made in other methods. This maintains uniformity in the use of asynchronous TCP listener creation throughout the codebase.
Line range hint
1-424: Summary: Successful transition to asynchronous TCP listener creationThe changes in this file consistently update the TCP listener creation process to use
tokio::net::TcpListener, enhancing the asynchronous capabilities of the test environment. This transition affects thecontroller,controller_with_forward_auth, anddnsmethods, ensuring uniformity across the codebase. These modifications improve the overall efficiency and scalability of the test environment setup.plane/src/controller/forward_auth.rs (4)
2-5: Updated imports align with axum module restructuringThe imports have been updated to correctly reference the necessary modules from
axum. ImportingBody,Bytes,Request,State,Uri,request,HeaderValue, andStatusCodereflects the reorganization in the latest versions ofaxum.
15-17:clone_request_with_empty_bodyreturn type correctly updatedThe return type of
clone_request_with_empty_bodyhas been updated tohyper::http::Request<Body>, which aligns with the changes in thehyperlibrary and ensures compatibility with the updated request handling.
39-39: Simplifiedforward_layerfunction signature improves clarityThe
forward_layerfunction signature has been simplified by removing the generic type parameter<B>and using concrete types forRequestandResponse. This enhances readability and aligns with the updatedaxummiddleware patterns.
72-72: Creation of body from static bytes is appropriateUsing
Body::from(Bytes::from_static(body))efficiently creates a response body from static byte slices, which is suitable for the fixed responses in this context.plane/src/client/sse.rs (7)
4-4: ImportingHeaderValuefromreqwest::headerThe import statement correctly brings in
HeaderValue,ACCEPT, andCONNECTIONfromreqwest::header, ensuring that the appropriate header types are used for HTTP requests.
179-179: Added import ofHeaderMapfor handling request headersIncluding
http::HeaderMapis necessary for accessing and manipulating the HTTP headers in thehandle_ssefunction.
187-187: Imported necessarytokioutilities for asynchronous operationsThe addition of
TcpListener,broadcast,JoinHandle, andtimeoutfromtokiois appropriate for setting up the asynchronous server and handling communication in the test environment.
239-241: UpdatedDemoSseServer::newto be asynchronousThe
newmethod ofDemoSseServeris now asynchronous to accommodate theawaitonTcpListener::bind. This change is necessary and aligns with the asynchronous nature of the binding operation.
248-249: ReplacedServer::from_tcpwithaxum::serveSwitching to
axum::servesimplifies the server creation process and reflects updates in the Axum framework. This change enhances code clarity and aligns with best practices.
271-271: Updated test to await the asynchronousDemoSseServer::newIn the
test_simple_ssetest, awaitingDemoSseServer::new().awaitis necessary due to the method becoming asynchronous. This ensures the test functions correctly with the updated constructor.
286-286: Updated test to await the asynchronousDemoSseServer::newIn the
test_sse_reconnecttest, the call toDemoSseServer::new()is now correctly awaited to match the updated asynchronous constructor, maintaining test integrity.plane/src/controller/mod.rs (3)
155-155: Confirm asynchronous binding ofTcpListenerThe use of
awaitwithTcpListener::bindis appropriate, as it is now an asynchronous operation. This change ensures proper handling of any async initialization within the binding process.
243-246: Verifyforward_layercompatibility withfrom_fn_with_stateEnsure that the
forward_layerfunction signature matches the requirements ofaxum::middleware::from_fn_with_state. The middleware function should accept the state (Urlin this case) as the first argument, followed by the request and next handler.#!/bin/bash # Description: Check that `forward_layer` accepts state as its first parameter. # Expected: Function signature starts with `pub async fn forward_layer(state: Url, ...` rg --type rust '^pub async fn forward_layer' -A 2
149-149: Ensure consistent error handling with updatedserver_handletypeThe
server_handlefield's type has changed toOption<JoinHandle<Result<(), std::io::Error>>>. Make sure that all usages ofserver_handle, especially when awaiting its result, correctly handlestd::io::Errorinstead ofhyper::Error.
| let response = get_client().get(url).send().await.unwrap(); | ||
|
|
||
| get_client().get(url).send().await.unwrap() | ||
| response |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reverting to a more concise implementation.
The current implementation assigns the response to a variable before returning it. While this approach can be beneficial for debugging or future modifications, it introduces unnecessary verbosity in its current form.
Consider reverting to the more concise version:
- let response = get_client().get(url).send().await.unwrap();
-
- response
+ get_client().get(url).send().await.unwrap()This change aligns with the Clippy suggestion to avoid unnecessary let bindings. However, if there are plans to add more operations on the response before returning, or if the named variable significantly aids in debugging, the current implementation may be justified.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let response = get_client().get(url).send().await.unwrap(); | |
| get_client().get(url).send().await.unwrap() | |
| response | |
| get_client().get(url).send().await.unwrap() |
🧰 Tools
🪛 GitHub Check: clippy
[warning] 33-33: returning the result of a
letbinding from a block
warning: returning the result of aletbinding from a block
--> dynamic-proxy/tests/test_http_redirect.rs:33:5
|
31 | let response = get_client().get(url).send().await.unwrap();
| ----------------------------------------------------------- unnecessaryletbinding
32 |
33 | response
| ^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
= note:#[warn(clippy::let_and_return)]on by default
help: return the expression directly
|
31 ~
32 |
33 ~ get_client().get(url).send().await.unwrap()
|
| use plane::client::{PlaneClient, PlaneClientError}; | ||
| use plane_test_macro::plane_test; | ||
| use reqwest::StatusCode; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent StatusCode Imports Detected
There are remaining imports of hyper::StatusCode in the following files:
dynamic-proxy/tests/graceful_https.rsdynamic-proxy/tests/graceful.rsdynamic-proxy/tests/test_http_versions.rsdynamic-proxy/tests/hello_world_http.rs
Please update these imports to reqwest::StatusCode to ensure consistency across the codebase.
🔗 Analysis chain
LGTM! Verify consistency across the codebase.
The change from hyper::StatusCode to reqwest::StatusCode looks good. This modification aligns with the use of reqwest as the HTTP client library. The StatusCode enum should have the same variants for standard HTTP status codes in both libraries, ensuring that the existing test cases remain valid.
To ensure consistency across the codebase, please run the following script:
This script will help identify any inconsistencies in StatusCode usage across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of StatusCode across the codebase
# Test 1: Check for any remaining imports of hyper::StatusCode
echo "Checking for remaining hyper::StatusCode imports:"
rg --type rust "use hyper::StatusCode"
# Test 2: Verify that all StatusCode usages are now from reqwest
echo "Verifying reqwest::StatusCode usage:"
rg --type rust "StatusCode::"
# Test 3: Check for any potential missed conversions
echo "Checking for potential missed conversions:"
rg --type rust "hyper::.*StatusCode"
Length of output: 7586
There was a problem hiding this comment.
This is intentional; we want to avoid breaking things if the version of hyper depended on by reqwest and axum differs.
There was a problem hiding this comment.
@paulgb, understood. Thank you for the clarification. I'll keep this in mind for future reviews.
(_/)
(o.o)
( )?
✏️ Learnings added
Learnt from: paulgb
PR: jamsocket/plane#817
File: plane/plane-tests/tests/forward_auth.rs:4-4
Timestamp: 2024-10-07T06:31:19.148Z
Learning: In the Rust codebase, importing `StatusCode` from `hyper`, `axum`, and `reqwest` in different files is intentional to avoid breaking things if the versions of `hyper` depended on by these crates differ. Maintaining separate imports prevents potential compatibility issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| let _client = client.clone(); | ||
| let _url = url.clone(); |
There was a problem hiding this comment.
Remove underscores from variable names
The variables _client and _url are used in the following line, so they shouldn't be prefixed with underscores. Underscores are typically used to indicate intentionally unused variables, which is not the case here.
Apply this diff to fix the variable names:
- let _client = client.clone();
- let _url = url.clone();
+ let client = client.clone();
+ let url = url.clone();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let _client = client.clone(); | |
| let _url = url.clone(); | |
| let client = client.clone(); | |
| let url = url.clone(); |
| @@ -13,6 +12,7 @@ use plane::{ | |||
| }, | |||
| }; | |||
| use plane_test_macro::plane_test; | |||
| use reqwest::StatusCode; | |||
There was a problem hiding this comment.
💡 Codebase verification
Consistency Issue Detected: Remaining hyper::StatusCode usages found.
The following files still import hyper::StatusCode:
dynamic-proxy/tests/graceful_https.rsdynamic-proxy/tests/graceful.rsdynamic-proxy/tests/test_http_versions.rsdynamic-proxy/tests/hello_world_http.rs
Please update these imports to reqwest::StatusCode to maintain consistency across the project.
🔗 Analysis chain
LGTM! Verify consistency across the project.
The change from hyper::StatusCode to reqwest::StatusCode is correct and doesn't introduce any issues in this file. The StatusCode::INTERNAL_SERVER_ERROR used in the no_drone_available test function is available in both libraries, ensuring the test continues to function as expected.
To ensure consistency across the project, please run the following script to check for any remaining hyper::StatusCode usages:
This will help identify any other files that might need similar updates to maintain consistency in HTTP client library usage throughout the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining hyper::StatusCode usages in the project
# Test: Search for hyper::StatusCode usages
rg --type rust 'hyper::StatusCode'
# Test: Search for reqwest::StatusCode usages to confirm the change is consistent
rg --type rust 'reqwest::StatusCode'
Length of output: 848
| @@ -48,7 +48,7 @@ pub async fn forward_layer<B>( | |||
| *forward_req.uri_mut() = uri; | |||
|
|
|||
| // Create a client | |||
| let client = Client::new(); | |||
| let client = Client::builder(TokioExecutor::new()).build(HttpConnector::new()); | |||
There was a problem hiding this comment.
This does not work in hyper 1.x.
| use hyper_util::{ | ||
| client::legacy::{connect::HttpConnector, Client}, | ||
| rt::TokioExecutor, | ||
| }; |
There was a problem hiding this comment.
Hyper 1.x removed Client (see: https://hyper.rs/guides/1/upgrading/)
There was a problem hiding this comment.
@paulgb, thank you for pointing that out. You're correct that in hyper 1.x, the Client type has been removed, so the standard hyper::Client API is not applicable here. I'll make sure to consider this in future reviews.
(_/)
(o.o)
(>?<)
✏️ Learnings added
Learnt from: paulgb
PR: jamsocket/plane#817
File: plane/src/controller/forward_auth.rs:9-12
Timestamp: 2024-10-07T06:32:51.246Z
Learning: In hyper version 1.x, the `Client` type has been removed, and the `hyper_util` crate provides the client functionality.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| let server_handle = tokio::spawn(async { | ||
| axum::serve( | ||
| listener, | ||
| app.into_make_service_with_connect_info::<SocketAddr>(), | ||
| ) | ||
| .with_graceful_shutdown(async { | ||
| graceful_terminate_receiver.await.ok(); | ||
| }) | ||
| .await | ||
| }); |
There was a problem hiding this comment.
Replace axum::serve with correct server startup method
The function axum::serve does not exist. To start the server correctly with the updated axum and hyper versions, use hyper::Server::from_tcp(listener)? and configure it appropriately.
Apply this diff to fix the server startup:
-let server_handle = tokio::spawn(async {
- axum::serve(
- listener,
- app.into_make_service_with_connect_info::<SocketAddr>(),
- )
- .with_graceful_shutdown(async {
- graceful_terminate_receiver.await.ok();
- })
- .await
-});
+let server_handle = tokio::spawn(async {
+ hyper::Server::from_tcp(listener)?
+ .serve(app.into_make_service_with_connect_info::<SocketAddr>())
+ .with_graceful_shutdown(async {
+ graceful_terminate_receiver.await.ok();
+ })
+ .await
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let server_handle = tokio::spawn(async { | |
| axum::serve( | |
| listener, | |
| app.into_make_service_with_connect_info::<SocketAddr>(), | |
| ) | |
| .with_graceful_shutdown(async { | |
| graceful_terminate_receiver.await.ok(); | |
| }) | |
| .await | |
| }); | |
| let server_handle = tokio::spawn(async { | |
| hyper::Server::from_tcp(listener)? | |
| .serve(app.into_make_service_with_connect_info::<SocketAddr>()) | |
| .with_graceful_shutdown(async { | |
| graceful_terminate_receiver.await.ok(); | |
| }) | |
| .await | |
| }); |
There was a problem hiding this comment.
this axum API has been changed, this is the right way.
There was a problem hiding this comment.
@paulgb, understood. Thank you for the clarification that axum::serve is the correct API in the updated version of Axum.
🐰❓
✏️ Learnings added
Learnt from: paulgb
PR: jamsocket/plane#817
File: plane/src/controller/mod.rs:272-281
Timestamp: 2024-10-07T06:33:57.304Z
Learning: In the updated version of Axum, `axum::serve` is the correct method to start the server, replacing `hyper::Server::from_tcp`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
paulgb
force-pushed
the
paul/dis-2737-update-axum-version-in-proxy
branch
from
34deae0 to
3b4eccd
Compare
| use hyper_util::{ | ||
| client::legacy::{connect::HttpConnector, Client}, | ||
| rt::TokioExecutor, | ||
| }; |
There was a problem hiding this comment.
Hyper 1.x removed Client (see: https://hyper.rs/guides/1/upgrading/)
| @@ -48,7 +48,7 @@ pub async fn forward_layer<B>( | |||
| *forward_req.uri_mut() = uri; | |||
|
|
|||
| // Create a client | |||
| let client = Client::new(); | |||
| let client = Client::builder(TokioExecutor::new()).build(HttpConnector::new()); | |||
There was a problem hiding this comment.
This does not work in hyper 1.x.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
plane/src/client/sse.rs (2)
179-181: LGTM: Improved type safety and efficiencyThe use of
HeaderValue::from_staticfor setting theACCEPTandCONNECTIONheaders improves type safety and efficiency. This is a good change.Consider extracting these static header values as constants at the top of the file for better maintainability:
const ACCEPT_HEADER_VALUE: HeaderValue = HeaderValue::from_static("text/event-stream"); const CONNECTION_HEADER_VALUE: HeaderValue = HeaderValue::from_static("keep-alive");Then use these constants in the
ensure_streammethod.
239-249: LGTM: Improved async handling and updated Axum API usageThe changes to make
newasync and useaxum::serveimprove the robustness of the test server setup and align with the latest Axum API. These are good improvements.Consider adding a comment explaining why
newis now async, as it might not be immediately obvious to future readers:/// Creates a new DemoSseServer. /// /// This method is async to allow for proper handling of the asynchronous TcpListener::bind operation. async fn new() -> Self { // ... existing implementation ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
- plane/Cargo.toml (3 hunks)
- plane/plane-tests/Cargo.toml (1 hunks)
- plane/plane-tests/tests/common/localhost_resolver.rs (1 hunks)
- plane/plane-tests/tests/common/test_env.rs (2 hunks)
- plane/plane-tests/tests/forward_auth.rs (1 hunks)
- plane/src/client/sse.rs (5 hunks)
- plane/src/controller/connect.rs (1 hunks)
- plane/src/controller/error.rs (1 hunks)
- plane/src/controller/forward_auth.rs (4 hunks)
- plane/src/controller/mod.rs (4 hunks)
- plane/src/proxy/proxy_server.rs (1 hunks)
- plane/src/proxy/request.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- plane/Cargo.toml
- plane/plane-tests/Cargo.toml
- plane/plane-tests/tests/common/localhost_resolver.rs
- plane/plane-tests/tests/common/test_env.rs
- plane/plane-tests/tests/forward_auth.rs
- plane/src/controller/connect.rs
- plane/src/controller/error.rs
- plane/src/controller/mod.rs
- plane/src/proxy/proxy_server.rs
- plane/src/proxy/request.rs
🧰 Additional context used
🔇 Additional comments (6)
plane/src/controller/forward_auth.rs (3)
15-15: Approved: Return type update enhances type consistencyThe change in the return type from
request::Request<Body>tohyper::http::Request<Body>is a good adjustment. It aligns the function with the direct use ofhypertypes, which is consistent with the overall changes in the file.This change improves type consistency and makes the code's dependencies more explicit.
71-72: Approved: Simplified response creationThe changes in the
response_helperfunction are a good improvement. ReturningResponseinstead ofResponse<BoxBody>and creating the body directly from static bytes simplifies the response creation process.This change:
- Reduces complexity in the function signature.
- Simplifies the body creation process.
- Aligns with modern Rust HTTP handling practices.
These modifications make the code more straightforward and easier to maintain.
39-39: Approved: Simplified function signature, verify impactThe simplification of the
forward_layerfunction signature by removing the generic type parameterBand directly usingRequestandNextis a good change. It makes the function more straightforward and easier to understand.To ensure this change doesn't have unintended consequences, please run the following script to check for any other usages of this function that might be affected:
If the script returns any results, those locations may need to be updated to match the new signature.
✅ Verification successful
Verified: No incompatible usages found
The changes to the
forward_layerfunction signature have been verified. There are no usages with generic type parameters, and existing usages are compatible with the updated signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of forward_layer function # Expected result: No incompatible usages found rg --type rust "forward_layer\s*<" .Length of output: 38
Script:
#!/bin/bash # Description: Search for all usages of forward_layer function in the Rust codebase # Expected result: List of all locations where forward_layer is used # Using ripgrep to find all instances of forward_layer rg --type rust "forward_layer" . # Additionally, using ast-grep to find function calls to forward_layer ast-grep --lang rust --pattern 'forward_layer($_)' .Length of output: 349
plane/src/client/sse.rs (3)
4-5: LGTM: Improved import consistencyThe change to import
HeaderValuefromreqwest::headerinstead oftungstenite::httpimproves consistency with the rest of the codebase, asreqwestis used for HTTP requests in this file.
179-187: LGTM: Appropriate import additions and reorderingThe new imports (
HeaderMapandTcpListener) are necessary for the updated test implementation. The reordering of imports improves readability and follows Rust's convention of grouping imports by crate.
271-271: LGTM: Proper async handling in testsThe changes to await
DemoSseServer::new()in the test cases are correct and necessary due to thenewmethod becoming async. This ensures proper initialization of the test server before proceeding with the tests.Also applies to: 286-286
| @@ -48,7 +48,7 @@ pub async fn forward_layer<B>( | |||
| *forward_req.uri_mut() = uri; | |||
|
|
|||
| // Create a client | |||
| let client = Client::new(); | |||
| let client = Client::builder(TokioExecutor::new()).build(HttpConnector::new()); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the HTTP client construction
The client is currently constructed using a specific executor. It's possible to simplify this by using hyper::Client::builder() directly, which uses the default executor and configurations.
You can modify the client creation as follows:
- let client = Client::builder(TokioExecutor::new()).build(HttpConnector::new());
+ let client = Client::builder().build(HttpConnector::new());This change removes the need for TokioExecutor and utilizes the default settings, making the code simpler and more maintainable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let client = Client::builder(TokioExecutor::new()).build(HttpConnector::new()); | |
| let client = Client::builder().build(HttpConnector::new()); |
| use hyper_util::{ | ||
| client::legacy::{connect::HttpConnector, Client}, | ||
| rt::TokioExecutor, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating to the current hyper client API
The code is still using hyper_util::client::legacy to build the HTTP client. Since the legacy module is intended for backward compatibility, it's recommended to update to the standard hyper::Client API to ensure future compatibility and take advantage of improvements in the newer versions.
You could replace the client construction with:
-use hyper_util::{
- client::legacy::{connect::HttpConnector, Client},
- rt::TokioExecutor,
-};
+use hyper::{Client, client::HttpConnector};
// ... later in the code ...
- let client = Client::builder(TokioExecutor::new()).build(HttpConnector::new());
+ let client = Client::builder().build(HttpConnector::new());This change simplifies the client creation and removes the dependency on TokioExecutor.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use hyper_util::{ | |
| client::legacy::{connect::HttpConnector, Client}, | |
| rt::TokioExecutor, | |
| }; | |
| use hyper::{Client, client::HttpConnector}; |
There was a problem hiding this comment.
hyper::Client no longer exists.
There was a problem hiding this comment.
@paulgb, thank you for pointing that out. You are correct; hyper::Client no longer exists in hyper version 1.x, and the hyper_util crate provides the client functionality. I apologize for the oversight.
(_/)
(•_•)
< |
🧠 Learnings used
Learnt from: paulgb
PR: jamsocket/plane#817
File: plane/src/controller/forward_auth.rs:9-12
Timestamp: 2024-10-07T06:32:51.429Z
Learning: In hyper version 1.x, the `Client` type has been removed, and the `hyper_util` crate provides the client functionality.
Learnt from: paulgb
PR: jamsocket/plane#817
File: plane/src/controller/forward_auth.rs:51-51
Timestamp: 2024-10-07T06:33:34.659Z
Learning: In `plane/src/controller/forward_auth.rs`, when using hyper 1.x in Rust, constructing a `Client` with `Client::builder().build(HttpConnector::new())` does not work without specifying an executor. The executor must be specified using `TokioExecutor::new()`, as in `Client::builder(TokioExecutor::new()).build(HttpConnector::new())`.
paulgb
force-pushed
the
paul/dis-2737-update-axum-version-in-proxy
branch
from
3b4eccd to
27aaf91
Compare
Will be rebased after #814 is merged, but otherwise ready for eyes.