Skip to content

Add unchecked constructors for FuelClient#3183

Open
MitchTurner wants to merge 15 commits intomasterfrom
bugfix/add-unchecked-constructor-for-fuel-client
Open

Add unchecked constructors for FuelClient#3183
MitchTurner wants to merge 15 commits intomasterfrom
bugfix/add-unchecked-constructor-for-fuel-client

Conversation

@MitchTurner
Copy link
Copy Markdown
Member

Linked Issues/PRs

Closes #3176

Description

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@MitchTurner MitchTurner marked this pull request as ready for review January 22, 2026 17:19
@MitchTurner MitchTurner requested review from a team, Dentosal and xgreenx as code owners January 22, 2026 17:19
@cursor
Copy link
Copy Markdown

cursor bot commented Jan 22, 2026

PR Summary

Medium Risk
Medium risk because it changes how FuelClient constructors interpret URLs (no longer auto-appending/overwriting /v1/graphql), which can break callers that relied on implicit normalization.

Overview
Client URL construction is refactored to remove implicit GraphQL endpoint normalization. normalize_url is made pub, FuelClient::new is deprecated (with guidance toward a forthcoming new_unchecked), and FuelClient::with_urls/new_with_rpc now mostly just parse URLs (only adding an http:// scheme when missing) instead of forcing the /v1/graphql path.

Call sites and tests are updated accordingly: e2e tests normalize endpoints explicitly before calling with_urls, the CLI and DoS test switch from new to with_urls, and the previous normalization-focused unit tests are removed/adjusted. A changelog entry is added for the new constructor work.

Written by Cursor Bugbot for commit 9f40c05. This will update automatically on new commits. Configure here.

cursor[bot]

This comment was marked as outdated.

}

#[cfg(feature = "rpc")]
pub async fn new_unchecked_with_rpc<G: AsRef<str>, R: AsRef<str>>(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

new_with_rpc is not released, you can make this function automatically not affect the URL

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's remove this function, we have new_with_rpc which is enough

Self::from_str(url.as_ref())
}

pub fn new_unchecked(url: impl AsRef<str>) -> anyhow::Result<Self> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just deprecate the new method, we want people to start to use the new constructors with multiple URLs.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I meant we don't need new_unchecked anymore. we have method that accepts multiple endpoints

Ok(client)
}

pub fn with_urls(urls: &[impl AsRef<str>]) -> anyhow::Result<Self> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 normalization.

cursor[bot]

This comment was marked as outdated.

@MitchTurner MitchTurner requested a review from rymnc as a code owner January 22, 2026 22:24
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@MitchTurner MitchTurner self-assigned this Jan 23, 2026
Copy link
Copy Markdown
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

I still we want to do at least instead of just Url::parse

    if !raw_url.starts_with("http") {
        raw_url = format!("http://{raw_url}");
    }

Comment on lines +22 to +23
use tempfile::TempDir;
// Used for writing assertions // Run programs
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
use tempfile::TempDir;
// Used for writing assertions // Run programs
use tempfile::TempDir;

Self::from_str(url.as_ref())
}

pub fn new_unchecked(url: impl AsRef<str>) -> anyhow::Result<Self> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I meant we don't need new_unchecked anymore. we have method that accepts multiple endpoints

}

#[cfg(feature = "rpc")]
pub async fn new_unchecked_with_rpc<G: AsRef<str>, R: AsRef<str>>(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's remove this function, we have new_with_rpc which is enough

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

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.

FuelClient::new removes potentially critical path information

2 participants