fermyon / spin-trigger-cron

Apache License 2.0
7 stars 8 forks source link

Error conversion definitions needed #16

Closed tpmccallum closed 6 months ago

tpmccallum commented 6 months ago

When using:

The compiler generates errors and asks for error implementations.

For example,

Request/Response:

let response: Response = spin_sdk::http::send(request).await?;
   |                                                                 ^ the trait `From<spin_sdk::http::SendError>` is not implemented for `spin_cron_sdk::Error`, which is required by `Result<(), spin_cron_sdk::Error>: FromResidual<Result<Infallible, spin_sdk::http::SendError>>``

Store:

let store = Store::open_default()?;
   |                 ---------------------^ the trait `From<spin_sdk::key_value::Error>` is not implemented for `spin_cron_sdk::Error`, which is required by `Result<(), spin_cron_sdk::Error>: FromResidual<Result<Infallible, spin_sdk::key_value::Error>>`
store.set("uvi", response_as_json_string["coord"]["lat"].to_string().as_bytes())?;
   |           --------------------------------------------------------------------------^ the trait `From<spin_sdk::key_value::Error>` is not implemented for `spin_cron_sdk::Error`, which is required by `Result<(), spin_cron_sdk::Error>: FromResidual<Result<Infallible, spin_sdk::key_value::Error>>`

Serde:

let response_as_json_string: Value = serde_json::from_slice(&response.body())?;
   |                                          ^^^^^^^^^^ use of undeclared crate or module `serde_json`

error[E0277]: `?` couldn't convert the error to `spin_cron_sdk::Error`

We were able to get around these messages. @rajatjindal re-wrote the original code (that the http-rust and cron-rust templates generated by default), although the following code is only seen as a workaround on the user's end for now. We think that the error conversions could be implemented inside the spin-trigger-cron itself. Happy for any feedback and questions. Please see the before and after code re: implementing the workaround:

Before

use serde_json::Value;
use spin_cron_sdk::{cron_component, Error, Metadata};
use spin_sdk::{
    http::{IncomingResponse, Method, Request},
    key_value::Store,
};

#[cron_component]
async fn handle_cron_event(metadata: Metadata) -> Result<(), Error> {
    println!("Handling request to {:?}", req.header("spin-full-url"));
    let request = Request::builder()
    .method(Method::Get)
    .uri("https://api.openweathermap.org/data/2.5/weather?lat=44.34&lon=10.99&appid=my-api-key")
    .build();
    let response: Response = spin_sdk::http::send(request).await?;
    let response_as_json_string: Value = serde_json::from_slice(&response.body())?;
    let store = Store::open_default()?;
    store.set("Latitude", response_as_json_string["coord"]["lat"].to_string().as_bytes())?;
    Ok(())
}

After

use serde_json::Value;
use spin_cron_sdk::{cron_component, Error, Metadata};
use spin_sdk::{
    http::{IncomingResponse, Method, Request},
    key_value::Store,
};

#[cron_component]
async fn handle_cron_event(metadata: Metadata) -> Result<(), Error> {
    println!("[{}] Hello from a cron component", metadata.timestamp);

    let request = Request::builder()
        .method(Method::Get)
        .uri("https://api.openweathermap.org/data/2.5/weather?lat=44.34&lon=10.99&appid=my-api-key")
        .build();

    let response = spin_sdk::http::send::<_, IncomingResponse>(request)
        .await
        .expect("making http request");

    let body = &response.into_body().await.expect("some message");

    let response_as_json_string: Value =
        serde_json::from_slice(body).expect("response body parsing");
    println!("{:?}", response_as_json_string);

    let store = Store::open_default().expect("open kv store");
    store
        .set(
            &metadata.timestamp.to_string(),
            response_as_json_string["coord"]["lat"]
                .to_string()
                .as_bytes(),
        )
        .expect("storing value in kv store");
    println!("Stored UVI of {:?} at timestamp: [{}]", response_as_json_string["coord"]["lat"].to_string(), metadata.timestamp);

    Ok(())
}
itowlson commented 6 months ago

While it's reasonable for the cron SDK to handle conversion of Spin-specific errors, I don't think the cron SDK should take a dependency on serde_json just in order to handle JSON errors.

Semi-related: it would be good to come up with some guidance on when a cron component should handle errors internally (e.g. by logging) and when it should return them for the trigger to process. (Cf. the WASI HTTP interface which doesn't allow returning an error at all, but expects the handler to do all its error handling internally.)

Also semi-related: given that the cron SDK Error has extremely limited semantics, I wonder if we are better guiding people to write the logic with a more forgiving error type and log or convert at top level e.g.

#[cron_component]
async fn handle_cron_event(metadata: Metadata) -> Result<(), Error> {
    really_handle_cron_event(metadata).await.map_err(...)
}

async fn really_handle_cron_event(metadata: Metadata) -> anyhow::Result<()> {
    // use the `?` operator as profligately as you like because anyhow can take it, rawr
}

This is the sort of thing I tend to end up with for the aforementioned WASI (e.g. streaming) HTTP cases.

tpmccallum commented 6 months ago

That really simplifies things a lot. Thanks @itowlson So now, I can perform that same task by just adding a couple of tweaks. For example:

cargo add anyhow
cargo add serde_json

Update the function's return type from the original -> Result<(), Error> to the newer anyhow approach -> anyhow::Result<()>

For example:

use serde_json::Value;
use spin_cron_sdk::{cron_component, Metadata};
use spin_sdk::{
    http::{Method, Request, Response},
    key_value::Store,
};

#[cron_component]
async fn handle_cron_event(metadata: Metadata) -> anyhow::Result<()> {
    let request = Request::builder()
    .method(Method::Get)
    .uri("https://api.openweathermap.org/data/2.5/weather?lat=44.34&lon=10.99&appid=my-api-key")
    .build();
    let response: Response = spin_sdk::http::send(request).await?;
    let response_as_json_string: Value = serde_json::from_slice(&response.body())?;
    let store = Store::open_default()?;
    store.set(&metadata.timestamp.to_string(), response_as_json_string["current"]["uvi"].to_string().as_bytes())?;
    println!("Stored UVI of {:?} at timestamp: [{}]", response_as_json_string["current"]["uvi"].to_string(), metadata.timestamp);
    Ok(())
}
$ spin build
    Building component hello-cron with `cargo build --target wasm32-wasi --release`
    Compiling hello-cron v0.1.0 (/Users/tpmccallum/Fermyon/cron-app/hello_cron)
    Finished `release` profile [optimized] target(s) in 0.55s
    Finished building all Spin components
$ spin up
    Logging component stdio to ".spin/logs/"
    Storing default key-value data to ".spin/sqlite_key_value.db"
    Stored UVI of "0.20" at timestamp: [1715826289]
itowlson commented 6 months ago

Oh! I actually thought you would need an ancillary function to return the anyhow::Result and then map_err it in the top-level function. But you're right, the cron_component macro lets you use an anyhow::Result directly, and that is waaaay better! Great find @tpmccallum

karthik2804 commented 6 months ago

We should probably update the template to use anyhow or is that a dependency the user can bring based on their requirements?