Initial implementation of Method #8

carllerche · 2017-03-16T22:54:19Z

This PR adds a dependency on the bytes crate, but does not expose it at all in the public API. It also adds a str::Str type that indicates that the inner Bytes is UTF-8 encoded, but the intension is to not expose this type at all. It will be reused as part of other types.

Continuation of #3 & #5

seanmonstar · 2017-03-16T23:15:22Z

src/method.rs

+    /// let method = Method::extension("FOO");
+    /// assert_eq!(method, "FOO");
+    /// ```
+    pub fn extension(s: &str) -> Method {


So, something I've noticed a while ago in hyper is that the Method::Extension(String) constructor allows constructing an invalid method. It's bothered me a little, and maybe this is the time to fix it.

Perhaps instead of easily constructing with any string at all, we'd need to at least check that the string only contains characters in the legal range. According to RFC7230, here's the ABNF:

method = token token = 1*tchar tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

So, maybe we have two options to get an extension:

A FromStr implementation

Method::from_str_unchecked(s: &str) -> Method

The first option would parse, and if it is a standard method, it would set it to the right enum variant.

The second option will always set it to the extension, and it is up to the caller to ensure that the given string is a valid method and not actually one of the existing standard methods.

carllerche · 2017-03-17T00:03:38Z

We may decide not to use bytes in this case because the Bytes struct is fairly large. In this case, it may be better to use:

enum Extension {
    Inline { data: [u8; 7], len: u8 },
    Allocated(String),
}

Given this list of methods: https://www.iana.org/assignments/http-methods/http-methods.xhtml, most methods are under 7 chars.

However, this is an internal detail, so I'm not super worried right now.

carllerche · 2017-03-17T05:36:14Z

Ok, I removed bytes for this case. HTTP methods are going to be usually pretty small, so the zero-copy functionality of bytes is over kill. By not using it, we save some space! woohoo.

alexcrichton

I might as a .as_str() method as well (shorthand for the .as_ref() impl) to just explicitly have a method for converting to a string.

alexcrichton · 2017-03-17T14:18:33Z

src/method.rs

+pub struct Method(Inner);
+
+#[derive(Debug)]
+pub struct FromBytesError;


I'd make this an opaque struct (with zero size) so we can extend it in the future if need be. Something like:

pub struct FromBytesError { _priv: () }

alexcrichton · 2017-03-17T14:20:23Z

src/method.rs

+    /// method, i.e. one that is defined as a constant in this module.
+    ///
+    /// The function is unsafe as the input is not checked as valid UTF-8.
+    pub unsafe fn from_bytes_unchecked(src: &[u8]) -> Method {


Naively I'd expect us to not have this and instead of From<&str> for Method and let users do their own str::from_utf8_unchecked perhaps?

Possibly, but see above too. I would rather not have to always do string comparison. That would defeat the point of having an enum at all.

alexcrichton · 2017-03-17T14:22:18Z

src/method.rs

+    pub fn is_safe(&self) -> bool {
+        match self.0 {
+            Get | Head | Options | Trace => true,
+            _ => false


I think this may be incorrect in the face of from_bytes_unchecked because we could hav ExtensionInline(b"GET"), right?

Same as above.

alexcrichton · 2017-03-17T14:23:18Z

src/method.rs

+    }
+
+    fn cause(&self) -> Option<&Error> {
+        None


This is a default method so I think it's safe to omit

alexcrichton · 2017-03-17T14:24:40Z

src/method.rs

+/// Currently includes 8 variants representing the 8 methods defined in
+/// [RFC 7230](https://tools.ietf.org/html/rfc7231#section-4.1), plus PATCH,
+/// and an Extension variant for all extensions.
+#[derive(Clone, PartialEq, Eq, Hash, Debug)]


I think the derived PartailEq and Hash may also be incorrect in the presence of from_bytes_unchecked (similar to below) as it's possible to have Inner::Get and Inner::ExtensionInline(b"GET")

But that is the point! We want to avoid string comparisons if we don't have to, and from_bytes_unchecked is unsafe. So, if you do it wrong, that is on you :)

carllerche · 2017-03-17T15:20:05Z

I'm going to remove from_bytes_unchecked for now in favor of debating it later.

alexcrichton · 2017-03-17T15:22:50Z

I guess as a naive user I would expect:

assert_eq!(Method::from("GET"), GET);

for all variants of Method::from that we have. I understand the performance implications, but it's just way too surprising for this to be a core library and to have such an assertion fail.

carllerche · 2017-03-17T15:24:09Z

@alexcrichton well, that's why From shouldn't be used for it :)

Anyway, I got rid of the fn, so we can debate it later.

seanmonstar

Looking good to me.

seanmonstar · 2017-03-18T05:56:33Z

src/method.rs

+
+#[derive(Debug)]
+pub struct FromBytesError {
+    _priv: (),


I again wonder if InvalidMethodError is more descriptive..

Initial implementation of Method

3428825

carllerche requested review from seanmonstar and alexcrichton March 16, 2017 22:54

seanmonstar reviewed Mar 16, 2017

View reviewed changes

Add method extensions

b7d41a5

carllerche force-pushed the method branch from cb8ce41 to b7d41a5 Compare March 17, 2017 05:44

alexcrichton reviewed Mar 17, 2017

View reviewed changes

carllerche mentioned this pull request Mar 17, 2017

Consider adding Method::from_bytes_unchecked #9

Open

Remove Method::from_bytes_unchecked

f4795fd

carllerche added 2 commits March 17, 2017 08:25

Don't reimplement default fn

bdadf2b

Don't have FromBytesError be a unit struct

9611ecb

alexcrichton approved these changes Mar 17, 2017

View reviewed changes

carllerche added 2 commits March 17, 2017 08:34

Merge branch 'master' into method

1d18cdc

Add Method::as_str and impl as_ref in terms of it

611f338

seanmonstar approved these changes Mar 18, 2017

View reviewed changes

carllerche merged commit edeaf0b into master Mar 18, 2017

carllerche deleted the method branch March 18, 2017 19:34

carllerche mentioned this pull request Mar 18, 2017

Method type #3

Closed

Uh oh!

Initial implementation of Method #8

Initial implementation of Method #8

Uh oh!

Conversation

carllerche commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllerche commented Mar 17, 2017

Uh oh!

carllerche commented Mar 17, 2017

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllerche commented Mar 17, 2017

Uh oh!

alexcrichton commented Mar 17, 2017

Uh oh!

carllerche commented Mar 17, 2017

Uh oh!

seanmonstar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!