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.

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 new CURLINFO_ constants and fn response_http_version() #539

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarijnS95
Copy link

Fixes #538

Use CURLINFO_HTTP_VERSION (a new constant for the curl-sys crate) to determine the HTTP version used in the last/latest connection.

Comment on lines 3138 to 3142
curl_sys::CURL_HTTP_VERSION_1_0 => HttpVersion::V10,
curl_sys::CURL_HTTP_VERSION_1_1 => HttpVersion::V11,
curl_sys::CURL_HTTP_VERSION_2_0 => HttpVersion::V2,
curl_sys::CURL_HTTP_VERSION_3 => HttpVersion::V3,
_ => unreachable!("TODO"),
Copy link
Author

Choose a reason for hiding this comment

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

According to https://curl.se/libcurl/c/CURLINFO_HTTP_VERSION.html this only returns a select few codes, but should we map them all "just in case"? It can also return 0 in case the version cannot be determined, should we map that "none" to Any or wrap this in an Option? And should we wrap other faulty codes into a Result of sorts or panic on invalid library return values?

Sorry, still a bit new to this crate and curl, while adding this change on a tangent of a tangent :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should assume that curl could return anything here, and the last thing we want to do is panic when encountering an unknown result. Curl is often dynamically linked, meaning we don't have control over which version of libcurl is actually used. If old Rust code using old curl-rust code starts to run against a new libcurl version due to a system upgrade that starts returning a hypothetical "HTTP 4", then we:

  1. For sure should not panic on any return value
  2. If we can, return the underlying i32 even though we don't understand it, allowing the caller to attempt to understand it themselves if they want to

One possible way of doing this would be to wrap the return value in a new enum that maybe looks something like this:

pub enum HttpVersionInfo {
    HttpVersion(HttpVersion),
    Unknown(i32),
}

Copy link
Author

Choose a reason for hiding this comment

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

That conflicts a bit with the use of a Result type. I'd rather wrap the existing Result<HttpVersion, CurlError> with a enum HttpVersionSpecificError { CurlError(CurlError), UnknownValue(i32) } of sorts :)

Copy link
Author

Choose a reason for hiding this comment

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

I ended up using your suggestion as Unknown isn't immediately invalid.

In another crate we've had discussions about a user hypothetically matching Unknown(0x1337), and the crate being bumped with a semver-compatible release (or breaking release, doesn't really matter here) that maps the new SuperDuperHttpVersion = 0x1337 value in this #[non_exhaustive] enum. The user will suddenly run into their mandatory _ => {} catch-all arm again. Not giving them access to the value entices contributions and requests upstream to match the new value, and have less ambiguity inbetween.

///
/// Corresponds to `CURLINFO_HTTP_VERSION` and may return an error if the
/// option isn't supported.
pub fn response_http_version(&self) -> Result<HttpVersion, Error> {
Copy link
Collaborator

@sagebind sagebind Nov 26, 2023

Choose a reason for hiding this comment

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

This function ideally would be called http_version to match the curl constant, but that's already taken. Perhaps instead should be called get_http_version. We want the names to match the underlying curl names as much as possible, even if it is less understandable.

Copy link
Author

@MarijnS95 MarijnS95 Nov 26, 2023

Choose a reason for hiding this comment

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

Very understandable, but that name is already taken for the setter unfortunately.

EDIT: Great ninja-edit 😬

Copy link
Author

Choose a reason for hiding this comment

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

Note that get_ prefixes are discouraged in Rust, but let's just use it here.

Comment on lines -431 to 432
#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum HttpVersion {
Copy link
Author

Choose a reason for hiding this comment

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

Curious why simple enums have to be compared by match/if let and are not allowed to be used in simple == by deriving these quite-standard Eq traits. Intentional or oversight?

Use `CURLINFO_HTTP_VERSION` (a new constant for the `curl-sys` crate) to
determine the HTTP version used in the last/latest connection.
@MarijnS95 MarijnS95 marked this pull request as ready for review January 7, 2024 11:08
@MarijnS95
Copy link
Author

Looks like the CI isn't too happy about the new constants added to curl-sys, will have to find a way to update "something" here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP response version
2 participants