Migrate query api to rust #45
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .await | ||
| { | ||
| Ok(response) => { | ||
| response |
There was a problem hiding this comment.
I'm not sure if the DAPR SDK throws an error for a non 200 response... you may need to also check the response status code, and pass the error message back to the calling app.
There was a problem hiding this comment.
That is correct. You need something like this using the is_success function:
match response {
Ok(res) => {
if res.status().is_success() {
Ok(())
} else {
...
}
},
Err(e) => ...
}
All of our calls like this need to be fixed. There are some in the comms abstractions.
There was a problem hiding this comment.
I'm currently debating whether we should handle this status check in the actual dapr invoker or in the code after we executed the invoke() function. I'm leaning towards handling this in the dapr invoker (in comms abstractions)
sources/shared/query-api/src/main.rs
Outdated
| } | ||
| Err(e) => { | ||
| println!("Error invoking the acquire method on the proxy: {:?}", e); | ||
| return StatusCode::INTERNAL_SERVER_ERROR.into_response(); |
There was a problem hiding this comment.
I think we could add an error message to the body of these INTERNAL_SERVER_ERROR responses
sources/shared/query-api/src/main.rs
Outdated
| debug!("Published the subscription event"); | ||
| } | ||
| Err(e) => { | ||
| println!("Error publishing the subscription event: {:?}", e); |
There was a problem hiding this comment.
Instead of println, lets use log::error or log::info
| } | ||
|
|
||
| #[derive(Serialize)] | ||
| pub struct AfterData { |
There was a problem hiding this comment.
The AfterData name is really throwing me. Could we think of a name that better describes this?
There was a problem hiding this comment.
How about SubscriptionData? Since this is solely used for sending the controlevent in a subscription?
| @@ -36,5 +36,5 @@ pub trait Publisher { | |||
| &self, | |||
| data: Value, | |||
| headers: Headers, | |||
| ) -> Result<(), Box<dyn std::error::Error>>; | |||
| ) -> Result<reqwest::Response, Box<dyn std::error::Error>>; | |||
There was a problem hiding this comment.
I don't think we should include reqwest::Response as part of the API of drasi comms. It tightly couples us to a specific library.
There was a problem hiding this comment.
How about a custom struct that contains the status code and/or the response body? Something like:
pub struct HttpResponse {
pub status_code: u16,
pub body: String,
}There was a problem hiding this comment.
That way we can still pass the status code and the ebody
There was a problem hiding this comment.
That might make sense for the Invoker, but the publisher implementation could be one that does not use Http, if we choose to not use Dapr in the future, many messages brokers have their own protocols.
I think if a non success code comes from the Dapr sidecar, the publish was unsuccessful, and we just return an error, and if the body contains details, we include that in the error result.
There was a problem hiding this comment.
that makes sense. for a successful service invocation, maybe we can just return the body as Bytes?
There was a problem hiding this comment.
oh actually this is the publisher, we don't really need to return anything if the publish is successful
There was a problem hiding this comment.
Sorry I was think about the wrong thing
sources/shared/query-api/src/main.rs
Outdated
| } | ||
| Err(e) => {} // | ||
| }, | ||
| None => {} |
There was a problem hiding this comment.
We could log a warning if the trace headers are missing.
| @@ -26,7 +26,7 @@ pub trait Invoker { | |||
| app_id: String, | |||
| method: String, | |||
| headers: Headers, | |||
| ) -> Result<(), Box<dyn std::error::Error>>; | |||
| ) -> Result<serde_json::Value, Box<dyn std::error::Error>>; | |||
There was a problem hiding this comment.
We can't assume the response will always be json
Description
Migrate Query API to Rust
Type of change
Fixes: #issue_number