-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add unchecked constructors for FuelClient
#3183
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
ef3a020
acf1960
a9bb871
ff7bd11
122f80d
d2e1845
4d3e17e
a89fc08
21e1a87
087ea1e
23d4e6e
cc2f8bd
5084c4a
0d4db6d
9f40c05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add unchecked constructor for FuelClient |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -325,7 +325,7 @@ impl Default for AWSClientManager { | |
| } | ||
|
|
||
| /// Normalizes a URL string by ensuring it has an http(s) scheme and the `/v1/graphql` path. | ||
| fn normalize_url(url_str: &str) -> anyhow::Result<Url> { | ||
| pub fn normalize_url(url_str: &str) -> anyhow::Result<Url> { | ||
| let mut raw_url = url_str.to_string(); | ||
| if !raw_url.starts_with("http") { | ||
| raw_url = format!("http://{raw_url}"); | ||
|
|
@@ -383,17 +383,36 @@ pub fn from_strings_errors_to_std_error(errors: Vec<String>) -> io::Error { | |
| } | ||
|
|
||
| impl FuelClient { | ||
| #[deprecated(since = "0.47.1", note = "Use `new_unchecked` instead")] | ||
| pub fn new(url: impl AsRef<str>) -> anyhow::Result<Self> { | ||
| Self::from_str(url.as_ref()) | ||
| } | ||
|
|
||
| pub fn new_unchecked(url: impl AsRef<str>) -> anyhow::Result<Self> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just deprecate the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant we don't need |
||
| let url = Url::parse(&url.as_ref()) | ||
| .map_err(anyhow::Error::msg) | ||
| .with_context(|| format!("Invalid fuel-core URL: {:?}", url.as_ref()))?; | ||
|
|
||
| Ok(Self { | ||
| transport: FailoverTransport::new(vec![url])?, | ||
| require_height: ConsistencyPolicy::Auto { | ||
| height: Arc::new(Mutex::new(None)), | ||
| }, | ||
| chain_state_info: Default::default(), | ||
| #[cfg(feature = "rpc")] | ||
| rpc_client: None, | ||
| #[cfg(feature = "rpc")] | ||
| aws_client: AWSClientManager::new(), | ||
| }) | ||
| } | ||
|
|
||
| #[cfg(feature = "rpc")] | ||
| pub async fn new_with_rpc<G: AsRef<str>, R: AsRef<str>>( | ||
| graph_ql_urls: impl Iterator<Item = G>, | ||
| rpc_url: R, | ||
| ) -> anyhow::Result<Self> { | ||
| let urls: Vec<_> = graph_ql_urls | ||
| .map(|str| normalize_url(str.as_ref())) | ||
| .map(|str| Url::parse(str.as_ref())) | ||
|
cursor[bot] marked this conversation as resolved.
cursor[bot] marked this conversation as resolved.
cursor[bot] marked this conversation as resolved.
|
||
| .try_collect()?; | ||
| let mut client = Self::with_urls(&urls)?; | ||
| let mut raw_rpc_url = <R as AsRef<str>>::as_ref(&rpc_url).to_string(); | ||
|
|
@@ -406,13 +425,29 @@ impl FuelClient { | |
| Ok(client) | ||
| } | ||
|
|
||
| #[cfg(feature = "rpc")] | ||
| pub async fn new_unchecked_with_rpc<G: AsRef<str>, R: AsRef<str>>( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered that but felt like it was inconsistent. I can change it though.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove this function, we have |
||
| graph_ql_urls: impl Iterator<Item = G>, | ||
| rpc_url: R, | ||
| ) -> anyhow::Result<Self> { | ||
| let urls: Vec<_> = graph_ql_urls | ||
| .map(|str| Url::parse(str.as_ref())) | ||
| .try_collect()?; | ||
| let mut client = Self::with_urls(&urls)?; | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
|
||
| let raw_rpc_url = <R as AsRef<str>>::as_ref(&rpc_url).to_string(); | ||
| let rpc_client = ProtoBlockAggregatorClient::connect(raw_rpc_url).await?; | ||
| client.rpc_client = Some(rpc_client); | ||
| client.aws_client = AWSClientManager::new(); | ||
| Ok(client) | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
cursor[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| pub fn with_urls(urls: &[impl AsRef<str>]) -> anyhow::Result<Self> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is only used by us, so we can also alter its behaviour and do not do |
||
| if urls.is_empty() { | ||
| return Err(anyhow!("Failed to create FuelClient. No URL is provided.")); | ||
| } | ||
| let urls = urls | ||
| .iter() | ||
| .map(|url| normalize_url(url.as_ref())) | ||
| .map(|url| Url::parse(url.as_ref())) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| Ok(Self { | ||
| transport: FailoverTransport::new(urls)?, | ||
|
|
@@ -1944,58 +1979,14 @@ impl FuelClient { | |
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn with_urls_normalizes_urls_to_graphql_endpoint() { | ||
| // Given | ||
| let urls = &["http://localhost:8080", "http://example.com:4000"]; | ||
|
|
||
| // When | ||
| let client = FuelClient::with_urls(urls).expect("should create client"); | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| client.get_default_url().as_str(), | ||
| "http://localhost:8080/v1/graphql" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn with_urls_adds_http_scheme_if_missing() { | ||
| // Given | ||
| let urls = &["localhost:8080"]; | ||
|
|
||
| // When | ||
| let client = FuelClient::with_urls(urls).expect("should create client"); | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| client.get_default_url().as_str(), | ||
| "http://localhost:8080/v1/graphql" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn with_urls_overwrites_existing_path() { | ||
| // Given - URLs that already have some path | ||
| let urls = &["http://localhost:8080/some/path", "http://example.com/api"]; | ||
|
|
||
| // When | ||
| let client = FuelClient::with_urls(urls).expect("should create client"); | ||
|
|
||
| // Then - path should be normalized to /v1/graphql | ||
| assert_eq!( | ||
| client.get_default_url().as_str(), | ||
| "http://localhost:8080/v1/graphql" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn new_and_with_urls_produce_same_url() { | ||
| // Given | ||
| let url = "http://localhost:8080"; | ||
| let url = "http://localhost:8080/v1/graphql"; | ||
|
|
||
| // When | ||
| let client_new = FuelClient::new(url).expect("should create client via new"); | ||
| let client_new = | ||
| FuelClient::new_unchecked(url).expect("should create client via new"); | ||
| let client_with_urls = | ||
| FuelClient::with_urls(&[url]).expect("should create client via with_urls"); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.