Actors API - Githubissues

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

Overall, the Pull Request titled "Actors API" contains various changes related to the Actor APIs in the code. However, there are several potential issues that need to be addressed, including the lack of comments explaining the changes, overly simple error messages, and unclear motivations for certain changes. Additionally, there are changes that could cause issues for existing code, such as the removal of certain methods and changes to the return types of underlying functions. It's important to clarify the need for such changes and communicate them thoroughly to collaborators. Overall, the PR contains several valuable changes that could enhance the codebase, but it is important to ensure that the changes are thoroughly tested and documented.

Details

Commit 9461a212761569023e07fcbfc4e23d9b24500e4c

Key Changes:

The patch adds several methods related to Actor API to the code, such as actor_invoke_method, actor_state, actor_create_reminder, etc.

Potential problems:

The patch lacks comments to explain the changes made to the code, which might make it harder for other developers to understand the underlying changes.
The patch uses println! statements for debugging purposes. It would be better to remove them once the code is deployed to production.
The error messages returned by the functions are not very informative, which can make debugging difficult in case of errors.
It is unclear if appropriate validations have been added to ensure that the inputs are in the expected format by the API-endpoint called by the functions.

Commit 79cb9df8a8def8ae82fe40f020aedd99161e8163

Key changes:

A new API for getting configuration details of entities and their actors has been added
APIs for deleting, performing an action, checking rebels using reminder and timer methods have been added

Potential problems:

The commenting out of methods actor_invoke_reminder(), actor_invoke_timer() and actor_deactivate() without any explanation raises some doubts on whether these methods were not required anymore or just temporarily disabled
Similarly, there is no explanation in the pull request for the changes made in the example and test files.

Commit f3884a7c725608f0121d6f20d6a9f5a42c366466

The patch fixes the issue with response body not being converted to a string in the Echo example API. The change is made in the handle_request function where the response is converted to a string before being returned. The change is specific to the Echo example API and doesn't seem to have any potential problems.

Commit 26a503ca548aebdb7893cf682d0c2adcde43dcc9

The patch removes user space methods for invoking reminders, timers, and deactivating actors. The most significant change is the removal of actor_invoke_reminder and actor_invoke_timer methods. This might lead to issues if someone has implemented code based on these methods. Along with this, the client.actor_deactivate method has also been removed. This change may cause problems if there was any code that used this method to deactivate actors.

It's unclear why the user space methods were removed, but it could be because they were redundant, or their implementation was causing issues. It's essential to communicate the rationale behind such changes to collaborators, especially if it involves a breaking change like this.

Commit 936aa581c4ca437cf4d67bc3194b0e286cb18c5f

The patch changes the configuration of actor types in an example codebase. Specifically, it changes the entities field to use only a single entity type (stormtrooper) and updates the actorIdleTimeout field in entitiesConfig from "1m" to "10m".

One potential issue that I see is that this change is specific to the echo example code, and does not account for the possibility of multiple entity types being used in different programs. Also, without context for the change, it's unclear why the configuration was modified in this way. Additionally, there may be other changes related to actor types that would be necessary to fully implement this change.

Commit 8d7887185b62e670710b5cb4cde22f7c31ea4e32

The patch changes the value of "actorScanInterval" in the echo application's configuration to "1h" from "30s". This might cause actors to linger and consume resources unnecessarily or generate silent failures when they shouldn't. The change is a fix, but we need to determine whether the new value is appropriate based on the application's behavior and requirements.

Commit 9003bf08351bd3db070ba4aa6b15c5982901d980

Key Changes:

The code now checks for 200 status code in several places where it was previously checking for 204
There are now print statements to indicate the status code received from the API

Potential problems:

There don't seem to be any issues with the changes itself
However, the number of print statements that are used imply that they are being used for debugging purposes. It is important to remove these print statements or ensure that they are only printed if the code is run in a development environment
Additionally, the error message in lines 347 and 396 do not provide any information beyond the status code received. It should be updated to provide additional context that will help in debugging in the future.

Commit 6af32f899b2e3f04cb76e04b24bd0f7842e8a4a5

The patch fixes the return value of an actor's invoke method in the Echo example. The method now returns a properly formatted HTTP response. The previous implementation returned an empty response body which could have caused issues while testing or using the API.

A potential problem with the patch could be that it is only applicable to the Echo example and may not be a generic solution. The reviewer should check if similar changes are needed in other actors or if the fix should be implemented at a higher level to avoid code duplication.

Commit f36f93ac1d28810e83d29ca00bddcf4ac3a5185d

Key Changes:

Change error messages for clarity.
Remove commented out code.
Change the HTTP status code check for the actor_state and get_actor_state_key functions.

Potential Problems:

None of the changes seem problematic.
However, there might be other issues in the code that are not addressed in this pull request.

Commit cac3a2d7b8013b4f84917c7ac35690744293f59c

Key changes:

Fixes print statements in main.rs
Removes response parsing for state transaction
Changes the return type of the actor_state function from Result<Value, Error> to Result<(), Error>

Potential problems:

It is unclear why the response parsing for state transaction was removed. It may have been done intentionally or as a mistake, but it would be good to clarify this in the pull request.
There may be other parts of the codebase that depend on the previous return type of actor_state. The author should ensure that there are no such dependencies and update the affected code if necessary.

Commit b599454393722ced89bcaa892ae0a1e46fc9df6f

The patch updates the key-value pair for the actor state in the main.rs file. Specifically, it changes the key from "key1" to "location" and adds a corresponding value of "Earth". This change ensures consistency between the inserted key and value.

There do not appear to be any potential problems with this patch since it is a simple correction of a key-value pair.

Commit 1f5cb37869a223b007cf1188aad0e88efe518d9d

The patch changes the return type of a function in src/lib.rs from Result<Value, Error> to Result<(), Error>. The change is in the actor_create_reminder function. Additionally, it updates the response message printed in examples/tests/src/main.rs. There does not seem to be any problem with this specific patch.

Commit 68ed2183e68bfc0926c4a2c2b706d7e8ce551180

The patch addresses two methods, actor_create_timer and actor_delete_timer. The return type of actor_create_timer has been changed to Result<(), Error> from Result<Value, Error>. Additionally, the messages printed at successful completion of actor_delete_reminder, actor_create_timer, and actor_delete_timer have been updated.

The changes seem straightforward and non-problematic. However, it's recommended to check if the test cases have been updated to match the changes in the API. Also, there is no context given about the project as a whole, so it's difficult to determine the significance of the patch in the overall scope of the project.

Hello, I am a code review bot on flows.network. Here are my reviews of changed source code files in this PR.

examples/echo/src/main.rs

Handling of DELETE and PUT requests with fixed paths:

In the current route handlers, there are specific paths defined for handling DELETE and PUT requests related to "stormtrooper" actors, such as /actors/stormtrooper/50 and /actors/stormtrooper/50/method/performAction. This is not a scalable approach, and if you have a different stormtrooper ID, you won't be able to use these routes.

Instead, you could parse path parameters and handle the routing based on the parsed parameters. For example:

(&Method::DELETE, path) if path.starts_with("/actors/stormtrooper/") => {
    let id: u32 = path.strip_prefix("/actors/stormtrooper/").unwrap().parse().unwrap();
    println!("Delete req for stormtrooper/{}", id);
    Ok(Response::default())
},

(&Method::PUT, path) if path.starts_with("/actors/stormtrooper/") => {
    let parts: Vec<_> = path.split('/').collect();
    let id: u32 = parts[2].parse().unwrap();
    if parts.len() == 5 && parts[3] == "method" {
        match parts[4] {
            "performAction" => {
                println!("Perform Action method invoked");
                Ok(Response::new(Body::from(
                    json!({
                        "Storm": "Trooper"
                      }).to_string()
                )))
            },
            // Add other individual methods here
            _ => {
                let mut not_found = Response::default();
                *not_found.status_mut() = StatusCode::NOT_FOUND;
                Ok(not_found)
            }
        }
    } else {
        let mut not_found = Response::default();
        *not_found.status_mut() = StatusCode::NOT_FOUND;
        Ok(not_found)
    }
},

Error handling while converting request body to string:

In various POST request handlers (e.g., for routes /A and /B), the request body is being converted directly to a string with String::from_utf8().unwrap(). If by any chance, the request body contains invalid UTF-8 data, the application will panic due to the call to unwrap().

Instead of panicking, you should handle this case gracefully by using a match statement or ? operator to return an error response to the client. For example:

(&Method::POST, "/A") => {
    match String::from_utf8(hyper::body::to_bytes(req.into_body()).await?.to_vec()) {
        Ok(data) => {
            println!("Received from A {}", data);
            Ok(Response::default())
        },
        Err(_) => {
            let mut bad_request = Response::default();
            *bad_request.status_mut() = StatusCode::BAD_REQUEST;
            Ok(bad_request)
        }
    }
}

Use of to_vec() in multiple places (hyper::body::to_bytes(req.into_body()).await?.to_vec()):

Instead of converting the request body bytes to a Vec in each handler, you can create a helper function to simplify this part of the code. Here's an example:

async fn get_request_body_utf8(req: Request<Body>) -> Result<String, hyper::Error> {
    let bytes = hyper::body::to_bytes(req.into_body()).await?;
    String::from_utf8(bytes.to_vec()).map_err(|_| hyper::Error::Status)
}

Now, you can use this helper function in your route handlers:

(&Method::POST, "/A") => {
    match get_request_body_utf8(req).await {
        Ok(data) => {
            println!("Received from A {}", data);
            Ok(Response::default())
        },
        Err(_) => {
            let mut bad_request = Response::default();
            *bad_request.status_mut() = StatusCode::BAD_REQUEST;
            Ok(bad_request)
        }
    }
}

Handle the Result correctly in the handle_request function signature:

Change the handle_request function signature to:

async fn handle_request(req: Request<Body>) -> Result<Response<Body>, hyper::Error> {

And then, wrap the result of the handle_request(req) in the main() function in a match statement:

let make_svc = make_service_fn(|_| async move {
    Ok::<_, Infallible>(service_fn(move |req| async move {
        match handle_request(req).await {
            Ok(res) => res,
            Err(_) => {
                let mut bad_request = Response::default();
                *bad_request.status_mut() = StatusCode::BAD_REQUEST;
                bad_request
            }
        }
    }))
});

The patch introduces new route handlers for the Actors API for stormtrooper type actors:

A GET request to /dapr/config: This route returns a JSON object containing stormtrooper actor configuration properties, such as actorIdleTimeout, actorScanInterval, drainOngoingCallTimeout, drainRebalancedActors, and reentrancy. It also includes a nested entitiesConfig object for more specific configurations.
A DELETE request to /actors/stormtrooper/50: This route simulates the deletion of a stormtrooper actor with an ID of 50 by printing a message ("Delete req for stormtrooper/50") and returning an empty response.
A PUT request to /actors/stormtrooper/50/method/performAction: This route simulates the invocation of the performAction method on the stormtrooper actor with an ID of 50. It prints a message ("Perform Action method invoked") and returns a JSON response containing {"Storm": "Trooper"}.
A PUT request to /actors/stormtrooper/50/method/remind/checkRebels: This route simulates the invocation of a reminder check for rebels on the stormtrooper actor with an ID of 50. It prints a message ("Reminder check rebels invoked") and returns an empty response.
A PUT request to /actors/stormtrooper/50/method/timer/checkRebels: This route simulates the invocation of a timer check for rebels on the stormtrooper actor with an ID of 50. It prints a message ("Timer check rebels invoked") and returns an empty response.

examples/tests/Cargo.toml

I cannot review the code based on this information, as it only provides the Cargo.toml configuration file for a Rust project. This file contains package metadata, dependencies, and specific features. Please provide the actual source code (i.e., the .rs files) to review.

The key change in this patch is the update of the dapr_sdk_wasi dependency:

Before: The dependency was pointing to the "main" branch of the repository.

dapr_sdk_wasi = {  git = "https://github.com/SAtacker/dapr-sdk-wasi", version = "0.0.1" , branch="main" }

After: The dependency is now pointing to the "actors-api" branch of the repository.

dapr_sdk_wasi = {  git = "https://github.com/SAtacker/dapr-sdk-wasi", version = "0.0.1" , branch="actors-api" }

This change indicates that the project now uses the "actors-api" branch from the "dapr-sdk-wasi" repository, which potentially includes new features or changes specific to the "Actors API".

examples/tests/src/main.rs

The code seems to follow the correct structure and appears to be written properly. A few things could be improved to increase code readability and maintainability. Below I highlight some points that might be addressed:

There are several hardcoded strings throughout the code. It would be better to define them as constants at the beginning of the file, so it becomes easier to modify them in the future if necessary. For example:

const ECHO_SERVICE: &str = "echo-service";
const STARWARS: &str = "starwars";
const STORMTROOPER: &str = "stormtrooper";
...

The bulk of the code is inside the main function, which makes it hard to read and maintain. It would be better to split the code into separate functions, each handling one part of the logic, like invoking service echo, saving state, publishing messages, etc. This way, the code would be easier to understand and maintain.
Formatting can be improved, especially for JSON data defining key-value pairs. Instead of writing the JSON data in a single JSON! macro, it would be better to create a struct that can be serialized and deserialized.
There's a commented block of code. If it is no longer required, I suggest removing it for cleaner code:

// let client = dapr::Dapr::new(3503);

Though it's not part of the provided source code, consider adding error handling for the individual requests as well. In case any request fails, the code will return an error, and it could be helpful to see which request failed and why.

Overall, the code is functional but can benefit from some refactoring and structure improvements.

This patch introduces code to interact with the "stormtrooper" actors using the Dapr client. Key changes include:

Invoking an actor method with actor_invoke_method and printing its response:

let val = client.actor_invoke_method("stormtrooper", "50", "performAction").await?;
println!("Actor invoke method response: {}", val);

Performing a state transaction on the "stormtrooper" actor with actor_state and printing the result:
```
client.actor_state(...).await?;
println!("Actor state transaction is performed");
```

Fetching the state for a specific key ("location") with actor_state_key and printing the response:

let val = client.actor_state_key("stormtrooper", "50", "location").await?;
println!("Actor state key response: {}", val);

Creating a reminder for the "stormtrooper" actor with actor_create_reminder and printing the result:
```
client.actor_create_reminder(...).await?;
println!("Actor create reminder successful");
```

Fetching the reminder for the "stormtrooper" actor with actor_get_reminder and printing the response:

let val = client.actor_get_reminder("stormtrooper", "50", "checkRebels").await?;
println!("Actor get reminder response: {}", val);

Deleting the reminder for the "stormtrooper" actor with actor_delete_reminder and printing the result:

client.actor_delete_reminder("stormtrooper", "50", "checkRebels").await?;
println!("Actor delete reminder successful");

Creating a timer for the "stormtrooper" actor with actor_create_timer and printing the result:
```
client.actor_create_timer(...).await?;
println!("Actor create timer successful");
```

Deleting the timer for the "stormtrooper" actor with actor_delete_timer and printing the result:

client.actor_delete_timer("stormtrooper", "50", "checkRebels").await?;
println!("Actor delete timer successful");

These changes extend the code to work with actor features and give a more comprehensive example of using the Actors API.

src/lib.rs

The code looks mostly fine, but there are a few places that can potentially be improved:

Error handling: There are numerous calls to reqwest methods which return Result. There is proper error handling using ? for early returns, but errors could be more descriptive by adding context about the failing request. You can do this by chaining the errors using map_err:

let json = client
    .post(&url)
    .json(&kvs)
    .send()
    .await
    .map_err(|e| anyhow!("Failed to send request: {}. URL: {}", e, url))?
    .json()
    .await
    .map_err(|e| anyhow!("Failed to parse JSON: {}. URL: {}", e, url))?;

Logging: The code uses multiple println! statements for printing debug information. It's recommended to use the log crate or a similar logging library, as it allows you to control the log level and output more easily. For instance, replace println! with the corresponding log macro, e.g.:

log::info!("URL is {}", url);

URL construction: For better readability and maintainability, you can use format! macro to construct the URL strings. This may help reduce the risk of errors when adding or modifying routes. For example:

let url = format!("{}invoke/{}/method/{}", self.url_base, app_id, method_name);

Reusing the reqwest::Client: The code currently creates a new instance of reqwest::Client for each request. It's more efficient to reuse the client because it can take advantage of connection pooling. You can create a single client in the Dapr struct and then use it throughout the methods. For example:

pub struct Dapr {
    pub url_base: String,
    client: reqwest::Client,
}

impl Dapr {
    pub fn new_with_url(url_base_: String) -> Dapr {
        Dapr {
            url_base: url_base_.to_string() + "/v1.0/",
            client: reqwest::Client::new(),
        }
    }
}

Then update the method calls to use self.client instead of creating a new instance:

let json = self
    .client
    .post(&url)
    .json(&kvs)
    .send()
    .await?
    .json()
    .await?;

These are only a few possible improvements, but the code seems to be functional and correct in terms of its Dapr API usage.

The patch adds several new methods to the Dapr struct for handling actors in Dapr. Here are the key changes:

actor_invoke_method: This method is for invoking a method on a specific actor. It takes actor type, actor ID, and method name as parameters, constructs the URL, and sends a PUT request. If the status code is 200, it returns the JSON response, otherwise, it returns an error.
actor_state: This method saves the state of an actor to the Dapr runtime. It takes actor type, actor ID, and a JSON value as parameters. It constructs the URL and sends a POST request with the JSON value in the request body. If the status code is 204, it returns successfully, otherwise, it returns an error.
actor_state_key: This method fetches the state of a specific key for an actor. It takes actor type, actor ID, and the key as parameters. It constructs the URL, sends a GET request, and if the status code is 200, it returns the JSON response. Otherwise, it returns an error.
actor_create_reminder, actor_get_reminder, and actor_delete_reminder: These methods are for creating, fetching, and deleting reminders for an actor. They take the actor type, actor ID, and reminder name as parameters. For creating and deleting reminders, it sends a POST and DELETE request, respectively, and verifies the status code is 204. In the case of fetching the reminder, it sends a GET request and checks for a 200 status code.
actor_create_timer and actor_delete_timer: These methods are for creating and deleting timers for an actor. They take the actor type, actor ID, and timer name as parameters. Similar to the reminder methods, they send POST and DELETE requests, respectively, and check for a 204 status code.

All these methods use reqwest::Client for making API requests and validate the status code before returning the appropriate result or error.

second-state / dapr-sdk-wasmedge

Actors API #6

Details

Commit 9461a212761569023e07fcbfc4e23d9b24500e4c

Commit 79cb9df8a8def8ae82fe40f020aedd99161e8163

Commit f3884a7c725608f0121d6f20d6a9f5a42c366466

Commit 26a503ca548aebdb7893cf682d0c2adcde43dcc9

Commit 936aa581c4ca437cf4d67bc3194b0e286cb18c5f

Commit 8d7887185b62e670710b5cb4cde22f7c31ea4e32

Commit 9003bf08351bd3db070ba4aa6b15c5982901d980

Commit 6af32f899b2e3f04cb76e04b24bd0f7842e8a4a5

Commit f36f93ac1d28810e83d29ca00bddcf4ac3a5185d

Commit cac3a2d7b8013b4f84917c7ac35690744293f59c

Commit b599454393722ced89bcaa892ae0a1e46fc9df6f

Commit 1f5cb37869a223b007cf1188aad0e88efe518d9d

Commit 68ed2183e68bfc0926c4a2c2b706d7e8ce551180

examples/echo/src/main.rs

examples/tests/Cargo.toml

examples/tests/src/main.rs

src/lib.rs