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.

Sign up for GitHub

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

Add Clone for Request/Response/Extensions #574

Closed
wants to merge 1 commit into from
Closed

Add Clone for Request/Response/Extensions #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ bytes = "1"
fnv = "1.0.5"
itoa = "1"

anymap = "1.0.0-beta.2"
Copy link
Member

Choose a reason for hiding this comment

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

Could we provide the functionality without the extra dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was my initial goal but to poc I just used it and as you can see its patched so we probably want to vend anyways.


[dev-dependencies]
indexmap = "<=1.8"
quickcheck = "0.9.0"
Expand Down Expand Up @@ -62,3 +64,6 @@ path = "benches/method.rs"
[[bench]]
name = "uri"
path = "benches/uri.rs"

[patch.crates-io]
anymap = { git = "https://github.com/LucioFranco/anymap" }
97 changes: 52 additions & 45 deletions src/extensions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use std::any::{Any, TypeId};
use std::collections::HashMap;
use anymap::{CloneAny, Map};
use std::fmt;
use std::hash::{BuildHasherDefault, Hasher};
use std::hash::Hasher;

type AnyMap = HashMap<TypeId, Box<dyn Any + Send + Sync>, BuildHasherDefault<IdHasher>>;
type AnyMap = Map<dyn CloneAny + Send + Sync>;

// With TypeIds as keys, there's no need to hash them. They are already hashes
// themselves, coming from the compiler. The IdHasher just holds the u64 of
Expand Down Expand Up @@ -31,7 +30,7 @@ impl Hasher for IdHasher {
///
/// `Extensions` can be used by `Request` and `Response` to store
/// extra data derived from the underlying protocol.
#[derive(Default)]
#[derive(Default, Clone)]
Copy link

@hlbarber hlbarber Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The derive(Clone) here means that the NotCloneExtension will remain in the clone* AnyMap, as NotCloneExtension(None). Is this what we want?

An alternative here might be:

  • The HashMap store values as enum Value { Clone(Box<dyn CloneAny>), NotClone(Box<dyn Any>) },
  • Clone filters out the Value::NotClone,
  • Have two methods fn insert and fn insert_clone, only the latter requiring Clone.

Copy link
Member Author

Choose a reason for hiding this comment

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

The derive(Clone) here means that the NotCloneExtension will remain in the new AnyMap, as NotCloneExtension(None). Is this what we want?

Im not totally following what you mean by this?

Copy link

@hlbarber hlbarber Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

let mut extensions_a = Extensions::new();
extensions_a.insert(NotCloneExtension::new(3_u32));
let extensions_b = extensions_a.clone();
let value = extensions_b.get::<NotCloneExtension<u32>>();

value here is Some(NotCloneExtension(None)) from what I read from the implementation. Is this what we want? If we take the changes bulleted above, then we could filter out the non-Clone extensions.

Having two separate methods fn insert and fn insert_clone then storing them as Value::NotClone and Value::Clone respectively, has the additional benefit of:

  • It's an addition rather than a breaking change.
  • It doesn't require a newtype when using get - e.g. get::<T> will retrieve T when it's inserted via insert or insert_clone. This is in contrast to get::<NotCloneExtension<T>> and get::<T>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right but now get returns an enum rather than option , I guess we could have the enum be three variants though not sure if that is better or not?

Copy link

@hlbarber hlbarber Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Something like this is what I was imagining

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=318f42c9a2477340c76ddf52aeefd908

It doesn't compile yet, but if we used some more of the anymap tricks I guess it would?

pub struct Extensions {
// If extensions are never used, no need to carry around an empty HashMap.
// That's 3 words. Instead, this is only 1 word.
Expand Down Expand Up @@ -59,16 +58,10 @@ impl Extensions {
/// assert!(ext.insert(4u8).is_none());
/// assert_eq!(ext.insert(9i32), Some(5i32));
/// ```
pub fn insert<T: Send + Sync + 'static>(&mut self, val: T) -> Option<T> {
pub fn insert<T: Clone + Send + Sync + 'static>(&mut self, val: T) -> Option<T> {
self.map
.get_or_insert_with(|| Box::new(HashMap::default()))
.insert(TypeId::of::<T>(), Box::new(val))
.and_then(|boxed| {
(boxed as Box<dyn Any + 'static>)
.downcast()
.ok()
.map(|boxed| *boxed)
})
.get_or_insert_with(|| Box::new(AnyMap::new()))
.insert(val)
}

/// Get a reference to a type previously inserted on this `Extensions`.
Expand All @@ -83,11 +76,8 @@ impl Extensions {
///
/// assert_eq!(ext.get::<i32>(), Some(&5i32));
/// ```
pub fn get<T: Send + Sync + 'static>(&self) -> Option<&T> {
self.map
.as_ref()
.and_then(|map| map.get(&TypeId::of::<T>()))
.and_then(|boxed| (&**boxed as &(dyn Any + 'static)).downcast_ref())
pub fn get<T: Clone + Send + Sync + 'static>(&self) -> Option<&T> {
self.map.as_ref().and_then(|map| map.get::<T>())
}

/// Get a mutable reference to a type previously inserted on this `Extensions`.
Expand All @@ -102,11 +92,8 @@ impl Extensions {
///
/// assert_eq!(ext.get::<String>().unwrap(), "Hello World");
/// ```
pub fn get_mut<T: Send + Sync + 'static>(&mut self) -> Option<&mut T> {
self.map
.as_mut()
.and_then(|map| map.get_mut(&TypeId::of::<T>()))
.and_then(|boxed| (&mut **boxed as &mut (dyn Any + 'static)).downcast_mut())
pub fn get_mut<T: Clone + Send + Sync + 'static>(&mut self) -> Option<&mut T> {
self.map.as_mut().and_then(|map| map.get_mut::<T>())
}

/// Remove a type from this `Extensions`.
Expand All @@ -122,16 +109,8 @@ impl Extensions {
/// assert_eq!(ext.remove::<i32>(), Some(5i32));
/// assert!(ext.get::<i32>().is_none());
/// ```
pub fn remove<T: Send + Sync + 'static>(&mut self) -> Option<T> {
self.map
.as_mut()
.and_then(|map| map.remove(&TypeId::of::<T>()))
.and_then(|boxed| {
(boxed as Box<dyn Any + 'static>)
.downcast()
.ok()
.map(|boxed| *boxed)
})
pub fn remove<T: Clone + Send + Sync + 'static>(&mut self) -> Option<T> {
self.map.as_mut().and_then(|map| map.remove::<T>())
}

/// Clear the `Extensions` of all inserted extensions.
Expand Down Expand Up @@ -166,9 +145,7 @@ impl Extensions {
/// ```
#[inline]
pub fn is_empty(&self) -> bool {
self.map
.as_ref()
.map_or(true, |map| map.is_empty())
self.map.as_ref().map_or(true, |map| map.is_empty())
}

/// Get the numer of extensions available.
Expand All @@ -184,28 +161,26 @@ impl Extensions {
/// ```
#[inline]
pub fn len(&self) -> usize {
self.map
.as_ref()
.map_or(0, |map| map.len())
self.map.as_ref().map_or(0, |map| map.len())
}

/// Extends `self` with another `Extensions`.
///
/// If an instance of a specific type exists in both, the one in `self` is overwritten with the
/// one from `other`.
///
///
/// # Example
///
///
/// ```
/// # use http::Extensions;
/// let mut ext_a = Extensions::new();
/// ext_a.insert(8u8);
/// ext_a.insert(16u16);
///
///
/// let mut ext_b = Extensions::new();
/// ext_b.insert(4u8);
/// ext_b.insert("hello");
///
///
/// ext_a.extend(ext_b);
/// assert_eq!(ext_a.len(), 3);
/// assert_eq!(ext_a.get::<u8>(), Some(&4u8));
Expand All @@ -229,9 +204,41 @@ impl fmt::Debug for Extensions {
}
}

/// A newtype that enables non clonable items to be added
/// to the extension map.
#[derive(Debug)]
pub struct NotCloneExtension<T>(Option<T>);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to include this convenience type within http itself.


impl<T> NotCloneExtension<T> {
/// Create a new `NotClone` with `T`.
pub fn new(inner: T) -> Self {
Self(Some(inner))
}

/// Get an immutable reference to `T`.
///
/// Returns None, if this is a clone of the original type.
pub fn get(&self) -> Option<&T> {
self.0.as_ref()
}

/// Get a mutable reference to `T`.
///
/// Returns None, if this is a clone of the original type.
pub fn get_mut(&mut self) -> Option<&mut T> {
self.0.as_mut()
}
}

impl<T> Clone for NotCloneExtension<T> {
fn clone(&self) -> Self {
Self(None)
}
}

#[test]
fn test_extensions() {
#[derive(Debug, PartialEq)]
#[derive(Clone, Debug, PartialEq)]
struct MyType(i32);

let mut extensions = Extensions::new();
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ mod error;
mod extensions;

pub use crate::error::{Error, Result};
pub use crate::extensions::Extensions;
pub use crate::extensions::{Extensions, NotCloneExtension};
#[doc(no_inline)]
pub use crate::header::{HeaderMap, HeaderName, HeaderValue};
pub use crate::method::Method;
Expand Down
22 changes: 6 additions & 16 deletions src/request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
//! ```

use std::any::Any;
use std::convert::{TryFrom};
use std::convert::TryFrom;
use std::fmt;

use crate::header::{HeaderMap, HeaderName, HeaderValue};
Expand Down Expand Up @@ -154,6 +154,7 @@ use crate::{Extensions, Result, Uri};
/// #
/// # fn main() {}
/// ```
#[derive(Clone)]
pub struct Request<T> {
head: Parts,
body: T,
Expand All @@ -163,6 +164,7 @@ pub struct Request<T> {
///
/// The HTTP request head consists of a method, uri, version, and a set of
/// header fields.
#[derive(Clone)]
pub struct Parts {
/// The request's method
pub method: Method,
Expand Down Expand Up @@ -231,7 +233,6 @@ impl Request<()> {
where
Uri: TryFrom<T>,
<Uri as TryFrom<T>>::Error: Into<crate::Error>,

{
Builder::new().method(Method::GET).uri(uri)
}
Expand All @@ -254,7 +255,6 @@ impl Request<()> {
where
Uri: TryFrom<T>,
<Uri as TryFrom<T>>::Error: Into<crate::Error>,

{
Builder::new().method(Method::PUT).uri(uri)
}
Expand All @@ -277,7 +277,6 @@ impl Request<()> {
where
Uri: TryFrom<T>,
<Uri as TryFrom<T>>::Error: Into<crate::Error>,

{
Builder::new().method(Method::POST).uri(uri)
}
Expand All @@ -300,7 +299,6 @@ impl Request<()> {
where
Uri: TryFrom<T>,
<Uri as TryFrom<T>>::Error: Into<crate::Error>,

{
Builder::new().method(Method::DELETE).uri(uri)
}
Expand All @@ -324,7 +322,6 @@ impl Request<()> {
where
Uri: TryFrom<T>,
<Uri as TryFrom<T>>::Error: Into<crate::Error>,

{
Builder::new().method(Method::OPTIONS).uri(uri)
}
Expand All @@ -347,7 +344,6 @@ impl Request<()> {
where
Uri: TryFrom<T>,
<Uri as TryFrom<T>>::Error: Into<crate::Error>,

{
Builder::new().method(Method::HEAD).uri(uri)
}
Expand All @@ -370,7 +366,6 @@ impl Request<()> {
where
Uri: TryFrom<T>,
<Uri as TryFrom<T>>::Error: Into<crate::Error>,

{
Builder::new().method(Method::CONNECT).uri(uri)
}
Expand Down Expand Up @@ -987,7 +982,7 @@ impl Builder {
/// ```
pub fn extension<T>(self, extension: T) -> Builder
where
T: Any + Send + Sync + 'static,
T: Any + Clone + Send + Sync + 'static,
{
self.and_then(move |mut head| {
head.extensions.insert(extension);
Expand Down Expand Up @@ -1051,19 +1046,14 @@ impl Builder {
/// .unwrap();
/// ```
pub fn body<T>(self, body: T) -> Result<Request<T>> {
self.inner.map(move |head| {
Request {
head,
body,
}
})
self.inner.map(move |head| Request { head, body })
}

// private

fn and_then<F>(self, func: F) -> Self
where
F: FnOnce(Parts) -> Result<Parts>
F: FnOnce(Parts) -> Result<Parts>,
{
Builder {
inner: self.inner.and_then(func),
Expand Down
13 changes: 5 additions & 8 deletions src/response.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ use crate::{Extensions, Result};
/// #
/// # fn main() {}
/// ```
#[derive(Clone)]
pub struct Response<T> {
head: Parts,
body: T,
Expand All @@ -185,6 +186,7 @@ pub struct Response<T> {
///
/// The HTTP response head consists of a status, version, and a set of
/// header fields.
#[derive(Clone)]
pub struct Parts {
/// The response's status
pub status: StatusCode,
Expand Down Expand Up @@ -690,7 +692,7 @@ impl Builder {
/// ```
pub fn extension<T>(self, extension: T) -> Builder
where
T: Any + Send + Sync + 'static,
T: Any + Clone + Send + Sync + 'static,
{
self.and_then(move |mut head| {
head.extensions.insert(extension);
Expand Down Expand Up @@ -754,19 +756,14 @@ impl Builder {
/// .unwrap();
/// ```
pub fn body<T>(self, body: T) -> Result<Response<T>> {
self.inner.map(move |head| {
Response {
head,
body,
}
})
self.inner.map(move |head| Response { head, body })
}

// private

fn and_then<F>(self, func: F) -> Self
where
F: FnOnce(Parts) -> Result<Parts>
F: FnOnce(Parts) -> Result<Parts>,
{
Builder {
inner: self.inner.and_then(func),
Expand Down