Skip to content
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

Implement get_all function to return non-folding set-cookie headers #597

Merged
merged 5 commits into from
Sep 2, 2024

Conversation

nakamura-shuta
Copy link
Contributor

@nakamura-shuta nakamura-shuta commented Jul 18, 2024

issue : #423

Implement get_all method for Headers using wasm-bindgen

This PR implements the get_all method for Headers with the following approach:

  1. Define the get_all method in worker_sys::ext::headers::Headers using wasm-bindgen
  2. Create a HeadersExt trait in worker_sys to extend web_sys::Headers functionality
  3. Implement get_all in worker::Headers using the worker_sys implementation

Key changes:

  • Add a new file worker-sys/src/ext/headers.rs to define Headers type and get_all method
  • Create HeadersExt trait to extend web_sys::Headers with get_all method
  • Update worker/src/headers.rs to use the new HeadersExt trait
  • Modify the get_all implementation in worker::Headers to use the new wasm-bindgen based method

Specification of the get_all method:

  • Retrieves all values of a given header name
  • Returns the result as Result<Vec>
  • Uses wasm-bindgen to directly call the JavaScript Headers.getAll() method

This implementation allows for retrieving multiple values for any header, not just 'Set-Cookie',
providing more flexibility and aligning with the Web API's Headers.getAll() method.

@kflansburg
Copy link
Member

Why limit only to set-cookie header?

@nakamura-shuta
Copy link
Contributor Author

The workerd code I used as reference for implementation made an error when a header other than set-cookie was specified, so I implemented it as is.

https://github.com/cloudflare/workerd/blob/a265d5c22bf2a6bc6394aa54db517ff86d6c788c/src/workerd/api/http.c%2B%2B#L229-L242

@kflansburg
Copy link
Member

I see, I can ask internally about this.

If this is already implemented in the runtime, would it make sense to just add the getAll method to the wasm_bindgen Header extension type here https://github.com/cloudflare/workers-rs/blob/main/worker-sys/src/ext/headers.rs?

@nakamura-shuta
Copy link
Contributor Author

nakamura-shuta commented Jul 22, 2024

@kflansburg

Yes, I think that is the simplest way.
I implemented it that way. (Like other functions, such as “entries”.)

If you have any suggestions for improving this implementation,
I would greatly appreciate your feedback.
Thank you.

@kflansburg
Copy link
Member

Can we invoke the JavaScript getAll method via wasm-bindgen, rather than re-implementing it in Rust?

Copy link
Member

@kflansburg kflansburg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Were you able to test this?

@@ -0,0 +1,20 @@
use wasm_bindgen::prelude::*;

#[wasm_bindgen]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be buried in a glue module like some of the other extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have included it in the glue module to match the others. Included in the glue module along with others

- Update HeadersExt trait to return js_sys::Array directly
- Modify Headers::get_all to handle potential invalid values
- Adjust import order in ext.rs for consistency
@nakamura-shuta
Copy link
Contributor Author

nakamura-shuta commented Sep 2, 2024

@kflansburg

This looks great! Were you able to test this?

I created the following application under the example directory and verified its operation with curl.
(Not git committed)

/// create sample application. examples/cookie/src/lib.rs
use worker::*;

// Main entry point for the worker
#[event(fetch)]
pub async fn main(req: Request, env: Env, _ctx: worker::Context) -> Result<Response> {
    // Set up a router and define a route for "/test_cookies"
    Router::with_data(())
        .get_async("/test_cookies", test_cookies)
        .run(req, env)
        .await
}

// Handler function for the "/test_cookies" route
async fn test_cookies(req: Request, _ctx: RouteContext<()>) -> Result<Response> {
    // Retrieve all Set-Cookie headers from the input request
    let input_cookies = req.headers().get_all("Set-Cookie")?;

    // Create a new response
    let mut res = Response::ok("Cookies processed")?;

    // Add the input Set-Cookie headers to the new response
    for cookie in &input_cookies {
        // Append each Set-Cookie header to the response
        res.headers_mut().append("Set-Cookie", cookie)?;
    }

    // Log the request and response headers for debugging
    console_log!("Request Set-Cookie headers: {:?}", input_cookies);
    console_log!("Response headers: {:?}", res.headers());

    // Return the response
    Ok(res)
}
# curl command in console

% curl 'http://localhost:8787/test_cookies' \
  -H 'Set-Cookie: a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT' \
  -H 'Set-Cookie: b=2; Expires=Wed, 21 Oct 2015 07:28:00 GMT' \
  -v
* Host localhost:8787 was resolved.
> GET /test_cookies HTTP/1.1
> ・・・・・・・
> Set-Cookie: a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT
> Set-Cookie: b=2; Expires=Wed, 21 Oct 2015 07:28:00 GMT
>
< HTTP/1.1 200 OK
< Content-Length: 17
< Content-Type: text/plain; charset=utf-8
< Set-Cookie: a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT
< Set-Cookie: b=2; Expires=Wed, 21 Oct 2015 07:28:00 GMT
<
* Connection #0 to host localhost left intact
Cookies processed
# wrangler dev console

% cd workers-rs/examples/cookie
% npx wrangler dev

[wrangler:inf] Ready on http://localhost:8787
Request Set-Cookie headers: ["a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT", "b=2; Expires=Wed, 21 Oct 2015 07:28:00 GMT"]
Response headers: Headers {
content-type = text/plain; charset=utf-8
set-cookie = a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT
set-cookie = b=2; Expires=Wed, 21 Oct 2015 07:28:00 GMT
}
[wrangler:inf] GET /test_cookies 200 OK (12ms)

Copy link
Member

@kflansburg kflansburg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kflansburg kflansburg merged commit 5100f4e into cloudflare:main Sep 2, 2024
3 checks passed
thibmeu pushed a commit to thibmeu/workers-rs that referenced this pull request Sep 2, 2024
…loudflare#597)

* Add `get_all` function. (cloudflare#423)

* Implement get_all using wasm-bindgen

* Refactor Headers get_all method and improve error handling

- Update HeadersExt trait to return js_sys::Array directly
- Modify Headers::get_all to handle potential invalid values
- Adjust import order in ext.rs for consistency

* Included in glue module along with other modules
renovate bot added a commit to spiraldb/vortex that referenced this pull request Sep 13, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [worker](https://redirect.github.com/cloudflare/workers-rs) |
workspace.dependencies | minor | `0.3.0` -> `0.4.0` |

---

### Release Notes

<details>
<summary>cloudflare/workers-rs (worker)</summary>

###
[`v0.4.0`](https://redirect.github.com/cloudflare/workers-rs/releases/tag/v0.4.0)

[Compare
Source](https://redirect.github.com/cloudflare/workers-rs/compare/v0.3.4...v0.4.0)

#### What's Changed

- Relax type of callback arguments to `Router` methods. This uses APIT
to allow more types than a function pointer to implement handlers (i.e.
async closure) by
[@&#8203;compiler-errors](https://redirect.github.com/compiler-errors)
in
[cloudflare/workers-rs#605
- Fix typos in CORS argument names by
[@&#8203;OliverEvans96](https://redirect.github.com/OliverEvans96) in
[cloudflare/workers-rs#629
- Implement `get_all` function to return non-folding set-cookie headers
by [@&#8203;nakamura-shuta](https://redirect.github.com/nakamura-shuta)
in
[cloudflare/workers-rs#597
- Add `FormData` conversion into `JsValue` by
[@&#8203;thibmeu](https://redirect.github.com/thibmeu) in
[cloudflare/workers-rs#634

> \[!CAUTION]
> Breaking: Make R2 `Object::size` return `u64` by
[@&#8203;lkolbly](https://redirect.github.com/lkolbly) in
[cloudflare/workers-rs#625

#### New Contributors

- [@&#8203;lkolbly](https://redirect.github.com/lkolbly) made their
first contribution in
[cloudflare/workers-rs#625
- [@&#8203;compiler-errors](https://redirect.github.com/compiler-errors)
made their first contribution in
[cloudflare/workers-rs#605
- [@&#8203;OliverEvans96](https://redirect.github.com/OliverEvans96)
made their first contribution in
[cloudflare/workers-rs#629
- [@&#8203;nakamura-shuta](https://redirect.github.com/nakamura-shuta)
made their first contribution in
[cloudflare/workers-rs#597
- [@&#8203;thibmeu](https://redirect.github.com/thibmeu) made their
first contribution in
[cloudflare/workers-rs#634

**Full Changelog**:
cloudflare/workers-rs@v0.3.4...v0.4.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/spiraldb/vortex).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguNzQuMSIsInRhcmdldEJyYW5jaCI6ImRldmVsb3AiLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants