allow non-'static HTTP method names #5
Merged
MightyPork
merged 1 commits from :pr-method-lifetime
into master
4 years ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch ':pr-method-lifetime'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Besides supporting rtsp_types::Method::Extension, this allows cleaner code.
I'll look at this, it seems fine at a glance. But what do you think of using the
http
crate's enum instead?https://docs.rs/http/0.2.4/http/method/struct.Method.html
That seems to be the de facto universal API for HTTP methods, many crates use it.
I never got around to switching to it... Maybe it could be behind a feature flag
I'd actually prefer to avoid it:
http
(the crate) doesn't pull its weight. It's 10,000 lines of code and 54unsafe
s (which are probably fine but add some auditing effort) for literally no value. There are probably a handful of other non-HTTP protocols that use digest authentication. Wikipedia mentions SIP at least.http
is universal when using HTTP either. Eg the tide/surf folks madehttp-types
instead.ureq
andminreq
use neither.My ideal would be just to take a byte slice rather than either have a distinct
digest_auth::Method
or depend on an extra crate. The caller is guaranteed to have a byte slice ready to go, as they've either received the method name or are about to send it. Doing any validation (http::Method::from_bytes
is fallible) seems silly when calculating a hash is a fine thing to do with untrusted bytes. And allocating (in theDisplay
conversion and/or inhttp::Method::from_bytes
when the method name exceeds 15 bytes) is unnecessary too. Even branches aren't necessary. I'd prefer dead-simple code, although I'm not resource-constrained enough that it's really a deal-breaker.Easy conversion from
http::Method
is a virtue, but just using a byte slice allows that viamethod.as_str()
.Improved it a bit, now the http crate is an optional feature, and I changed OTHER to use Cow instead of
&str
so it better supports owned strings. There's now some otherFrom
impls and unit tests too.Can you check if this works for your application before I make the release?
I don't see any advantage in using byte slices when headers are always ASCII (or maybe UTF-8). However, one thing I can imagine is changing
HttpMethod
tostruct HttpMethod<'a>(Cow<'a, str>)
and turning the GET/POST/HEAD variants into its associated constants. That would eliminate some needless testsOkay, I went ahead and did just that, it's now
struct HttpMethod<'a>(Cow<'a, str>)
. I feel much better about this implementation.v0.3.0 is now published
Thank you! It works fine for me.
Out of curiosity: why do you feel better about the
Cow
? My understanding was theAuthContext
(and thus things it uses like theHttpMethod
) can be short-lived. That's how I'm using it now. So I don't know whenHttpMethod
would need to take ownership of the method string.Good to hear. I like the flexibility of Cow, you can avoid dealing with lifetimes if you have an owned string. I already used Cow a lot in the crate, so it felt natural to continue.
You're right, it doesn't add much benefit here. Most users of the crate will use the constants like GET, so this is more of an implementation detail. I hoped it would benefit the
http
crate support, but sadly you can't extract the ownedString
from their enum. Maybe it will be useful for some other crates...a1f3c6ebfb
.