feat: Implementation of redirect option for fetch()

[nabetti1720]

Issue # (if available)

340

Description of changes

• Minimal improvements have been made to allow for redirect processing.(reference information)

Checklist

• [x] Created unit tests in tests/unit and/or in Rust for my feature if needed
• [x] Ran make fix to format JS and apply Clippy auto fixes
• [x] Made sure my code didn't add any additional warnings: make check
• [ ] Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[nabetti1720]

We will continue to address the following points:

1. Return network error response.
2. Replace only the url when build_request() is called multiple times.
3. Add test code

[nabetti1720]

We have revised our policy a bit.

1. Return network error response.

Since it has already been pointed out in the review that simply returning an Error is sufficient, we are not going to do this in the immediate future. If necessary, we would like to implement a method to generate and return a Network error response by calling something like Response::error(ctx).

2. Replace only the url when build_request() is called multiple times.

Reading through the HTTP-redirect fetch, we see that we need to control multiple request parameters, not just the url, depending on the method and status code. To control this, we would like to keep the build_request() to generate the next request parameter. After this, commit additional modifications.

3. Add test code

I am wondering if it is possible to do a fetch() redirect test without accessing the actual site. Any good ideas? Note that the minimum test for the pattern necessary to make a decision was implemented in rust.

[richarddavison]

Thanks again for the PR. I think it LGTM. 🚀

We have revised our policy a bit.

1. Return network error response.

Since it has already been pointed out in the review that simply returning an Error is sufficient, we are not going to do this in the immediate future. If necessary, we would like to implement a method to generate and return a Network error response by calling something like Response::error(ctx).

What value would this bring?

2. Replace only the url when build_request() is called multiple times.

Reading through the HTTP-redirect fetch, we see that we need to control multiple request parameters, not just the url, depending on the method and status code. To control this, we would like to keep the build_request() to generate the next request parameter. After this, commit additional modifications.

You can ignore the “cors” logic as it’s mainly relevant to browsers and is not planned to be supported.

3. Add test code

I am wondering if it is possible to do a fetch() redirect test without accessing the actual site. Any good ideas? Note that the minimum test for the pattern necessary to make a decision was implemented in rust.

Yes, runtime_client.rs has tests using wiremock: https://github.com/awslabs/llrt/blob/bfaa41ecd20df9de971aa0358571e30c2a46ae3f/src/runtime_client.rs#L500

[nabetti1720]

Thanks again for the PR. I think it LGTM. 🚀

We have revised our policy a bit.

1. Return network error response.

Since it has already been pointed out in the review that simply returning an Error is sufficient, we are not going to do this in the immediate future. If necessary, we would like to implement a method to generate and return a Network error response by calling something like Response::error(ctx).

What value would this bring?

Yes, I have not found a use case for llrt at this time. So I do not think that implementation is necessary.

2. Replace only the url when build_request() is called multiple times.

Reading through the HTTP-redirect fetch, we see that we need to control multiple request parameters, not just the url, depending on the method and status code. To control this, we would like to keep the build_request() to generate the next request parameter. After this, commit additional modifications.

You can ignore the “cors” logic as it’s mainly relevant to browsers and is not planned to be supported.

Understood. So, would it be safe to assume that we have roughly completed the implementation? Please point out any specifications that are of concern.

3. Add test code

I am wondering if it is possible to do a fetch() redirect test without accessing the actual site. Any good ideas? Note that the minimum test for the pattern necessary to make a decision was implemented in rust.

Yes, runtime_client.rs has tests using wiremock:

https://github.com/awslabs/llrt/blob/bfaa41ecd20df9de971aa0358571e30c2a46ae3f/src/runtime_client.rs#L500

Thank you for sharing your excellent samples! This is the first implementation I've seen, so it may take some time, but I'll get to work on creating a mock server as soon as possible :)

[nabetti1720]

I am creating a test with a mock server as soon as possible, but the fetch() call part is not working properly. I am thinking of using fetch() defined in the globals of vm. Is the procedure correct?

let vm = Vm::new().await.unwrap();

async_with!(vm.ctx => |ctx| {
    async move {
        let result = ctx.globals().fetch(format!("http://{}/expect/200/", mock_server.address()), None).await;
        assert!(result.is_ok());
    }
})
.await;

vm.runtime.idle().await;

error[E0599]: no method named `fetch` found for struct `rquickjs::Object<'_>` in the current scope
   --> src/http/fetch.rs:490:44
    |
490 |                 let result = ctx.globals().fetch(format!("http://{}/expect/200/", mock_server.address()), None).await;
    |                                            ^^^^^ method not found in `Object<'_>`

[richarddavison]

 ctx.globals().fetch(

You would have to access the global object like this:

let fetch = ctx.globals().get("fetch")?;
let res: Promise<Value> = fetch(....)?;
let response = res.await?;

[nabetti1720]

My apologies. I have tried everything, but I cannot resolve the error that the type of fetch is not specified. What is the type of fetch in this case?

let vm = Vm::new().await.unwrap();

async_with!(vm.ctx => |ctx| {
    async move {
        let fetch = ctx.globals().get("fetch")?;
        let res: Promise<Value> = fetch(format!("http://{}/expect/200/", mock_server.address()), None)?;
        let response = res.await?;
        assert!(response.is_ok());
        Ok(())
    }
})
.await;

vm.runtime.idle().await;

error[E0282]: type annotations needed
   --> src/http/fetch.rs:490:21
    |
490 |                 let fetch = ctx.globals().get("fetch")?;
    |                     ^^^^^
491 |                 let res: Promise<Value> = fetch(format!("http://{}/expect/200/", mock_server.address()), None)?;
    |                                           -------------------------------------------------------------------- type must be known at this point
    |
help: consider giving `fetch` an explicit type
    |
490 |                 let fetch: /* Type */ = ctx.globals().get("fetch")?;
    |                          ++++++++++++

[richarddavison]

My apologies. I have tried everything, but I cannot resolve the error that the type of fetch is not specified. What is the type of fetch in this case?

No need to apologize. Fetch is a Function :)

let fetch: Function = ...

And you call it like this:

fetch.call((url, options))?; //send tuple as argument

[nabetti1720]

We are continuing to try things, but there are two issues.

1. When I try to use assert! in `async_with!, I get an error. Therefore, I am wondering if it is possible to bring the response outside. In addition, what should be done to determine when an exception occurs?
2. Is it OK if the type is not specified in response as in fetch? (Issue No. 1 has not been resolved and not confirmed.)

let vm = Vm::new().await.unwrap();

async_with!(vm.ctx => |ctx| {
    async move {
        let fetch: Function = ctx.globals().get("fetch")?;
        let url = format!("http://{}/expect/200/", mock_server.address());
        let options = {};
        let res: Promise<Value> = fetch.call((url, options))?;
        let response = res.await?;
        // The assert macro cannot be used in asynchronous functions.
        // assert!(response.is_ok()); 
        Ok(())
    }
})
.await;

vm.runtime.idle().await;

error[E0282]: type annotations needed
   --> src/http/fetch.rs:497:17
    |
497 |                 Ok(())
    |                 ^^ cannot infer type of the type parameter `E` declared on the enum `Result`
    |
help: consider specifying the generic arguments
    |
497 |                 Ok::<(), E>(())
    |                   +++++++++

[richarddavison]

We are continuing to try things, but there are two issues.

1. When I try to use assert! in `async_with!, I get an error. Therefore, I am wondering if it is possible to bring the response outside. In addition, what should be done to determine when an exception occurs?

Yes, you can return the result from the await right away:

let fetch_result: Result<Value> = async_with!(vm.ctx => |ctx| {
 async move {
   ...
   let res: Promise<Value> = fetch.call((url, options))?;
   res.await.catch(&ctx).unwrap()
  }}).await;

let fetch_response = Class::<Response>::from_value(fetch_result.unwrap()).unwrap();
let response = fetch_response.borrow();
assert!(response.status,200) //etc
...

[nabetti1720]

Yes, you can return the result from the await right away:

I'm sorry. I continue to have trouble getting res out of the async block. I asked ChatGPT and got several suggestions for how to write L494, but they were all no good...

error[E0277]: the `?` operator can only be used in an async block that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> src/http/fetch.rs:490:65
    |
489 | /             async move {
490 | |                 let fetch: Function = ctx.globals().get("fetch")?;
    | |                                                                 ^ cannot use the `?` operator in an async block that returns `rquickjs::Value<'_>`
491 | |                 let url = format!("http://{}/expect/200/", mock_server.address());
492 | |                 let options = {};
493 | |                 let res: Promise<Value> = fetch.call((url, options))?;
494 | |                 res.await.catch(&ctx).unwrap()
495 | |             }}).await;
    | |_____________- this function should return `Result` or `Option` to accept `?`
    |
    = help: the trait `FromResidual<std::result::Result<Infallible, rquickjs::Error>>` is not implemented for `rquickjs::Value<'_>`

error[E0277]: the `?` operator can only be used in an async block that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> src/http/fetch.rs:493:69
    |
489 | /             async move {
490 | |                 let fetch: Function = ctx.globals().get("fetch")?;
491 | |                 let url = format!("http://{}/expect/200/", mock_server.address());
492 | |                 let options = {};
493 | |                 let res: Promise<Value> = fetch.call((url, options))?;
    | |                                                                     ^ cannot use the `?` operator in an async block that returns `rquickjs::Value<'_>`
494 | |                 res.await.catch(&ctx).unwrap()
495 | |             }}).await;
    | |_____________- this function should return `Result` or `Option` to accept `?`
    |
    = help: the trait `FromResidual<std::result::Result<Infallible, rquickjs::Error>>` is not implemented for `rquickjs::Value<'_>`

error[E0308]: mismatched types
   --> src/http/fetch.rs:488:43
    |
488 |            let fetch_result: Result<Value> = async_with!(vm.ctx => |ctx| {
    |  ____________________________________________^
489 | |/             async move {
490 | ||                 let fetch: Function = ctx.globals().get("fetch")?;
491 | ||                 let url = format!("http://{}/expect/200/", mock_server.address());
492 | ||                 let options = {};
493 | ||                 let res: Promise<Value> = fetch.call((url, options))?;
494 | ||                 res.await.catch(&ctx).unwrap()
495 | ||             }}).await;
    | ||_____________-_^ expected `Result<Value<'_>, Error>`, found `async` block
    |  |_____________|
    |                the found `async` block
    |
    = note:       expected enum `std::result::Result<rquickjs::Value<'_>, rquickjs::Error>`
            found `async` block `{async block@src/http/fetch.rs:489:13: 495:14}`
    = note: this error originates in the macro `async_with` (in Nightly builds, run with -Z macro-backtrace for more info)

[richarddavison]

Sorry my misstake, I forgot to remove unwrap. You need to do that if you want to return Result using ? from the async block. Options is also wrong here: {} in rust is a closure, not a JS object. You can pass a Request object or a JS object like this:

  let options = Object::new(ctx).unwrap();
  options.set("redirect","manual").unwrap();
  //etc

let fetch_result: Result<Value> = async_with!(vm.ctx => |ctx| {
    async move {
        let fetch: Function = ctx.globals().get("fetch")?;
        let url = format!("http://{}/expect/200/", mock_server.address());

        let res: Promise<Value> = fetch.call((url, options))?;
        res.await.catch(&ctx)
    }}).await;

[nabetti1720]

Sorry my misstake, I forgot to remove unwrap. You need to do that if you want to return Result using ? from the async block. Options is also wrong here: {} in rust is a closure, not a JS object. You can pass a Request object or a JS object like this:

Thank you very much. However, when creating the initial object for the options, it seems that ctx is not allowed in this position because of the AsyncContext.

I also removed the unwrap() on the last line of the async block, but the error did not change significantly.

The codes and errors at this time are as follows.

let vm = Vm::new().await.unwrap();

let fetch_result: Result<Value> = async_with!(vm.ctx => |ctx| {
    async move {
        let fetch: Function = ctx.globals().get("fetch")?;
        let options = Object::new(ctx).unwrap();
        options.set("redirect","manual").unwrap();

        let url = format!("http://{}/expect/200/", mock_server.address());

        let res: Promise<Value> = fetch.call((url, options))?;
        res.await.catch(&ctx)
    }}).await;

let fetch_response = Class::<Response>::from_value(fetch_result.unwrap()).unwrap();
let response = fetch_response.borrow();
assert!(response.status() == 200);

vm.runtime.idle().await;

error[E0277]: `?` couldn't convert the error to `CaughtError<'_>`
   --> src/http/fetch.rs:490:65
    |
490 |                 let fetch: Function = ctx.globals().get("fetch")?;
    |                                                                 ^ the trait `std::convert::From<rquickjs::Error>` is not implemented for `CaughtError<'_>`, which is required by `std::result::Result<rquickjs::Value<'_>, CaughtError<'_>>: FromResidual<std::result::Result<Infallible, rquickjs::Error>>`
    |
    = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
    = help: the following other types implement trait `FromResidual<R>`:
              <std::result::Result<T, F> as FromResidual<Yeet<E>>>
              <std::result::Result<T, F> as FromResidual<std::result::Result<Infallible, E>>>
    = note: required for `std::result::Result<rquickjs::Value<'_>, CaughtError<'_>>` to implement `FromResidual<std::result::Result<Infallible, rquickjs::Error>>`

error[E0277]: `?` couldn't convert the error to `CaughtError<'_>`
   --> src/http/fetch.rs:496:69
    |
496 |                 let res: Promise<Value> = fetch.call((url, options))?;
    |                                                                     ^ the trait `std::convert::From<rquickjs::Error>` is not implemented for `CaughtError<'_>`, which is required by `std::result::Result<rquickjs::Value<'_>, CaughtError<'_>>: FromResidual<std::result::Result<Infallible, rquickjs::Error>>`
    |
    = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
    = help: the following other types implement trait `FromResidual<R>`:
              <std::result::Result<T, F> as FromResidual<Yeet<E>>>
              <std::result::Result<T, F> as FromResidual<std::result::Result<Infallible, E>>>
    = note: required for `std::result::Result<rquickjs::Value<'_>, CaughtError<'_>>` to implement `FromResidual<std::result::Result<Infallible, rquickjs::Error>>`

error[E0308]: mismatched types
   --> src/http/fetch.rs:488:43
    |
488 |            let fetch_result: Result<Value> = async_with!(vm.ctx => |ctx| {
    |  ____________________________________________^
489 | |/             async move {
490 | ||                 let fetch: Function = ctx.globals().get("fetch")?;
491 | ||                 let options = Object::new(ctx).unwrap();
492 | ||                 options.set("redirect","manual").unwrap();
...   ||
497 | ||                 res.await.catch(&ctx)
498 | ||             }}).await;
    | ||_____________-_^ expected `Result<Value<'_>, Error>`, found `async` block
    |  |_____________|
    |                the found `async` block
    |
    = note:       expected enum `std::result::Result<rquickjs::Value<'_>, rquickjs::Error>`
            found `async` block `{async block@src/http/fetch.rs:489:13: 498:14}`
    = note: this error originates in the macro `async_with` (in Nightly builds, run with -Z macro-backtrace for more info)

[nabetti1720]

The following lines are also available in ? -> .unwrap(), the error that occurred in this line disappeared.

        ...
        let fetch: Function = ctx.globals().get("fetch")?;
        ...
        let res: Promise<Value> = fetch.call((url, options))?;
        ...

I cannot resolve the L497 error. Regardless of the contents of L497, the same error always occurs.

The error message is,

expected `Result<Value<'_>, Error>`, found `async` block

so it looks like I need to implement something that returns Result<Value<'_>, Error> after async block.

[richarddavison]

No more pseudo code! I've tested this and it compiles. We make all the assertions inside the async block and catch the exceptions inside the async_with! macro as they can't be returned outside of it.

async_with!(vm.ctx => |ctx| {
    let globals = ctx.globals();
    let run = async {
        let options = Object::new(ctx.clone())?;
        options.set("redirect", "manual")?;

        let fetch: Function = globals.get("fetch")?;
        let url = format!("http://{}/expect/200/", "url");
        let response_promise: Promise<Value> = fetch.call((url,options))?;
        let response = response_promise.await?;
        let response = Class::<Response>::from_value(response)?;
        let response = response.borrow();

        //do more tests and assertions in here
        assert_eq!(response.status(),200);

        Ok(())
    };
    run.await.catch(&ctx).unwrap();
})
.await;

[nabetti1720]

Hi @richarddavison, Thank you for your dedication and support! Finally, the cargo test started to pass when accessing the mock server I created.