FlashSystems / tokio-fastcgi

Async FastCGI handler library for Tokio.rs
Other
5 stars 2 forks source link

Add get_param_keys. #1

Closed aaronriekenberg closed 2 years ago

aaronriekenberg commented 2 years ago

Would you consider the following change to allow access to an iterator over the Request parameter keys?

I could not find a way currently to do this but maybe I am missing something.

Happy to change the name of the method or add more tests.

Thanks!

FlashSystems commented 2 years ago

Hello @aaronriekenberg, thank you for your contribution. I think an gat_param_keys method would be worthwhile.

I'm currently unsure if just returning the keys is a good approach. If you want the values too - which is likely - you have to call get_param or get_str_param for each of them. This is wasteful because the map will be searched for the value again. Maybe a get_params method that returns an iterator over all parameters and their values would be more useful and efficient.

I have to think about is some more and would value your input on this.

aaronriekenberg commented 2 years ago

@FlashSystems Thanks for the review.

Maybe a better design is to add a get_params() that returns the HashMap by reference:

    pub fn get_params(&self) -> &HashMap<String, Vec<u8>> {
        &self.params
    }

The advantage over get_param_keys is the user can iterate over keys or values or call any methods on HashMap. This avoids wrapping more methods of the HashMap.

One disadvantage is if the user wants to use the values of the map as strings - the user must convert from Vec<u8> to String as get_str_param does.

My use case is I want to iterate over all request parameters to find all HTTP headers sent as fastcgi parameters.

Maybe get_params is the best general solution?

FlashSystems commented 2 years ago

I've implemented a params_iter and str_params_iter methods in the params_iter branch. Take a look if this suits your use-case. I opted against exposing the whole HashMap because - for my taste - this leaks to many details about the implementation. Rust iterators have a bunch of powerful methods that should cover almost any use-case.

aaronriekenberg commented 2 years ago

I think your str_params_iter will work perfectly for my use case.

This is a very elegant solution. I have learned something on rust iterators this looks very useful 😄

Thank you!