allow non-'static HTTP method names #5

Merged
MightyPork merged 1 commits from :pr-method-lifetime into master 4 years ago
Ghost commented 4 years ago
There is no content yet.
Poster

Besides supporting rtsp_types::Method::Extension, this allows cleaner code.

Besides supporting rtsp_types::Method::Extension, this [allows cleaner code](https://github.com/scottlamb/moonfire-playground/commit/e77360d0d6433751d06e72bca5cf7cc9cb1f9c4a).
Owner

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'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](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
Poster

I'd actually prefer to avoid it:

  • When my crate isn't even using HTTP (the protocol), http (the crate) doesn't pull its weight. It's 10,000 lines of code and 54 unsafes (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.
  • I don't think http is universal when using HTTP either. Eg the tide/surf folks made http-types instead. ureq and minreq 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 the Display conversion and/or in http::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 via method.as_str().

I'd actually prefer to avoid it: * When my crate isn't even using HTTP (the protocol), `http` (the crate) doesn't pull its weight. It's 10,000 lines of code and 54 `unsafe`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. * I don't think `http` is universal when using HTTP either. Eg the tide/surf folks [made](https://blog.yoshuawuyts.com/async-http/#shared-abstractions) `http-types` instead. `ureq` and `minreq` 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 the `Display` conversion and/or in `http::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 via `method.as_str()`.
MightyPork closed this pull request 4 years ago
Owner

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 other From 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 to struct HttpMethod<'a>(Cow<'a, str>) and turning the GET/POST/HEAD variants into its associated constants. That would eliminate some needless tests

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 other `From` 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` to `struct HttpMethod<'a>(Cow<'a, str>)` and turning the GET/POST/HEAD variants into its associated constants. That would eliminate some needless tests
Owner

Okay, I went ahead and did just that, it's now struct HttpMethod<'a>(Cow<'a, str>). I feel much better about this implementation.

Okay, I went ahead and did just that, it's now `struct HttpMethod<'a>(Cow<'a, str>)`. I feel much better about this implementation.
Owner

v0.3.0 is now published

v0.3.0 is now published
Poster

Thank you! It works fine for me.

Out of curiosity: why do you feel better about the Cow? My understanding was the AuthContext (and thus things it uses like the HttpMethod) can be short-lived. That's how I'm using it now. So I don't know when HttpMethod would need to take ownership of the method string.

Thank you! It works fine for me. Out of curiosity: why do you feel better about the `Cow`? My understanding was the `AuthContext` (and thus things it uses like the `HttpMethod`) can be short-lived. That's how I'm using it now. So I don't know when `HttpMethod` would need to take ownership of the method string.
Owner

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 owned String from their enum. Maybe it will be useful for some other crates...

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 owned `String` from their enum. Maybe it will be useful for some other crates...
The pull request has been merged as a1f3c6ebfb.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: packages/digest_auth_rs#5
Loading…
There is no content yet.