add `Method` & `StatusCode` to azure_core #849

ctaggart · 2022-06-23T15:57:56Z

This removes the last dependency on the http crate by adding a Method and StatusCode types. I'm interested in having compatibility with http-types and supporting surf in addition to reqwest. This draft wraps http-types implementations of Method and StatusCode, but exposes the API from http crate that we are using. There are a few different ways we could go here. Looking for feedback.

rylev · 2022-06-24T12:53:03Z

sdk/core/src/method.rs

@@ -1,4 +1,5 @@
-use std::ops::Deref;
+use crate::error::{Error, ErrorKind};
+use std::{ops::Deref, str::FromStr};

 #[derive(PartialEq, Eq, Clone, Debug)]


Why not Copy?

rylev · 2022-06-24T12:56:17Z

sdk/core/src/http_client/reqwest.rs

+
+/// Construct a new `HttpClient` with the `reqwest` backend.
+pub fn new_http_client() -> std::sync::Arc<dyn HttpClient> {
+    std::sync::Arc::new(::reqwest::Client::new())


Suggested change

std::sync::Arc::new(::reqwest::Client::new())

std::sync::Arc::new(reqwest::Client::new())

I put these in a module named reqwest, so needed that prefix. Could rename the module.

Hmmm... or you could rename the 3rd party crate. use ::request as hyperium_request.

rylev · 2022-06-24T12:56:30Z

sdk/core/src/http_client/reqwest.rs

+}
+
+#[async_trait]
+impl HttpClient for ::reqwest::Client {


Suggested change

impl HttpClient for ::reqwest::Client {

impl HttpClient for reqwest::Client {

rylev · 2022-06-24T13:03:02Z

sdk/core/src/http_client/reqwest.rs

+}
+
+fn try_from_method(method: &crate::Method) -> crate::Result<::reqwest::Method> {
+    ::reqwest::Method::from_str(method.as_ref()).map_kind(ErrorKind::DataConversion)


Suggested change

::reqwest::Method::from_str(method.as_ref()).map_kind(ErrorKind::DataConversion)

reqwest::Method::from_str(method.as_ref()).map_kind(ErrorKind::DataConversion)

This should never happen. I wonder if we should have a wrapper error that says something like: if you ever see this, please report a bug.

rylev · 2022-06-24T13:04:00Z

sdk/core/src/http_client/reqwest.rs

+    http_types::StatusCode::try_from(status)
+        .map_err(|_| {
+            Error::with_message(ErrorKind::DataConversion, || {
+                format!("invalid status code {status}")
+            })
+        })
+        .map(crate::StatusCode)


We should use crate::StatusCode's TryFrom<u16> impl

rylev · 2022-06-24T13:06:01Z

sdk/core/src/status_code.rs

+    }
+    pub fn from_u16(value: u16) -> crate::Result<Self> {


Suggested change

}

pub fn from_u16(value: u16) -> crate::Result<Self> {

}

pub fn from_u16(value: u16) -> crate::Result<Self> {

rylev · 2022-06-24T13:07:32Z

sdk/core/src/status_code.rs

+    pub const TOO_MANY_REQUESTS: StatusCode = StatusCode(http_types::StatusCode::TooManyRequests);
+}
+
+impl Default for StatusCode {


Not sure I like this impl. OK does not logically feel like the "default" status code to me.

rylev · 2022-06-24T13:08:13Z

sdk/data_tables/Cargo.toml

 log = "0.4"
-once_cell = "1.0"
+# once_cell = "1.0"


Can remove. http crate doesn’t have Mehod::MERGE.

rylev · 2022-06-24T13:08:58Z

sdk/data_tables/src/responses/submit_transaction_response.rs

+                        .parse::<u16>()
+                        .map_err(|_| {
+                            Error::message(ErrorKind::DataConversion, "invalid HTTP status code")
+                        })?;


I think StatusCode has a TryFrom<&str> impl no?

rylev · 2022-06-24T13:09:45Z

sdk/data_tables/src/transaction.rs

@@ -40,7 +40,7 @@ impl Transaction {
        for transaction_operation in self.transaction_operations.iter() {
            s.push_str(&format!("--changeset_{}\nContent-Type: application/http\nContent-Transfer-Encoding: binary\n\n", self.change_set_uuid.hyphenated()));
            s.push_str(&format!(
-                "{} {} HTTP/1.1\n",
+                "{:?} {} HTTP/1.1\n",


Not sure I like that method doesn't have a display. The string representations of the HTTP methods are cononical no?

The string representations of the HTTP methods are cononical no?

Correct. As per RFC 9110, 9: Methods:

The method token is case-sensitive because it might be used as a gateway to object-based systems with case-sensitive method names. By convention, standardized methods are defined in all-uppercase US-ASCII letters.

Bigger question is do we want to use the http-types StatusCode directly. This wrapper shows what the differences are, where the gaps are.

yoshuawuyts

I feel like I'm missing some context about direction. Obviously as the lead author of http-types I'm thrilled by the prospect that we may want to use it for the azure SDK. But I want to make sure that we properly understand and discuss the tradeoffs of doing so. Because if we're going to adopt it, I want to make sure we're doing so for the right reasons.

For example: should we expose http-types types from the Rust Azure SDK's public API? If we do that, how do we feel about the governance? Are there any reasons beyond the code changes in this API why we may want to adopt this?

I think it would be worth laying out a vision of how you see the integration working out in the longer term. And then maybe we can discuss it offline here / perhaps bring it to a meeting to discuss further?

yoshuawuyts · 2022-06-24T14:28:53Z

sdk/core/src/status_code.rs

+    pub const BAD_GATEWAY: StatusCode = StatusCode(http_types::StatusCode::BadGateway);
+    pub const GATEWAY_TIMEOUT: StatusCode = StatusCode(http_types::StatusCode::GatewayTimeout);
+    pub const INTERNAL_SERVER_ERROR: StatusCode =
+        StatusCode(http_types::StatusCode::InternalServerError);
+    pub const NOT_MODIFIED: StatusCode = StatusCode(http_types::StatusCode::NotModified);
+    pub const OK: StatusCode = StatusCode(http_types::StatusCode::Ok);
+    pub const REQUEST_TIMEOUT: StatusCode = StatusCode(http_types::StatusCode::RequestTimeout);
+    pub const SERVICE_UNAVAILABLE: StatusCode =
+        StatusCode(http_types::StatusCode::ServiceUnavailable);
+    pub const TOO_MANY_REQUESTS: StatusCode = StatusCode(http_types::StatusCode::TooManyRequests);
+}


I'm so-so about this construct. In my opinion status codes are best represented in Rust as non-exhaustive enums; providing a balance between extensibility and enumerability.

I feel like we'd be better off either re-exporting http_types::StatusCode as-is, or if we don't want to rely on using http-types in our public API, either copying over the implementation - or creating a wrapper/type-alias to it.

cataggar · 2022-06-25T17:52:26Z

We'll discuss this in our next meeting. A purpose of this PR was to show the differences between http and http-type implementations and start a discussion. I prefer http-types representations of both Method & StatusCode as enums.

https://docs.rs/http-types/latest/src/http_types/method.rs.html

#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
pub enum Method {

https://docs.rs/http-types/latest/src/http_types/status_code.rs.html

#[repr(u16)]
#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
pub enum StatusCode {

http instead has consts for all the values.

#[derive(Clone, PartialEq, Eq, Hash)]
pub struct Method(Inner);

enum Inner {
    Options,
    Get,

https://docs.rs/http/latest/src/http/status.rs.html

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct StatusCode(NonZeroU16);

Differences

The biggest change needed for http-types is error the need for its error type to implement std::error::Error trouble mapping TryFrom errors #848 refactor: overhaul errors http-rs/http-types#395.
http::StatusCode has a fn as_u16, http-types is missing it and uses as u16 everywhere. The convenience function would be nice.
http::StatusCode has a default StatusCode::OK. Some of our Default implementations use this. Should be easy to figure out.
Because http-types uses an enum instead of constants, the name casing is different. Please BAD_GATEWAY with BadGateway and the 7 other status codes we use. This is just a few global replaces.

I created #852 with a subset of changes that leave http in place for now.

ctaggart added 2 commits June 23, 2022 11:49

add Method & StatusCode to azure_core

42ffe87

mocks

138e73a

ctaggart marked this pull request as ready for review June 23, 2022 20:48

rylev suggested changes Jun 24, 2022

View reviewed changes

yoshuawuyts reviewed Jun 24, 2022

View reviewed changes

cataggar mentioned this pull request Jun 25, 2022

add Method & StatusCode to azure_core #852

Merged

cataggar closed this Jun 25, 2022

ctaggart mentioned this pull request Jun 27, 2022

switch Method and StatusCode to http-types #860

Merged

ctaggart deleted the http-types3 branch July 5, 2022 18:14

cataggar mentioned this pull request May 15, 2024

Consider replacing http-types crate with http crate #1644

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `Method` & `StatusCode` to azure_core #849

add `Method` & `StatusCode` to azure_core #849

ctaggart commented Jun 23, 2022 •

edited by cataggar

Loading

rylev Jun 24, 2022

rylev Jun 24, 2022

cataggar Jun 24, 2022

rylev Jun 24, 2022

rylev Jun 24, 2022

rylev Jun 24, 2022

rylev Jun 24, 2022

rylev Jun 24, 2022

rylev Jun 24, 2022

rylev Jun 24, 2022

cataggar Jun 24, 2022

rylev Jun 24, 2022

rylev Jun 24, 2022

yoshuawuyts Jun 24, 2022

cataggar Jun 24, 2022

yoshuawuyts left a comment •

edited

Loading

yoshuawuyts Jun 24, 2022 •

edited

Loading

cataggar commented Jun 25, 2022

	std::sync::Arc::new(::reqwest::Client::new())
	std::sync::Arc::new(reqwest::Client::new())

	impl HttpClient for ::reqwest::Client {
	impl HttpClient for reqwest::Client {

	::reqwest::Method::from_str(method.as_ref()).map_kind(ErrorKind::DataConversion)
	reqwest::Method::from_str(method.as_ref()).map_kind(ErrorKind::DataConversion)

add Method & StatusCode to azure_core #849

add Method & StatusCode to azure_core #849

Conversation

ctaggart commented Jun 23, 2022 • edited by cataggar Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts left a comment • edited Loading

Choose a reason for hiding this comment

yoshuawuyts Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

cataggar commented Jun 25, 2022

add `Method` & `StatusCode` to azure_core #849

add `Method` & `StatusCode` to azure_core #849

ctaggart commented Jun 23, 2022 •

edited by cataggar

Loading

yoshuawuyts left a comment •

edited

Loading

yoshuawuyts Jun 24, 2022 •

edited

Loading