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

Support .cookie(...) for customize() #3214

Open
kukuro-kt opened this issue Dec 4, 2023 · 7 comments
Open

Support .cookie(...) for customize() #3214

kukuro-kt opened this issue Dec 4, 2023 · 7 comments
Labels
A-web project: actix-web C-feature Category: new functionality good-first-issue easy to pick up for newcomers

Comments

@kukuro-kt
Copy link

As #3213 wrote, something like customize().cookie(...).

I'm not sure if I need to write more details?

@robjtede robjtede added good-first-issue easy to pick up for newcomers C-feature Category: new functionality A-web project: actix-web labels Dec 4, 2023
@Thomblin
Copy link

Thomblin commented Dec 4, 2023

I want to give this a try. See #3215 for my first draft

@kukuro-kt
Copy link
Author

One more thing, why cookie needs to be converted to HeaderValue first, while append_header(...) can insert String directly?

Current implement of cookie(...):

    pub fn cookie(&mut self, cookie: cookie::Cookie<'_>) -> &mut Self {
        match cookie.to_string().try_into_value() {
            Ok(hdr_val) => self.append_header((header::SET_COOKIE, hdr_val)),
            Err(err) => {
                self.error = Some(err.into());
                self
            }
        }
    }

My thought:

    pub fn cookie(&mut self, cookie: cookie::Cookie<'_>) -> &mut Self {
        self.append_header((header::SET_COOKIE, cookie.to_string()))
    }

Is there any reason to do this conversion?

@Thomblin
Copy link

Thomblin commented Dec 5, 2023

To which cookie function do you refer? I implemented this according to https://docs.rs/actix-web/latest/actix_web/struct.HttpResponse.html#method.add_cookie which performs the same check to validate the cookie

@kukuro-kt
Copy link
Author

To which cookie function do you refer?

This one: https://docs.rs/actix-web/latest/actix_web/struct.HttpResponseBuilder.html#method.cookie

By the way, this implementation saves error to self instead of returning a Result

@Thomblin
Copy link

Thomblin commented Dec 6, 2023

self.append_header expects hdr_val to implement TryIntoHeaderPair which returns (HeaderName, HeaderValue), so in the end it is the same as HeaderValue::from_str if I understand that correctly

self.append_header((header::SET_COOKIE, cookie.to_string())) should not work either as impl<V> TryIntoHeaderPair for (String, V) returns Result<(HeaderName, HeaderValue), Self::Error>

@kukuro-kt
Copy link
Author

kukuro-kt commented Dec 6, 2023

I mean, although customize() has not supported cookie() or add_cookie() yet, there is still a simple way to add cookie:

actix_web::HttpResponse::Ok()
  .json(...)
  .customize()
  .append_header((header::COOKIE, cookie.to_string())); //no need to unwrap() or `?`

append_header() provided by CustomizeResponder does not return a Result but saves error to self:

Err(err) => self.error = Some(err.into()),

However, the add_cookie() you implemented returns a Result, so we have to write like this:

actix_web::HttpResponse::Ok()
  .json(...)
  .customize()
  .add_cookie(cookie)?; //need to unwrap() or `?`

Return a Result here may not be in line with the api design concept of CustomizeResponder?

@Thomblin
Copy link

Thomblin commented Dec 6, 2023

I see your point. I guess it depends if you prefer to fail silently or not. As far as I understand Rust, the idea is usually to make possible errors transparent by using the Result type. I don't know why there are two different implementations to add a cookie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web C-feature Category: new functionality good-first-issue easy to pick up for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants