-
Notifications
You must be signed in to change notification settings - Fork 0
Hyper As Base for Rest Adapter #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Migrates the REST adapter to use hyper directly as the HTTP server foundation (addressing issue #132) and removes the now-redundant in-repo crates/http implementation.
Changes:
- Replaced the custom
httpcrate usage in the REST adapter with ahyper-based HTTP/1 server. - Introduced request routing via
RequestRoute(method + URL) and extracted server config into a dedicated module. - Removed
crates/httpfrom the workspace and updated dependencies/lockfile accordingly.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
adapter/rest/src/main.rs |
Implements a hyper-based server loop, request handling, and flow execution response mapping. |
adapter/rest/src/route.rs |
Adds method+URL-based flow identification and slug extraction helper. |
adapter/rest/src/config.rs |
Defines HTTP server config and environment loading defaults. |
adapter/rest/Cargo.toml |
Swaps http workspace crate dependency for hyper/hyper-util/http-body-util. |
crates/base/src/store.rs |
Adds logging around flow execution requests (but currently introduces a move/compile issue). |
Cargo.toml |
Removes crates/http from workspace members and workspace dependency overrides. |
Cargo.lock |
Updates lockfile to reflect dependency changes and removal of the local http crate. |
crates/http/* |
Removes the legacy custom HTTP implementation from the repository. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn json_error(status: StatusCode, msg: &str) -> Response<Full<Bytes>> { | ||
| let body = format!(r#"{{"error": "{}"}}"#, msg); | ||
| Response::builder() | ||
| .status(status) | ||
| .header("content-type", "application/json") | ||
| .body(Full::new(Bytes::from(body))) | ||
| .unwrap() |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json_error interpolates msg directly into a JSON string without escaping, so quotes/newlines in msg can produce invalid JSON. Build the body via serde_json (or at least JSON-escape msg) before returning.
| let method = req.method().clone(); | ||
| let path = req.uri().path().to_string(); | ||
|
|
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path is taken from req.uri().path(), which drops the query string. Previously the request parser included ?query in path and also exposed parsed query params in the body structure, so route matching and flow inputs can change. If query strings matter for routing or inputs, use uri().path_and_query() and/or parse the query into a Value like before.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if the request is matching | ||
| match regex::Regex::new(regex_str) { | ||
| Ok(regex) => { | ||
| log::debug!("Successfully compiled regex: {}", regex_str); | ||
| regex.is_match(&self.url) | ||
| } | ||
| Err(err) => { | ||
| log::error!("Failed to compile regex: {}", err); | ||
| false | ||
| } | ||
| } |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identify compiles a new Regex for every candidate flow (Regex::new(regex_str)), which can become a hot path since get_possible_flow_match iterates over all matching keys/flows. Consider caching compiled regexes by pattern string (e.g., LRU/once_cell cache) or storing a precompiled regex in the flow representation if possible.
| fn json_error(status: StatusCode, msg: &str) -> Response<Full<Bytes>> { | ||
| let body = format!(r#"{{"error": "{}"}}"#, msg); | ||
| Response::builder() | ||
| .status(status) | ||
| .header("content-type", "application/json") | ||
| .body(Full::new(Bytes::from(body))) | ||
| .unwrap() |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json_error interpolates msg directly into a JSON string. If msg contains quotes/backslashes/newlines, the response becomes invalid JSON (and can lead to confusing client behavior). Build the body via serde_json (or properly escape the string) instead of manual string formatting.
| // headers struct | ||
| let Value { | ||
| kind: | ||
| Some(StructValue(Struct { | ||
| fields: header_fields, | ||
| Some(Kind::ListValue(ListValue { | ||
| values: header_fields, | ||
| })), | ||
| } = headers | ||
| } = headers_val | ||
| else { | ||
| return None; | ||
| return json_error( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| "headers was not a list of header entries", | ||
| ); | ||
| }; |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headers_val is a &Value (from fields.get(...)), but this destructuring pattern matches against an owned Value. This will not compile. Match on a reference (e.g., &Value { .. }) or use headers_val.kind.as_ref() / if let Some(Kind::ListValue(...)) = headers_val.kind.as_ref() to avoid moving out of a borrowed value.
| // status_code number | ||
| let Some(Kind::NumberValue(code)) = status_code_val.kind else { | ||
| return json_error( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| "status_code was not a number", | ||
| ); | ||
| }; |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status_code_val is a &Value, so status_code_val.kind attempts to move the kind field out of a borrowed value and will not compile. Use status_code_val.kind.as_ref() (and then map/clone as needed) to read it without moving.
| let method = req.method().clone(); | ||
| let path = req.uri().path().to_string(); | ||
|
|
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle_request uses req.uri().path(), which drops the query string. Previously the full request target (including ?query=...) could be used for matching and/or input construction. If flow matching or downstream logic depends on query parameters, consider using uri().path_and_query() and/or parsing the query into the Value input passed to validate_and_execute_flow.
| let value: Option<Value> = if body.is_empty() { | ||
| None | ||
| } else { | ||
| match prost::Message::decode(body.as_slice()) { |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prost::Message::decode(...) cannot be called directly on the Message trait because the concrete Self type is unknown; this will not compile. Decode using the concrete type (e.g., Value::decode(...) or <Value as prost::Message>::decode(...)).
| match prost::Message::decode(body.as_slice()) { | |
| match Value::decode(body.as_slice()) { |
| } | ||
|
|
||
| let uuid = uuid::Uuid::new_v4().to_string(); | ||
| let flow_id = flow.flow_id; |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flow_id is moved out of flow (let flow_id = flow.flow_id;) and then flow is moved into convert_validation_flow(flow, ...). Since flow_id is not Copy, this leaves flow partially moved and will not compile. Use let flow_id = flow.flow_id.clone(); (or borrow it via &flow.flow_id for logging) before passing flow onward.
| let flow_id = flow.flow_id; | |
| let flow_id = flow.flow_id.clone(); |
| let value: Option<Value> = if body.is_empty() { | ||
| None | ||
| } else { | ||
| match prost::Message::decode(body.as_slice()) { | ||
| Ok(v) => Some(v), | ||
| Err(e) => { | ||
| log::warn!("Failed to decode request body as protobuf Value: {}", e); | ||
| return json_error( | ||
| StatusCode::BAD_REQUEST, | ||
| "Failed to decode request body as protobuf Value", | ||
| ); | ||
| } | ||
| } | ||
| }; |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The request body is decoded as a protobuf-encoded tucana::shared::Value. For an HTTP/REST adapter this is a behavior change vs typical JSON payloads (and the prior implementation built Value from JSON). If clients send JSON, this will always return 400. Consider decoding based on Content-Type (e.g., JSON -> Value) and preserving the previous REST semantics.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
Resolves: #132